Skip to content

Add transport layer that will be used by generated BiDi Modules#17758

Open
pujagani wants to merge 10 commits into
SeleniumHQ:trunkfrom
pujagani:add-transport-layer
Open

Add transport layer that will be used by generated BiDi Modules#17758
pujagani wants to merge 10 commits into
SeleniumHQ:trunkfrom
pujagani:add-transport-layer

Conversation

@pujagani

@pujagani pujagani commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🔗 Related Issues

Add a thin transport layer that the generated BiDi modules can use in future.

💥 What does this PR do?

It as a mechanism to provide an handle to BiDi. End user cannot use to to call any methods to it but it is used to pass to BiDi module's constructor to send commands and subscribe to events.

🔧 Implementation Notes

🤖 AI assistance

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

💡 Additional Considerations

🔄 Types of changes

  • New feature

@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jul 9, 2026
@qodo-code-review

qodo-code-review Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Introduce opaque BiDi Handle and Module base for generated BiDi APIs

✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Add package-scoped BiDi Handle to send commands and manage event subscriptions.
• Introduce Module base class for generated BiDi modules to reuse the driver’s BiDi connection.
• Extend HasBiDi/RemoteWebDriver to expose Handle without exposing BiDi internals to users.
Diagram

graph TD
  A["Client code"] --> B["WebDriver"] --> C["HasBiDi"] --> D["Handle"] --> E["BiDi"] --> F[("WebSocket")]
  G["Generated Module"] --> D

  subgraph Legend
    direction LR
    _user["Caller"] ~~~ _api["API / Interface"] ~~~ _cls["Class"] ~~~ _ws[("Transport")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Define a Transport interface (send/subscribe/unsubscribe) implemented by BiDi
  • ➕ Clearer abstraction boundary than a concrete Handle wrapper
  • ➕ Easier to mock for unit tests; no need to expose BiDi at all
  • ➖ Larger public API surface and more types to maintain
  • ➖ Requires refactoring BiDi to implement the interface, potentially wider impact
2. Make generated modules accept BiDi directly (package-private constructors)
  • ➕ Fewer new types; modules call existing BiDi APIs directly
  • ➖ Harder to keep BiDi usage constrained as code generation evolves
  • ➖ Increases risk of accidental API exposure and tighter coupling to BiDi internals
3. Expose only Optional and keep modules in same package
  • ➕ No new interface method on HasBiDi; avoids expanding implementer requirements
  • ➖ Pushes null/optional handling into every module constructor
  • ➖ Still encourages direct dependence on BiDi rather than a narrow seam

Recommendation: The Handle approach is a good incremental seam: it narrows what generated modules can do (send/subscribe/unsubscribe) while reusing the existing shared BiDi connection. If mocking/testing becomes a priority, consider evolving Handle into a small Transport interface later; for now, keeping it as a concrete, package-scoped facade minimizes disruption.

Files changed (6) +133 / -0

Enhancement (6) +133 / -0
BiDi.javaExpose BiDi as an opaque Handle for internal module use +4/-0

Expose BiDi as an opaque Handle for internal module use

• Adds BiDi#asHandle() to create a package-usable transport facade over the active BiDi connection. This supports passing a constrained capability to generated modules without exposing BiDi methods directly.

java/src/org/openqa/selenium/bidi/BiDi.java

BiDiProvider.javaImplement HasBiDi#getHandle() in BiDiProvider wrapper +5/-0

Implement HasBiDi#getHandle() in BiDiProvider wrapper

• Extends the BiDiProvider anonymous implementation to return a Handle derived from the lazily-created BiDi instance. Keeps the BiDi acquisition pattern while enabling module construction via Handle.

java/src/org/openqa/selenium/bidi/BiDiProvider.java

Handle.javaAdd package-scoped Handle facade for send/subscribe/unsubscribe +49/-0

Add package-scoped Handle facade for send/subscribe/unsubscribe

• Introduces a new public type whose constructor and methods are package-private, making it effectively opaque outside org.openqa.selenium.bidi. Wraps BiDi to provide a minimal surface for commands and event subscription lifecycle.

java/src/org/openqa/selenium/bidi/Handle.java

HasBiDi.javaExpand HasBiDi contract to provide a Handle +2/-0

Expand HasBiDi contract to provide a Handle

• Adds getHandle() to the HasBiDi interface, establishing the supported way for modules to obtain a transport handle from a driver. Leaves maybeGetBiDi() as the deprecated path.

java/src/org/openqa/selenium/bidi/HasBiDi.java

Module.javaAdd Module base class for generated BiDi modules +55/-0

Add Module base class for generated BiDi modules

• Introduces a @Beta abstract base that validates driver BiDi support and obtains a Handle from HasBiDi. Provides protected helpers for sending commands and managing event subscriptions via the shared connection.

java/src/org/openqa/selenium/bidi/Module.java

RemoteWebDriver.javaProvide Handle from RemoteWebDriver with compatibility fallback +18/-0

Provide Handle from RemoteWebDriver with compatibility fallback

• Implements HasBiDi#getHandle() by calling maybeGetBiDi() (rather than reading the private field) to avoid breaking subclasses that override maybeGetBiDi(). Throws a targeted BiDiException when BiDi is unavailable/misconfigured.

java/src/org/openqa/selenium/remote/RemoteWebDriver.java

@qodo-code-review

qodo-code-review Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 18 rules

Grey Divider


Action required

1. No tests for Handle API 📘 Rule violation ▣ Testability
Description
This PR introduces new BiDi transport/public-surface behavior (Handle, Module, and
BiDi#asHandle()), but the change set contains no corresponding tests exercising these new code
paths. This increases the risk of regressions and makes the new API contract undocumented by
automation.
Code

java/src/org/openqa/selenium/bidi/Module.java[R36-54]

+  protected Module(WebDriver driver) {
+    Require.nonNull("WebDriver", driver);
+    if (!(driver instanceof HasBiDi)) {
+      throw new BiDiException("WebDriver does not support BiDi protocol");
+    }
+    this.handle = ((HasBiDi) driver).getHandle();
+  }
+
+  protected final <X> X send(Command<X> command) {
+    return handle.send(command);
+  }
+
+  protected final <X> String subscribe(Event<X> event, Consumer<X> handler) {
+    return handle.subscribe(event, handler);
+  }
+
+  protected final void unsubscribe(String subscriptionId) {
+    handle.unsubscribe(subscriptionId);
+  }
Evidence
PR adds new transport-layer API entry points (BiDi#asHandle() and the new Module base class
delegating through Handle), but no tests were added/updated in this change set to validate the new
behavior as required.

Rule 389273: Require tests for all new functionality and bug fixes
java/src/org/openqa/selenium/bidi/BiDi.java[171-172]
java/src/org/openqa/selenium/bidi/Module.java[36-54]

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 BiDi transport-layer functionality was added (`Handle`, `Module`, and driver `getHandle()` plumbing) without any accompanying automated tests.

## Issue Context
This PR adds new public-facing behavior in the Java binding (e.g., `BiDi#asHandle()` and `Module` delegating through `Handle`). Without tests, future refactors can silently break module command sending/subscription behavior.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[171-172]
- java/src/org/openqa/selenium/bidi/Module.java[36-54]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[480-494]

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


2. RemoteWebDriver.getHandle() missing Javadoc 📘 Rule violation ✧ Quality
Description
New/changed public API methods were added without proper Javadoc blocks:
RemoteWebDriver#getHandle() is documented only with // comments, and the public methods
BiDi#asHandle() and HasBiDi#getHandle() likewise lack preceding /** ... */ Javadoc, violating
the requirement for complete Javadoc on public API methods.
Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R480-494]

+  // Calls maybeGetBiDi() rather than reading the private field directly so that subclasses
+  // that override maybeGetBiDi() (e.g. AppiumDriver) are not broken before they migrate to
+  // overriding getHandle(). Remove the @SuppressWarnings and switch to the field once
+  // maybeGetBiDi() is deleted.
+  @SuppressWarnings("deprecation")
+  @Override
+  public Handle getHandle() {
+    return maybeGetBiDi()
+        .map(BiDi::asHandle)
+        .orElseThrow(
+            () ->
+                new BiDiException(
+                    "Check if this browser version supports BiDi and if the"
+                        + " 'webSocketUrl: true' capability is set."));
+  }
Evidence
The checklist requires a Javadoc block for each changed/new public API method. In the diff,
RemoteWebDriver adds public Handle getHandle() with only line (//) comments rather than a `/**
... */ block immediately above the signature, and both BiDi#asHandle()` (a public, non-void
method) and the added HasBiDi#getHandle() in the public HasBiDi interface similarly have no
preceding Javadoc, demonstrating non-compliance with the documentation requirement.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[480-494]
java/src/org/openqa/selenium/bidi/BiDi.java[173-179]
java/src/org/openqa/selenium/bidi/HasBiDi.java[23-43]

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

## Issue description
Several new/changed public API methods added in this PR are missing proper Javadoc blocks (`/** ... */`) immediately above their signatures: `RemoteWebDriver#getHandle()` currently has only `//` line comments, and `BiDi#asHandle()` plus `HasBiDi#getHandle()` have no Javadoc at all.

## Issue Context
Public API methods must use Javadoc (not line comments) to meet documentation requirements; for non-void return types, include at least one descriptive sentence and an `@return` tag (or use an appropriate form such as `/** {@inheritDoc} */` where applicable).

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[480-494]
- java/src/org/openqa/selenium/bidi/BiDi.java[177-179]
- java/src/org/openqa/selenium/bidi/HasBiDi.java[42-42]

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


3. Unsubscribe leaks remote subscription ✓ Resolved 🐞 Bug ☼ Reliability
Description
Module.unsubscribe delegates to Handle.unsubscribe, which only removes the local callback
(BiDi.removeListener) and never issues session.unsubscribe, so the browser can keep streaming
events even after callers “unsubscribe”. Because Connection.isEventSubscribed is derived from the
local callback map, once the last listener is removed the later BiDi.clearListener(event) path
will not send session.unsubscribe, making the remote subscription effectively impossible to clean
up for that event within the same connection.
Code

java/src/org/openqa/selenium/bidi/Handle.java[R42-48]

+  <X> long subscribe(Event<X> event, Consumer<X> handler) {
+    return bidi.addListener(event, handler);
+  }
+
+  void unsubscribe(long id) {
+    bidi.removeListener(id);
+  }
Evidence
The new transport calls BiDi.addListener (which sends session.subscribe) but its unsubscribe
path calls BiDi.removeListener, which only removes local callbacks and never sends
session.unsubscribe. Local subscription state is removed when the last callback is removed, so
subsequent clearListener(event) won’t send an unsubscribe either, leaving the browser-side
subscription active.

java/src/org/openqa/selenium/bidi/Handle.java[42-48]
java/src/org/openqa/selenium/bidi/Module.java[48-54]
java/src/org/openqa/selenium/bidi/BiDi.java[89-96]
java/src/org/openqa/selenium/bidi/BiDi.java[128-137]
java/src/org/openqa/selenium/bidi/BiDi.java[165-167]
java/src/org/openqa/selenium/bidi/Connection.java[205-231]

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

### Issue description
`Handle.unsubscribe(long)` maps to `BiDi.removeListener(long)` which only mutates local listener state; it does not send `session.unsubscribe`. This creates a mismatch with `subscribe`, which does send `session.subscribe`, leading to leaked remote subscriptions and unnecessary event traffic.

### Issue Context
- `BiDi.addListener` sends `session.subscribe` on every subscription.
- `BiDi.removeListener` only removes local callbacks.
- `BiDi.clearListener(event)` only sends `session.unsubscribe` when `Connection.isEventSubscribed(event)` is true, but `Connection.removeListener` removes the event entry when the last local listener is removed.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Handle.java[38-48]
- java/src/org/openqa/selenium/bidi/Module.java[44-54]
- java/src/org/openqa/selenium/bidi/BiDi.java[89-167]
- java/src/org/openqa/selenium/bidi/Connection.java[205-254]

### Suggested direction
- Introduce a subscription type that retains enough info to unsubscribe on the wire (e.g., `Subscription` holding `Event`, optional context IDs, and local listener id), and expose `unsubscribe(Subscription)` / `Subscription.close()`.
- Alternatively, teach `Connection.removeListener`/`BiDi.removeListener` to perform a `session.unsubscribe` when the last local handler for an event is removed (requires tracking whether the subscription was global vs context-scoped, leveraging `contextListenerIds` in `BiDi`).

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


View more (1)
4. HasBiDi.getHandle() breaks implementers ✗ Dismissed 📘 Rule violation ⌂ Architecture
Description
A new abstract method Handle getHandle() was added to the existing public interface HasBiDi
without a default implementation, which forces every existing implementer to update and is a
source/binary incompatible change.
Code

java/src/org/openqa/selenium/bidi/HasBiDi.java[42]

+  Handle getHandle();
Evidence
The checklist requires newly added interface methods to have a default implementation (or documented
justification) to avoid breaking existing implementers. The diff shows Handle getHandle(); added
to HasBiDi as an abstract method with no default body.

Rule 330197: Default implementations for new interface methods
Rule 389266: Maintain backward-compatible public API and ABI
java/src/org/openqa/selenium/bidi/HasBiDi.java[42-42]

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

## Issue description
`HasBiDi` gained a new abstract method (`getHandle()`) without a default implementation, which is not backward-compatible for existing implementers.

## Issue Context
This PR adds `Handle` as a transport layer for generated BiDi modules. Adding a non-default method to a public interface forces downstream implementers to implement it immediately.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/HasBiDi.java[23-43]

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



Remediation recommended

5. No context-scoped subscribe API 🐞 Bug ⚙ Maintainability
Description
Handle/Module only expose subscribe(Event, Consumer) and omit the context-scoped subscription
overloads (contexts parameter) that the underlying BiDi API provides, forcing future generated
modules to subscribe globally even when they need per-browsing-context scoping. Existing code
already relies on context-scoped subscription (BiDi.addListener(Set<String>, ...)), so this
limitation is likely to cause extra noise/traffic or prevent implementing the same behavior through
the new transport layer.
Code

java/src/org/openqa/selenium/bidi/Handle.java[R38-44]

+  <X> X send(Command<X> command) {
+    return bidi.send(command);
+  }
+
+  <X> long subscribe(Event<X> event, Consumer<X> handler) {
+    return bidi.addListener(event, handler);
+  }
Evidence
Handle.subscribe only supports global subscriptions, while BiDi has explicit overloads for
context-scoped subscriptions that send session.subscribe with contexts. Current modules already
use the context-scoped overload, demonstrating this is needed behavior in-repo.

java/src/org/openqa/selenium/bidi/Handle.java[38-44]
java/src/org/openqa/selenium/bidi/BiDi.java[98-126]
java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java[62-67]

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 new transport layer (`Handle`/`Module`) only supports global event subscription. The existing BiDi API supports context-scoped subscriptions via `session.subscribe` with a `contexts` parameter, and current modules already use it.

### Issue Context
Without context-scoped subscribe methods on `Handle`, generated modules cannot reproduce the current behavior of subscribing only for specific browsing contexts.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Handle.java[38-44]
- java/src/org/openqa/selenium/bidi/Module.java[44-54]
- java/src/org/openqa/selenium/bidi/BiDi.java[98-126]

### Suggested direction
- Add `Handle.subscribe(String browsingContextId, Event<X>, Consumer<X>)` and `Handle.subscribe(Set<String> browsingContextIds, Event<X>, Consumer<X>)` delegating to the matching `BiDi.addListener` overloads.
- Expose corresponding `protected final` overloads on `Module` so generated modules can use them without touching `Handle` directly.

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



Informational

6. Handle lacks @beta annotation ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Handle is introduced as a public API type and is exposed via HasBiDi#getHandle(), but unlike the
surrounding BiDi surface it is not annotated @Beta, which can mis-signal it as a stable/committed
API despite being intended as an internal-ish transport handle.
Code

java/src/org/openqa/selenium/bidi/Handle.java[R22-31]

+/**
+ * An opaque handle to the active BiDi connection, used by {@link Module} subclasses to send
+ * commands and subscribe to events.
+ *
+ * <p>Constructor and all methods are package-private: external callers that hold a {@code Handle}
+ * reference cannot invoke it, only pass it to module constructors inside this package. Create an
+ * instance via {@link BiDi#asHandle()}.
+ */
+public class Handle {
+
Evidence
The new Handle type is public and is part of the public API surface via HasBiDi#getHandle(), but it
is not marked @Beta while adjacent BiDi entry points are @Beta, creating inconsistent API stability
signaling.

java/src/org/openqa/selenium/bidi/Handle.java[22-31]
java/src/org/openqa/selenium/bidi/HasBiDi.java[23-43]
java/src/org/openqa/selenium/bidi/Module.java[25-34]
java/src/org/openqa/selenium/bidi/BiDi.java[29-35]

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

### Issue description
`org.openqa.selenium.bidi.Handle` is a newly added **public** type and is returned from the public `HasBiDi#getHandle()` API, but it is not annotated with `@Beta` even though the rest of the BiDi-facing surface in this area is.

This is primarily an API-signaling/maintenance problem: it can encourage downstream code to treat `Handle` as stable even if Selenium intends to keep it flexible while generated BiDi modules are evolving.

### Issue Context
`HasBiDi` is annotated `@Beta` and now exposes `Handle` directly. `Module` and `BiDi` are also `@Beta`, but `Handle` itself is not.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Handle.java[18-31]

### Suggested fix
Add `import org.openqa.selenium.Beta;` and annotate the class with `@Beta` (or otherwise make the intended stability explicit).

ⓘ 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 ffe777f

Results up to commit 8c1da41


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


Action required
1. RemoteWebDriver.getHandle() missing Javadoc 📘 Rule violation ✧ Quality
Description
New/changed public API methods were added without proper Javadoc blocks:
RemoteWebDriver#getHandle() is documented only with // comments, and the public methods
BiDi#asHandle() and HasBiDi#getHandle() likewise lack preceding /** ... */ Javadoc, violating
the requirement for complete Javadoc on public API methods.
Code

java/src/org/openqa/selenium/remote/RemoteWebDriver.java[R480-494]

+  // Calls maybeGetBiDi() rather than reading the private field directly so that subclasses
+  // that override maybeGetBiDi() (e.g. AppiumDriver) are not broken before they migrate to
+  // overriding getHandle(). Remove the @SuppressWarnings and switch to the field once
+  // maybeGetBiDi() is deleted.
+  @SuppressWarnings("deprecation")
+  @Override
+  public Handle getHandle() {
+    return maybeGetBiDi()
+        .map(BiDi::asHandle)
+        .orElseThrow(
+            () ->
+                new BiDiException(
+                    "Check if this browser version supports BiDi and if the"
+                        + " 'webSocketUrl: true' capability is set."));
+  }
Evidence
The checklist requires a Javadoc block for each changed/new public API method. In the diff,
RemoteWebDriver adds public Handle getHandle() with only line (//) comments rather than a `/**
... */ block immediately above the signature, and both BiDi#asHandle()` (a public, non-void
method) and the added HasBiDi#getHandle() in the public HasBiDi interface similarly have no
preceding Javadoc, demonstrating non-compliance with the documentation requirement.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/remote/RemoteWebDriver.java[480-494]
java/src/org/openqa/selenium/bidi/BiDi.java[173-179]
java/src/org/openqa/selenium/bidi/HasBiDi.java[23-43]

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

## Issue description
Several new/changed public API methods added in this PR are missing proper Javadoc blocks (`/** ... */`) immediately above their signatures: `RemoteWebDriver#getHandle()` currently has only `//` line comments, and `BiDi#asHandle()` plus `HasBiDi#getHandle()` have no Javadoc at all.

## Issue Context
Public API methods must use Javadoc (not line comments) to meet documentation requirements; for non-void return types, include at least one descriptive sentence and an `@return` tag (or use an appropriate form such as `/** {@inheritDoc} */` where applicable).

## Fix Focus Areas
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[480-494]
- java/src/org/openqa/selenium/bidi/BiDi.java[177-179]
- java/src/org/openqa/selenium/bidi/HasBiDi.java[42-42]

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


2. HasBiDi.getHandle() breaks implementers ✗ Dismissed 📘 Rule violation ⌂ Architecture
Description
A new abstract method Handle getHandle() was added to the existing public interface HasBiDi
without a default implementation, which forces every existing implementer to update and is a
source/binary incompatible change.
Code

java/src/org/openqa/selenium/bidi/HasBiDi.java[42]

+  Handle getHandle();
Evidence
The checklist requires newly added interface methods to have a default implementation (or documented
justification) to avoid breaking existing implementers. The diff shows Handle getHandle(); added
to HasBiDi as an abstract method with no default body.

Rule 330197: Default implementations for new interface methods
Rule 389266: Maintain backward-compatible public API and ABI
java/src/org/openqa/selenium/bidi/HasBiDi.java[42-42]

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

## Issue description
`HasBiDi` gained a new abstract method (`getHandle()`) without a default implementation, which is not backward-compatible for existing implementers.

## Issue Context
This PR adds `Handle` as a transport layer for generated BiDi modules. Adding a non-default method to a public interface forces downstream implementers to implement it immediately.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/HasBiDi.java[23-43]

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


3. Unsubscribe leaks remote subscription ✓ Resolved 🐞 Bug ☼ Reliability
Description
Module.unsubscribe delegates to Handle.unsubscribe, which only removes the local callback
(BiDi.removeListener) and never issues session.unsubscribe, so the browser can keep streaming
events even after callers “unsubscribe”. Because Connection.isEventSubscribed is derived from the
local callback map, once the last listener is removed the later BiDi.clearListener(event) path
will not send session.unsubscribe, making the remote subscription effectively impossible to clean
up for that event within the same connection.
Code

java/src/org/openqa/selenium/bidi/Handle.java[R42-48]

+  <X> long subscribe(Event<X> event, Consumer<X> handler) {
+    return bidi.addListener(event, handler);
+  }
+
+  void unsubscribe(long id) {
+    bidi.removeListener(id);
+  }
Evidence
The new transport calls BiDi.addListener (which sends session.subscribe) but its unsubscribe
path calls BiDi.removeListener, which only removes local callbacks and never sends
session.unsubscribe. Local subscription state is removed when the last callback is removed, so
subsequent clearListener(event) won’t send an unsubscribe either, leaving the browser-side
subscription active.

java/src/org/openqa/selenium/bidi/Handle.java[42-48]
java/src/org/openqa/selenium/bidi/Module.java[48-54]
java/src/org/openqa/selenium/bidi/BiDi.java[89-96]
java/src/org/openqa/selenium/bidi/BiDi.java[128-137]
java/src/org/openqa/selenium/bidi/BiDi.java[165-167]
java/src/org/openqa/selenium/bidi/Connection.java[205-231]

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

### Issue description
`Handle.unsubscribe(long)` maps to `BiDi.removeListener(long)` which only mutates local listener state; it does not send `session.unsubscribe`. This creates a mismatch with `subscribe`, which does send `session.subscribe`, leading to leaked remote subscriptions and unnecessary event traffic.

### Issue Context
- `BiDi.addListener` sends `session.subscribe` on every subscription.
- `BiDi.removeListener` only removes local callbacks.
- `BiDi.clearListener(event)` only sends `session.unsubscribe` when `Connection.isEventSubscribed(event)` is true, but `Connection.removeListener` removes the event entry when the last local listener is removed.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Handle.java[38-48]
- java/src/org/openqa/selenium/bidi/Module.java[44-54]
- java/src/org/openqa/selenium/bidi/BiDi.java[89-167]
- java/src/org/openqa/selenium/bidi/Connection.java[205-254]

### Suggested direction
- Introduce a subscription type that retains enough info to unsubscribe on the wire (e.g., `Subscription` holding `Event`, optional context IDs, and local listener id), and expose `unsubscribe(Subscription)` / `Subscription.close()`.
- Alternatively, teach `Connection.removeListener`/`BiDi.removeListener` to perform a `session.unsubscribe` when the last local handler for an event is removed (requires tracking whether the subscription was global vs context-scoped, leveraging `contextListenerIds` in `BiDi`).

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



Remediation recommended
4. No context-scoped subscribe API 🐞 Bug ⚙ Maintainability
Description
Handle/Module only expose subscribe(Event, Consumer) and omit the context-scoped subscription
overloads (contexts parameter) that the underlying BiDi API provides, forcing future generated
modules to subscribe globally even when they need per-browsing-context scoping. Existing code
already relies on context-scoped subscription (BiDi.addListener(Set<String>, ...)), so this
limitation is likely to cause extra noise/traffic or prevent implementing the same behavior through
the new transport layer.
Code

java/src/org/openqa/selenium/bidi/Handle.java[R38-44]

+  <X> X send(Command<X> command) {
+    return bidi.send(command);
+  }
+
+  <X> long subscribe(Event<X> event, Consumer<X> handler) {
+    return bidi.addListener(event, handler);
+  }
Evidence
Handle.subscribe only supports global subscriptions, while BiDi has explicit overloads for
context-scoped subscriptions that send session.subscribe with contexts. Current modules already
use the context-scoped overload, demonstrating this is needed behavior in-repo.

java/src/org/openqa/selenium/bidi/Handle.java[38-44]
java/src/org/openqa/selenium/bidi/BiDi.java[98-126]
java/src/org/openqa/selenium/bidi/module/SpeculationInspector.java[62-67]

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 new transport layer (`Handle`/`Module`) only supports global event subscription. The existing BiDi API supports context-scoped subscriptions via `session.subscribe` with a `contexts` parameter, and current modules already use it.

### Issue Context
Without context-scoped subscribe methods on `Handle`, generated modules cannot reproduce the current behavior of subscribing only for specific browsing contexts.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Handle.java[38-44]
- java/src/org/openqa/selenium/bidi/Module.java[44-54]
- java/src/org/openqa/selenium/bidi/BiDi.java[98-126]

### Suggested direction
- Add `Handle.subscribe(String browsingContextId, Event<X>, Consumer<X>)` and `Handle.subscribe(Set<String> browsingContextIds, Event<X>, Consumer<X>)` delegating to the matching `BiDi.addListener` overloads.
- Expose corresponding `protected final` overloads on `Module` so generated modules can use them without touching `Handle` directly.

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


Results up to commit 1360947


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


Action required
1. No tests for Handle API 📘 Rule violation ▣ Testability
Description
This PR introduces new BiDi transport/public-surface behavior (Handle, Module, and
BiDi#asHandle()), but the change set contains no corresponding tests exercising these new code
paths. This increases the risk of regressions and makes the new API contract undocumented by
automation.
Code

java/src/org/openqa/selenium/bidi/Module.java[R36-54]

+  protected Module(WebDriver driver) {
+    Require.nonNull("WebDriver", driver);
+    if (!(driver instanceof HasBiDi)) {
+      throw new BiDiException("WebDriver does not support BiDi protocol");
+    }
+    this.handle = ((HasBiDi) driver).getHandle();
+  }
+
+  protected final <X> X send(Command<X> command) {
+    return handle.send(command);
+  }
+
+  protected final <X> String subscribe(Event<X> event, Consumer<X> handler) {
+    return handle.subscribe(event, handler);
+  }
+
+  protected final void unsubscribe(String subscriptionId) {
+    handle.unsubscribe(subscriptionId);
+  }
Evidence
PR adds new transport-layer API entry points (BiDi#asHandle() and the new Module base class
delegating through Handle), but no tests were added/updated in this change set to validate the new
behavior as required.

Rule 389273: Require tests for all new functionality and bug fixes
java/src/org/openqa/selenium/bidi/BiDi.java[171-172]
java/src/org/openqa/selenium/bidi/Module.java[36-54]

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 BiDi transport-layer functionality was added (`Handle`, `Module`, and driver `getHandle()` plumbing) without any accompanying automated tests.

## Issue Context
This PR adds new public-facing behavior in the Java binding (e.g., `BiDi#asHandle()` and `Module` delegating through `Handle`). Without tests, future refactors can silently break module command sending/subscription behavior.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[171-172]
- java/src/org/openqa/selenium/bidi/Module.java[36-54]
- java/src/org/openqa/selenium/remote/RemoteWebDriver.java[480-494]

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



Informational
2. Handle lacks @beta annotation ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Handle is introduced as a public API type and is exposed via HasBiDi#getHandle(), but unlike the
surrounding BiDi surface it is not annotated @Beta, which can mis-signal it as a stable/committed
API despite being intended as an internal-ish transport handle.
Code

java/src/org/openqa/selenium/bidi/Handle.java[R22-31]

+/**
+ * An opaque handle to the active BiDi connection, used by {@link Module} subclasses to send
+ * commands and subscribe to events.
+ *
+ * <p>Constructor and all methods are package-private: external callers that hold a {@code Handle}
+ * reference cannot invoke it, only pass it to module constructors inside this package. Create an
+ * instance via {@link BiDi#asHandle()}.
+ */
+public class Handle {
+
Evidence
The new Handle type is public and is part of the public API surface via HasBiDi#getHandle(), but it
is not marked @Beta while adjacent BiDi entry points are @Beta, creating inconsistent API stability
signaling.

java/src/org/openqa/selenium/bidi/Handle.java[22-31]
java/src/org/openqa/selenium/bidi/HasBiDi.java[23-43]
java/src/org/openqa/selenium/bidi/Module.java[25-34]
java/src/org/openqa/selenium/bidi/BiDi.java[29-35]

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

### Issue description
`org.openqa.selenium.bidi.Handle` is a newly added **public** type and is returned from the public `HasBiDi#getHandle()` API, but it is not annotated with `@Beta` even though the rest of the BiDi-facing surface in this area is.

This is primarily an API-signaling/maintenance problem: it can encourage downstream code to treat `Handle` as stable even if Selenium intends to keep it flexible while generated BiDi modules are evolving.

### Issue Context
`HasBiDi` is annotated `@Beta` and now exposes `Handle` directly. `Module` and `BiDi` are also `@Beta`, but `Handle` itself is not.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Handle.java[18-31]

### Suggested fix
Add `import org.openqa.selenium.Beta;` and annotate the class with `@Beta` (or otherwise make the intended stability explicit).

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


Qodo Logo

Comment thread java/src/org/openqa/selenium/bidi/HasBiDi.java
Comment thread java/src/org/openqa/selenium/remote/RemoteWebDriver.java
Comment thread java/src/org/openqa/selenium/bidi/Handle.java Outdated
Comment thread java/src/org/openqa/selenium/bidi/Handle.java
@pujagani pujagani marked this pull request as draft July 10, 2026 04:26
@pujagani pujagani force-pushed the add-transport-layer branch from fbef9ae to 6d114f7 Compare July 10, 2026 12:21
@pujagani pujagani marked this pull request as ready for review July 10, 2026 12:44
Comment thread java/src/org/openqa/selenium/bidi/Module.java
Comment thread java/src/org/openqa/selenium/bidi/Handle.java
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@qodo-code-review

Copy link
Copy Markdown
Contributor

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

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

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants