Home Original page

[dotnet] [bidi] Fix context aware event handlers by nvborisenko · Pull Request #16787 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Refactor duplicated logic into helpers

To reduce code duplication, create two private helper methods—one for
synchronous Action handlers and one for asynchronous Func<T, Task> handlers—to
encapsulate the repeated context-filtering logic.

Examples:

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [136-142] 
        return BiDi.BrowsingContext.OnNavigationStartedAsync(async e =>
        {
            if (e.Context == this)
            {
                await handler(e).ConfigureAwait(false);
            }
        }, options.WithContext(this));
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [147-153] 
        return BiDi.BrowsingContext.OnNavigationStartedAsync(e =>
        {
            if (e.Context == this)
            {
                handler(e);
            }
        }, options.WithContext(this));

Solution Walkthrough:

Before:

public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ...)
{
    return BiDi.BrowsingContext.OnNavigationStartedAsync(async e =>
    {
        if (e.Context == this)
        {
            await handler(e).ConfigureAwait(false);
        }
    }, ...);
}

public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...)
{
    return BiDi.BrowsingContext.OnNavigationStartedAsync(e =>
    {
        if (e.Context == this)
        {
            handler(e);
        }
    }, ...);
}
// ... this pattern is repeated for many other event handlers.

After:

private Action<T> WrapHandler<T>(Action<T> handler) where T : IHasContext
{
    return e => { if (e.Context == this) handler(e); };
}

private Func<T, Task> WrapHandler<T>(Func<T, Task> handler) where T : IHasContext
{
    return async e => { if (e.Context == this) await handler(e).ConfigureAwait(false); };
}

public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ...)
{
    return BiDi.BrowsingContext.OnNavigationStartedAsync(WrapHandler(handler), ...);
}

public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...)
{
    return BiDi.BrowsingContext.OnNavigationStartedAsync(WrapHandler(handler), ...);
}
// ... all other handlers are simplified similarly.
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies significant code duplication across all modified methods and proposes an excellent refactoring that would centralize the logic, greatly improving code quality and maintainability.

High
Possible issue
Add exception handling to handlers

Add try-catch blocks around the invocation of user-provided handler delegates in
all event subscription methods. This prevents unhandled exceptions within user
code from crashing the application.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [134-154]

 public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null)
 {
     return BiDi.BrowsingContext.OnNavigationStartedAsync(async e =>
     {
         if (e.Context == this)
         {
-            await handler(e).ConfigureAwait(false);
+            try
+            {
+                await handler(e).ConfigureAwait(false);
+            }
+            catch
+            {
+                // Ignore exceptions from user-provided handlers.
+            }
         }
     }, options.WithContext(this));
 }
 
 public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ContextSubscriptionOptions? options = null)
 {
     return BiDi.BrowsingContext.OnNavigationStartedAsync(e =>
     {
         if (e.Context == this)
         {
-            handler(e);
+            try
+            {
+                handler(e);
+            }
+            catch
+            {
+                // Ignore exceptions from user-provided handlers.
+            }
         }
     }, options.WithContext(this));
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly identifies that unhandled exceptions in user-provided event handlers can crash the application. Wrapping the handler calls in a try-catch block is a valid strategy to improve the library's robustness.

Medium
validate handler parameter

Add a null check for the handler parameter at the beginning of the method and
throw an ArgumentNullException if it is null.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [134-135]

 public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null)
 {
+    if (handler is null)
+    {
+        throw new ArgumentNullException(nameof(handler));
+    }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This is a good practice for public APIs to validate arguments and fail early. Adding a null check for the handler parameter improves code robustness and provides clearer error messages.

Low
Learned
best practice
Extract shared context-filter helper

The PR repeats the same context-filtering wrapper pattern across many event
methods; extract a small private helper to centralize the filter + invocation
and call it from each handler to reduce duplication and future drift.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [134-154]

+private bool IsForThisContext(BrowsingContext? context) => Equals(context, this);
+
+private Action<T> FilterForThisContext<T>(Action<T> handler, Func<T, BrowsingContext?> getContext) =>
+    e => { if (IsForThisContext(getContext(e))) handler(e); };
+
+private Func<T, Task> FilterForThisContext<T>(Func<T, Task> handler, Func<T, BrowsingContext?> getContext) =>
+    async e => { if (IsForThisContext(getContext(e))) await handler(e).ConfigureAwait(false); };
+
 public Task<Subscription> OnNavigationStartedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null)
 {
-    return BiDi.BrowsingContext.OnNavigationStartedAsync(async e =>
-    {
-        if (e.Context == this)
-        {
-            await handler(e).ConfigureAwait(false);
-        }
-    }, options.WithContext(this));
+    return BiDi.BrowsingContext.OnNavigationStartedAsync(
+        FilterForThisContext(handler, e => e.Context),
+        options.WithContext(this));
 }
 
 public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ContextSubscriptionOptions? options = null)
 {
-    return BiDi.BrowsingContext.OnNavigationStartedAsync(e =>
-    {
-        if (e.Context == this)
-        {
-            handler(e);
-        }
-    }, options.WithContext(this));
+    return BiDi.BrowsingContext.OnNavigationStartedAsync(
+        FilterForThisContext(handler, e => e.Context),
+        options.WithContext(this));
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Replace ad-hoc duplication with shared helpers/utilities to reduce repetition and centralize logic.

Low
  • Update