Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Dec 6, 2025

User description

Now modules are not required to create new instance based on provided options. It is safe to modify original options.

💥 What does this PR do?

Simplify new modules development.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Refactored JSON options creation to provide fresh instances per module

  • Moved JsonSerializerOptions initialization to static factory method

  • Simplified module Initialize methods by directly modifying passed options

  • Eliminates need for modules to create new JsonSerializerOptions copies


Diagram Walkthrough

flowchart LR
  BiDi["BiDi class"]
  CreateMethod["CreateDefaultJsonOptions<br/>static method"]
  AsModule["AsModule method"]
  Modules["Module.Initialize<br/>methods"]
  
  BiDi -- "removed field<br/>_jsonOptions" --> CreateMethod
  AsModule -- "calls" --> CreateMethod
  CreateMethod -- "provides new<br/>options" --> AsModule
  AsModule -- "passes to" --> Modules
  Modules -- "directly modifies<br/>converters" --> Modules
Loading

File Walkthrough

Relevant files
Refactoring
12 files
BiDi.cs
Extract JSON options creation to factory method                   
+15/-15 
BrowserModule.cs
Simplify Initialize to modify passed options directly       
+3/-9     
BrowsingContextModule.cs
Simplify Initialize to modify passed options directly       
+6/-12   
EmulationModule.cs
Simplify Initialize to modify passed options directly       
+4/-10   
InputModule.cs
Simplify Initialize to modify passed options directly       
+3/-9     
LogModule.cs
Simplify Initialize to modify passed options directly       
+5/-11   
NetworkModule.cs
Simplify Initialize to modify passed options directly       
+6/-12   
PermissionsModule.cs
Simplify Initialize to modify passed options directly       
+3/-9     
ScriptModule.cs
Simplify Initialize to modify passed options directly       
+7/-13   
SessionModule.cs
Simplify Initialize to modify passed options directly       
+3/-9     
StorageModule.cs
Simplify Initialize to modify passed options directly       
+3/-9     
WebExtensionModule.cs
Simplify Initialize to modify passed options directly       
+2/-8     

@qodo-code-review
Copy link
Contributor

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: 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: 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: Comprehensive Audit Trails

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

Status:
No auditing: The new code focuses on JSON options creation and module initialization without adding or
modifying any logging of critical actions, which may be acceptable if handled elsewhere
but is not visible in this diff.

Referred Code
    return (T)_modules.GetOrAdd(typeof(T), _ => Module.Create<T>(this, Broker, CreateDefaultJsonOptions()));
}

private static JsonSerializerOptions CreateDefaultJsonOptions()
{
    return new JsonSerializerOptions
    {
        PropertyNameCaseInsensitive = true,
        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
        Converters =
        {
            new DateTimeOffsetConverter(),
        }
    };
}

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:
Missing null checks: The newly added factory method returns a configured JsonSerializerOptions instance but
added Initialize methods now mutate the passed options without visible null or edge-case
checks, which may rely on upstream guarantees not shown in the diff.

Referred Code
private static JsonSerializerOptions CreateDefaultJsonOptions()
{
    return new JsonSerializerOptions
    {
        PropertyNameCaseInsensitive = true,
        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
        Converters =
        {
            new DateTimeOffsetConverter(),
        }
    };

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:
Converter mutation: Modules now directly add converters to provided JsonSerializerOptions instances which
could be shared if upstream callers reuse options, potentially causing unintended side
effects unless the new per-call factory guarantees isolation across uses.

Referred Code

... (clipped 3 lines)


</details>

> Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a>
</details></td></tr>

<tr><td align="center" colspan="2">

<!-- placeholder --> <!-- /compliance --update_compliance=true -->

</td></tr></tbody></table>
<details><summary>Compliance status legend</summary>
🟢 - Fully Compliant<br>
🟡 - Partial Compliant<br>
🔴 - Not Compliant<br>
⚪ - Requires Further Human Verification<br>
🏷️ - Compliance label<br>
</details>

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Optimize by reusing default converters

Optimize CreateDefaultJsonOptions by caching the default JSON converters in a
static readonly field to avoid creating a new list on every call.

dotnet/src/webdriver/BiDi/BiDi.cs [93-105]

+private static readonly List<JsonConverter> DefaultJsonConverters =
+[
+    new DateTimeOffsetConverter(),
+];
+
 private static JsonSerializerOptions CreateDefaultJsonOptions()
 {
-    return new JsonSerializerOptions
+    var options = new JsonSerializerOptions
     {
         PropertyNameCaseInsensitive = true,
         PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
         DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
-        Converters =
-        {
-            new DateTimeOffsetConverter(),
-        }
     };
+
+    foreach (var converter in DefaultJsonConverters)
+    {
+        options.Converters.Add(converter);
+    }
+
+    return options;
 }
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies a small inefficiency and proposes a valid optimization to cache the default converters list, but the performance gain is marginal as this code is not on a hot path.

Low
  • More

@nvborisenko nvborisenko merged commit d9d1d79 into SeleniumHQ:trunk Dec 6, 2025
13 checks passed
@nvborisenko nvborisenko deleted the bidi-new-json-context branch December 6, 2025 18:31
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