Skip to content

[dotnet] [bidi] Hide url required for websocket only #17208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
nvborisenko wants to merge 3 commits into
base: trunk
Choose a base branch
from
Draft

[dotnet] [bidi] Hide url required for websocket only #17208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c2e6a77
[dotnet] [bidi] Hide url required for websocket only
nvborisenko Mar 11, 2026
5a2cefc
Pass user defined
nvborisenko Mar 11, 2026
8ccaea6
Fix order
nvborisenko Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions dotnet/src/webdriver/BiDi/BiDi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,14 @@ private BiDi() { }

public Emulation.IEmulationModule Emulation => AsModule<Emulation.EmulationModule>();

public static async Task<IBiDi> ConnectAsync(string url, Action<BiDiOptionsBuilder>? configure = null, CancellationToken cancellationToken = default)
public static async Task<IBiDi> ConnectAsync(Action<BiDiOptionsBuilder> configure, CancellationToken cancellationToken = default)
{
if (configure is null) throw new ArgumentNullException(nameof(configure));

BiDiOptionsBuilder builder = new();
configure?.Invoke(builder);
configure(builder);

var transport = await builder.TransportFactory(new Uri(url), cancellationToken).ConfigureAwait(false);
var transport = await builder.TransportFactory(cancellationToken).ConfigureAwait(false);
Comment on lines +56 to +63
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Mar 11, 2026

Choose a reason for hiding this comment

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

Action required

1. connectasync signature changed 📘 Rule violation ✓ Correctness

BiDi.ConnectAsync removed the url parameter and made configure non-optional, forcing consumer
code changes beyond a version bump. BiDiOptionsBuilder.UseWebSocket also now requires a url,
breaking existing callers and violating API/ABI compatibility expectations.
Agent Prompt 
## Issue description
Public API signatures were changed in a breaking way (`BiDi.ConnectAsync` and `BiDiOptionsBuilder.UseWebSocket`), requiring downstream user code changes.

## Issue Context
Compliance requires that users can upgrade by changing only the package version. If the new API is desired, keep it but add compatibility overloads that delegate to it.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/BiDi.cs[56-63]
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[38-46]

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

Comment on lines +56 to +63
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

This change removes/changes the public BiDi.ConnectAsync(string url, ...) API, which violates the repo’s compatibility invariant (AGENTS.md:11-14) and deprecation guidance (AGENTS.md:65-67). To avoid breaking existing consumers, keep the existing overload (and behavior) and introduce the new overload alongside it, marking the old one [Obsolete] with a migration message instead of deleting/changing its signature.

Copilot uses AI. Check for mistakes.

BiDi bidi = new();

Expand Down
19 changes: 9 additions & 10 deletions dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,21 @@ namespace OpenQA.Selenium.BiDi;
/// </summary>
public sealed class BiDiOptionsBuilder
{
internal Func<Uri, CancellationToken, Task<ITransport>> TransportFactory { get; private set; }
= (uri, ct) => WebSocketTransport.ConnectAsync(uri, null, ct);
private Func<CancellationToken, Task<ITransport>>? _transportFactory;

/// <summary>
/// Configures the BiDi connection to use a WebSocket transport.
/// Configures the BiDi connection to use a WebSocket transport with the specified URL.
/// </summary>
/// <remarks>
/// WebSocket is the default transport; calling this method is only necessary
/// when you need to customize the underlying <see cref="ClientWebSocketOptions"/>
/// (e.g., to set headers, proxy, or certificates).
/// </remarks>
/// <param name="url">The WebSocket URL to connect to.</param>
/// <param name="configure">An optional action to configure the <see cref="ClientWebSocketOptions"/> before connecting.</param>
/// <returns>The current <see cref="BiDiOptionsBuilder"/> instance for chaining.</returns>
public BiDiOptionsBuilder UseWebSocket(Action<ClientWebSocketOptions>? configure = null)
public BiDiOptionsBuilder UseWebSocket(string url, Action<ClientWebSocketOptions>? configure = null)
{
TransportFactory = (uri, ct) => WebSocketTransport.ConnectAsync(uri, configure, ct);
var uri = new Uri(url);
_transportFactory = ct => WebSocketTransport.ConnectAsync(uri, configure, ct);
return this;
}
Comment on lines 32 to 43
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

UseWebSocket changed from an optional-configure overload to requiring a URL string, which is a breaking change for public API consumers and conflicts with the repo’s compatibility/deprecation policy (AGENTS.md:11-14, 65-67). Consider keeping the old overload (e.g., for configuring ClientWebSocketOptions) and adding the new URL-taking overload, marking the old one [Obsolete] if you want to steer usage.

Copilot uses AI. Check for mistakes.

internal Func<CancellationToken, Task<ITransport>> TransportFactory
=> _transportFactory ?? throw new BiDiException("Transport is not configured. Call UseWebSocket(url) on BiDiOptionsBuilder.");
}
6 changes: 5 additions & 1 deletion dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ public static async Task<IBiDi> AsBiDiAsync(this IWebDriver webDriver, Action<Bi

if (webSocketUrl is null) throw new BiDiException("The driver is not compatible with bidirectional protocol or \"webSocketUrl\" not enabled in driver options.");

var bidi = await BiDi.ConnectAsync(webSocketUrl, configure, cancellationToken).ConfigureAwait(false);
var bidi = await BiDi.ConnectAsync(options =>
{
configure?.Invoke(options);
options.UseWebSocket(webSocketUrl);
Comment on lines +39 to +40
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

configure?.Invoke(options) runs before options.UseWebSocket(webSocketUrl), but UseWebSocket(...) overwrites the transport factory. If a caller uses configure to customize the WebSocket transport (headers/proxy/certs), their configuration will be discarded. Reorder/reshape this so caller configuration is preserved (or merge WebSocket option configuration rather than overwriting it).

Suggested change
configure?.Invoke(options);
options.UseWebSocket(webSocketUrl);
options.UseWebSocket(webSocketUrl);
configure?.Invoke(options);

Copilot uses AI. Check for mistakes.
Comment on lines +37 to +40
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

webSocketUrl is declared as string? and then captured into a lambda; nullable flow analysis generally won’t consider it non-null inside the delegate, which can produce nullable warnings (and potentially fail builds if warnings are treated as errors). Assign it to a non-null local (or use the null-forgiving operator after the null-check) before passing it into the lambda.

Suggested change
var bidi = await BiDi.ConnectAsync(options =>
{
configure?.Invoke(options);
options.UseWebSocket(webSocketUrl);
var nonNullWebSocketUrl = webSocketUrl;
var bidi = await BiDi.ConnectAsync(options =>
{
configure?.Invoke(options);
options.UseWebSocket(nonNullWebSocketUrl);

Copilot uses AI. Check for mistakes.
}, cancellationToken).ConfigureAwait(false);
Comment on lines +37 to +41
Copy link
Copy Markdown
Contributor

@qodo-code-review qodo-code-review bot Mar 11, 2026

Choose a reason for hiding this comment

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

Action required

2. Transport config overwritten 🐞 Bug ✓ Correctness

WebDriverExtensions.AsBiDiAsync() always calls UseWebSocket(webSocketUrl) after running the
user-supplied configure callback, overwriting any transport configuration performed inside
configure. This makes caller transport/WebSocket configuration silently ineffective because the last
UseWebSocket call replaces the builder’s transport factory.
Agent Prompt 
### Issue description
`WebDriverExtensions.AsBiDiAsync()` currently runs the caller-provided `configure` callback and then unconditionally calls `options.UseWebSocket(webSocketUrl)`, which overwrites any transport configuration performed inside `configure`.

### Issue Context
`BiDiOptionsBuilder.UseWebSocket(...)` assigns `_transportFactory`, and only the last assignment is used. Therefore the unconditional `UseWebSocket(webSocketUrl)` call in `AsBiDiAsync` discards prior transport configuration.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs[37-41]
- dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs[38-46]

### Suggested implementation direction
- Option A (non-breaking): after `configure?.Invoke(options)`, only call `UseWebSocket(webSocketUrl)` if transport was not configured.
  - This likely requires adding an `internal bool IsTransportConfigured => _transportFactory != null;` to `BiDiOptionsBuilder`.
- Option B: call `options.UseWebSocket(webSocketUrl)` first, then `configure?.Invoke(options)` to allow callers to override the transport (if they choose to).
- If you need to preserve the pre-refactor ergonomics (customizing `ClientWebSocketOptions` without needing to supply the URL), add a new overload of `AsBiDiAsync` that accepts `Action<ClientWebSocketOptions>?` and uses it in `UseWebSocket(webSocketUrl, configureWs)`.

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


return bidi;
}
Expand Down
Loading