Home Original page

[dotnet] [bidi] Simplify context aware command options by nvborisenko · Pull Request #16954 · SeleniumHQ/selenium

Conversation

@nvborisenko

@nvborisenko nvborisenko commented

Jan 19, 2026

•

edited by qodo-code-review bot

Loading

User description

💥 What does this PR do?

  • Rename BrowsingContext*Options to Context*Options
  • Propagate Timeout command property
  • Align context options with its parent (class instead of record)

🔄 Types of changes

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

PR Type

Enhancement, Bug fix

Description

  • Rename BrowsingContext*Options to Context*Options for consistency

  • Convert context options from records to classes inheriting CommandOptions

  • Propagate Timeout property from context options to command options

  • Align context-aware command options with parent class structure

Diagram Walkthrough

flowchart LR
  A["BrowsingContext*Options<br/>records"] -- "rename to" --> B["Context*Options<br/>classes"]
  B -- "inherit from" --> C["CommandOptions"]
  C -- "provides" --> D["Timeout property"]
  B -- "propagate to" --> E["Command Options"]
Loading

File Walkthrough
Relevant files
Refactoring
BrowsingContext.cs
Update GetTreeAsync method signature                                         

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs

  • Update method signature to use ContextGetTreeOptions instead of
    BrowsingContextGetTreeOptions
 +1/-1     
BrowsingContextNetworkModule.cs
Update network module method signatures                                   

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs

  • Update AddDataCollectorAsync method to use
    ContextAddDataCollectorOptions
  • Update SetCacheBehaviorAsync method to use
    ContextSetCacheBehaviorOptions
 +2/-2     
BrowsingContextScriptModule.cs
Update script module method signature                                       

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextScriptModule.cs

  • Update AddPreloadScriptAsync method to use
    ContextAddPreloadScriptOptions
 +1/-1     
GetTreeCommand.cs
Refactor GetTreeOptions and ContextGetTreeOptions               

dotnet/src/webdriver/BiDi/BrowsingContext/GetTreeCommand.cs

  • Convert GetTreeOptions to use primary constructor
  • Rename BrowsingContextGetTreeOptions to ContextGetTreeOptions and
    change from record to class inheriting CommandOptions
  • Add Timeout property propagation from context options
 +5/-5     
AddDataCollectorCommand.cs
Refactor AddDataCollectorOptions and ContextAddDataCollectorOptions

dotnet/src/webdriver/BiDi/Network/AddDataCollectorCommand.cs

  • Add sealed modifier to AddDataCollectorOptions
  • Rename BrowsingContextAddDataCollectorOptions to
    ContextAddDataCollectorOptions and change from class to sealed class
    inheriting CommandOptions
  • Add Timeout property propagation from context options
 +4/-3     
AddInterceptCommand.cs
Refactor AddInterceptOptions and ContextAddInterceptOptions

dotnet/src/webdriver/BiDi/Network/AddInterceptCommand.cs

  • Add sealed modifier to AddInterceptOptions
  • Rename BrowsingContextAddInterceptOptions to
    ContextAddInterceptOptions and change from record to class inheriting
    CommandOptions
  • Add Timeout property propagation from context options
 +4/-3     
SetCacheBehaviorCommand.cs
Refactor SetCacheBehaviorOptions and ContextSetCacheBehaviorOptions

dotnet/src/webdriver/BiDi/Network/SetCacheBehaviorCommand.cs

  • Rename BrowsingContextSetCacheBehaviorOptions to
    ContextSetCacheBehaviorOptions and change from record to sealed class
    inheriting CommandOptions
  • Add Timeout property propagation from context options
 +3/-3     
AddPreloadScriptCommand.cs
Refactor AddPreloadScriptOptions and ContextAddPreloadScriptOptions

dotnet/src/webdriver/BiDi/Script/AddPreloadScriptCommand.cs

  • Convert AddPreloadScriptOptions to use primary constructor
  • Rename BrowsingContextAddPreloadScriptOptions to
    ContextAddPreloadScriptOptions and change from record to sealed class
    inheriting CommandOptions
  • Add Timeout property propagation from context options
 +5/-5     

@qodo-code-review

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

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Add fast argument validation

Validate dataTypes for null and ensure maxEncodedDataSize is non-negative before
calling into the network module to fail fast with clear exceptions.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs [83-91]

 public Task<AddDataCollectorResult> AddDataCollectorAsync(IEnumerable<DataType> dataTypes, int maxEncodedDataSize, ContextAddDataCollectorOptions? options = null)
 {
+    ArgumentNullException.ThrowIfNull(dataTypes);
+    ArgumentOutOfRangeException.ThrowIfNegative(maxEncodedDataSize);
+
     AddDataCollectorOptions addDataCollectorOptions = new(options)
     {
         Contexts = [context]
     };
 
     return networkModule.AddDataCollectorAsync(dataTypes, maxEncodedDataSize, addDataCollectorOptions);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation and null/availability guards at integration boundaries by checking presence/type/range before use.

Low
Defensively copy caller collections

Copy caller-provided enumerables (e.g., via ToArray()) when assigning to option
properties so later mutations of the original collection don’t affect the
command options.

dotnet/src/webdriver/BiDi/Network/AddDataCollectorCommand.cs [33-38]

 internal AddDataCollectorOptions(ContextAddDataCollectorOptions? options) : this()
 {
     CollectorType = options?.CollectorType;
-    UserContexts = options?.UserContexts;
+    UserContexts = options?.UserContexts is null ? null : options.UserContexts.ToArray();
     Timeout = options?.Timeout;
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - When storing collection-like state from callers, make defensive copies to avoid leaking internal mutable state and unexpected external mutation.

Low
General
Make base options class abstract

Mark the ContextAddInterceptOptions base class as abstract to prevent direct
instantiation and clarify its role. This ensures only its more specific derived
classes are used.

dotnet/src/webdriver/BiDi/Network/AddInterceptCommand.cs [46-49]

-public class ContextAddInterceptOptions : CommandOptions
+public abstract class ContextAddInterceptOptions : CommandOptions
 {
     public IEnumerable<UrlPattern>? UrlPatterns { get; set; }
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that ContextAddInterceptOptions acts as a base class and making it abstract is a good design practice to enforce this role, improving code robustness.

Low
✅ Use standard braces for class definition
Suggestion Impact:The class ContextSetCacheBehaviorOptions was changed from an empty semicolon-terminated declaration to a brace-based class body, aligning with the suggested C# style. The commit also added an internal helper method inside the class, so it was not a purely stylistic change.

code diff:

-public sealed class ContextSetCacheBehaviorOptions : CommandOptions;
+public sealed class ContextSetCacheBehaviorOptions : CommandOptions
+{
+    internal static SetCacheBehaviorOptions WithContext(ContextSetCacheBehaviorOptions? options, BrowsingContext.BrowsingContext context) => new()
+    {
+        Contexts = [context],
+        Timeout = options?.Timeout
+    };
+}

Replace the semicolon with curly braces {} for the empty class definition of
ContextSetCacheBehaviorOptions to improve readability and align with
conventional C# style.

dotnet/src/webdriver/BiDi/Network/SetCacheBehaviorCommand.cs [41]

-public sealed class ContextSetCacheBehaviorOptions : CommandOptions;
+public sealed class ContextSetCacheBehaviorOptions : CommandOptions
+{
+}

[Suggestion processed]

Suggestion importance[1-10]: 3

__

Why: This is a valid stylistic suggestion that improves code consistency and readability by adhering to common C# conventions for empty class definitions.

Low
  • Update

RenderMichael

RenderMichael

Choose a reason for hiding this comment

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

These changes look good. I left one comment, not blocking the PR, about taking these changes even further.

This was referenced

Feb 20, 2026

This was referenced

Feb 22, 2026

Labels