[dotnet] [bidi] Refactor BiDi module initialization to pass BiDi explicitly by nvborisenko · Pull Request #16983

[dotnet] [bidi] Refactor BiDi module initialization to pass BiDi explicitly by nvborisenko · Pull Request #16983 · SeleniumHQ/selenium

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

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