Skip to content

[dotnet] [bidi] Refactor BiDi module initialization to pass BiDi explicitly#16983

Merged
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-module-holder
Jan 23, 2026
Merged

[dotnet] [bidi] Refactor BiDi module initialization to pass BiDi explicitly#16983
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-module-holder

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 23, 2026

The primary focus is to consistently pass the required dependencies explicitly. It allows keeping a reference to BiDi object only when required.

💥 What does this PR do?

Now modules should implement Initialize(BiDi, JsonSerializerOptions).

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Breaking change (fix or feature that would cause existing functionality to change)

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 23, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 23, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Refactor interception handlers for better encapsulation

Refactor the Interception record's On...Async methods to accept handlers for
specific Intercepted... event types, improving encapsulation by handling object
creation internally.

dotnet/src/webdriver/BiDi/Network/NetworkModule.HighLevel.cs [131-176]

 public sealed record Interception(NetworkModule Network, Intercept Intercept) : IAsyncDisposable
 {
     IList<Subscription> OnBeforeRequestSentSubscriptions { get; } = [];
     IList<Subscription> OnResponseStartedSubscriptions { get; } = [];
     IList<Subscription> OnAuthRequiredSubscriptions { get; } = [];
 
     public async Task RemoveAsync()
     {
         await Network.RemoveInterceptAsync(Intercept).ConfigureAwait(false);
 
         foreach (var subscription in OnBeforeRequestSentSubscriptions)
         {
             await subscription.UnsubscribeAsync().ConfigureAwait(false);
         }
 
         foreach (var subscription in OnResponseStartedSubscriptions)
         {
             await subscription.UnsubscribeAsync().ConfigureAwait(false);
         }
 
         foreach (var subscription in OnAuthRequiredSubscriptions)
         {
             await subscription.UnsubscribeAsync().ConfigureAwait(false);
         }
     }
 
-    public async Task OnBeforeRequestSentAsync(Func<BeforeRequestSentEventArgs, Task> handler, SubscriptionOptions? options = null)
+    public async Task OnBeforeRequestSentAsync(Func<InterceptedRequest, Task> handler, SubscriptionOptions? options = null)
     {
-        var subscription = await Network.OnBeforeRequestSentAsync(async args => await Filter(args, handler), options).ConfigureAwait(false);
+        var subscription = await Network.OnBeforeRequestSentAsync(async args =>
+        {
+            if (await Filter(args, _ => Task.CompletedTask).ConfigureAwait(false))
+            {
+                var interceptedRequest = new InterceptedRequest(Network, args.Context, args.IsBlocked, args.Navigation, args.RedirectCount, args.Request, args.Timestamp, args.Initiator, args.Intercepts);
+                await handler(interceptedRequest);
+            }
+        }, options).ConfigureAwait(false);
 
         OnBeforeRequestSentSubscriptions.Add(subscription);
     }
 
-    public async Task OnResponseStartedAsync(Func<ResponseStartedEventArgs, Task> handler, SubscriptionOptions? options = null)
+    public async Task OnResponseStartedAsync(Func<InterceptedResponse, Task> handler, SubscriptionOptions? options = null)
     {
-        var subscription = await Network.OnResponseStartedAsync(async args => await Filter(args, handler), options).ConfigureAwait(false);
+        var subscription = await Network.OnResponseStartedAsync(async args =>
+        {
+            if (await Filter(args, _ => Task.CompletedTask).ConfigureAwait(false))
+            {
+                var interceptedResponse = new InterceptedResponse(Network, args.Context, args.IsBlocked, args.Navigation, args.RedirectCount, args.Request, args.Timestamp, args.Response, args.Intercepts);
+                await handler(interceptedResponse);
+            }
+        }, options).ConfigureAwait(false);
 
         OnResponseStartedSubscriptions.Add(subscription);
     }
 
-    public async Task OnAuthRequiredAsync(Func<AuthRequiredEventArgs, Task> handler, SubscriptionOptions? options = null)
+    public async Task OnAuthRequiredAsync(Func<InterceptedAuth, Task> handler, SubscriptionOptions? options = null)
     {
-        var subscription = await Network.OnAuthRequiredAsync(async args => await Filter(args, handler), options).ConfigureAwait(false);
+        var subscription = await Network.OnAuthRequiredAsync(async args =>
+        {
+            if (await Filter(args, _ => Task.CompletedTask).ConfigureAwait(false))
+            {
+                var interceptedAuth = new InterceptedAuth(Network, args.Context, args.IsBlocked, args.Navigation, args.RedirectCount, args.Request, args.Timestamp, args.Response, args.Intercepts);
+                await handler(interceptedAuth);
+            }
+        }, options).ConfigureAwait(false);
 
         OnAuthRequiredSubscriptions.Add(subscription);
     }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies an opportunity to improve encapsulation and continue the decoupling effort started in the PR, making the API more robust and easier to use.

Medium
Learned
best practice
Make removal idempotent and safe

Make RemoveAsync idempotent and ensure subscriptions are always attempted for
cleanup (even if RemoveInterceptAsync or an UnsubscribeAsync throws).

dotnet/src/webdriver/BiDi/Network/NetworkModule.HighLevel.cs [137-155]

+private int _removed;
+
 public async Task RemoveAsync()
 {
-    await Network.RemoveInterceptAsync(Intercept).ConfigureAwait(false);
-
-    foreach (var subscription in OnBeforeRequestSentSubscriptions)
+    if (Interlocked.Exchange(ref _removed, 1) == 1)
     {
-        await subscription.UnsubscribeAsync().ConfigureAwait(false);
+        return;
     }
 
-    foreach (var subscription in OnResponseStartedSubscriptions)
+    try
     {
-        await subscription.UnsubscribeAsync().ConfigureAwait(false);
+        await Network.RemoveInterceptAsync(Intercept).ConfigureAwait(false);
     }
+    finally
+    {
+        foreach (var subscription in OnBeforeRequestSentSubscriptions)
+        {
+            await subscription.UnsubscribeAsync().ConfigureAwait(false);
+        }
 
-    foreach (var subscription in OnAuthRequiredSubscriptions)
-    {
-        await subscription.UnsubscribeAsync().ConfigureAwait(false);
+        foreach (var subscription in OnResponseStartedSubscriptions)
+        {
+            await subscription.UnsubscribeAsync().ConfigureAwait(false);
+        }
+
+        foreach (var subscription in OnAuthRequiredSubscriptions)
+        {
+            await subscription.UnsubscribeAsync().ConfigureAwait(false);
+        }
     }
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Make lifecycle/disposal code robust and idempotent to avoid double-dispose and partial cleanup on failures.

Low
Add null guards for inputs

Validate bidi, broker, and jsonSerializerOptions for null at the entry point to
prevent downstream NullReferenceExceptions during module initialization.

dotnet/src/webdriver/BiDi/Module.cs [30-41]

 internal static TModule Create<TModule>(BiDi bidi, Broker broker, JsonSerializerOptions jsonSerializerOptions)
     where TModule : Module, new()
 {
+    ArgumentNullException.ThrowIfNull(bidi);
+    ArgumentNullException.ThrowIfNull(broker);
+    ArgumentNullException.ThrowIfNull(jsonSerializerOptions);
+
     TModule module = new()
     {
         Broker = broker
     };
 
     module.Initialize(bidi, jsonSerializerOptions);
 
     return module;
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Add explicit validation and null/availability guards at integration boundaries to fail fast with clear errors.

Low
  • Update

@nvborisenko nvborisenko merged commit 574dd1a into SeleniumHQ:trunk Jan 23, 2026
15 checks passed
@nvborisenko nvborisenko deleted the bidi-module-holder branch January 23, 2026 00:52
This was referenced Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants