[dotnet] [bidi] Hide url required for websocket only#17208
Draft
nvborisenko wants to merge 3 commits intoSeleniumHQ:trunkfrom
Draft
[dotnet] [bidi] Hide url required for websocket only#17208nvborisenko wants to merge 3 commits intoSeleniumHQ:trunkfrom
nvborisenko wants to merge 3 commits intoSeleniumHQ:trunkfrom
Conversation
Contributor
Review Summary by QodoRefactor BiDi connection to require explicit WebSocket URL configuration
WalkthroughsDescription• Refactors BiDi connection setup to require explicit URL configuration • Removes implicit WebSocket URL passing from ConnectAsync method • Makes UseWebSocket method accept URL parameter directly • Enforces transport configuration through builder pattern File Changes1. dotnet/src/webdriver/BiDi/BiDi.cs
|
Contributor
Code Review by Qodo
1. ConnectAsync signature changed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+56
to
+63
| 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); |
Contributor
There was a problem hiding this comment.
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
+37
to
+41
| var bidi = await BiDi.ConnectAsync(options => | ||
| { | ||
| configure?.Invoke(options); | ||
| options.UseWebSocket(webSocketUrl); | ||
| }, cancellationToken).ConfigureAwait(false); |
Contributor
There was a problem hiding this comment.
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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the .NET BiDi connection setup to require explicit transport configuration via BiDiOptionsBuilder.UseWebSocket(url) rather than implicitly passing a WebSocket URL into BiDi.ConnectAsync.
Changes:
- Updated
WebDriverExtensions.AsBiDiAsyncto configure BiDi connection by callingUseWebSocket(webSocketUrl)inside the options callback. - Refactored
BiDiOptionsBuilderto store a configured transport factory and throw if transport is not configured. - Changed
BiDi.ConnectAsyncto require an options configuration callback and removed theurlparameter overload.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
dotnet/src/webdriver/BiDi/WebDriver.Extensions.cs |
Switches BiDi connection creation to configure transport via builder callback. |
dotnet/src/webdriver/BiDi/BiDiOptionsBuilder.cs |
Makes WebSocket URL explicit in UseWebSocket and enforces transport configuration. |
dotnet/src/webdriver/BiDi/BiDi.cs |
Changes ConnectAsync signature to rely solely on builder configuration for transport. |
Comment on lines
+39
to
+40
| configure?.Invoke(options); | ||
| options.UseWebSocket(webSocketUrl); |
There was a problem hiding this comment.
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); |
Comment on lines
+37
to
+40
| var bidi = await BiDi.ConnectAsync(options => | ||
| { | ||
| configure?.Invoke(options); | ||
| options.UseWebSocket(webSocketUrl); |
There was a problem hiding this comment.
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); |
Comment on lines
+56
to
+63
| 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); |
There was a problem hiding this comment.
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.
Comment on lines
32
to
43
| /// <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; | ||
| } |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors the BiDi connection setup in the .NET WebDriver codebase to improve clarity and enforce explicit transport configuration. The changes remove the implicit passing of the WebSocket URL, requiring users to specify the URL via the
UseWebSocketmethod onBiDiOptionsBuilder. This makes the transport configuration more robust and less error-prone.🔄 Types of changes