Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Dec 6, 2025

User description

💥 What does this PR do?

  • DTO type is a record
  • Equality by ID only
  • Improve ToString() method excluding BiDi (friendly for AOT)

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

PR Type

Enhancement, Bug fix


Description

  • Convert BrowsingContext to record type with ID-based equality

  • Implement consistent equality and hash code across BiDi DTOs

  • Add PrintMembers() for AOT-friendly string representation

  • Standardize ID comparison using ordinal string comparison


Diagram Walkthrough

flowchart LR
  A["BiDi DTO Classes"] -->|Convert to record| B["BrowsingContext record"]
  A -->|Add equality methods| C["Equals by ID only"]
  A -->|Implement PrintMembers| D["AOT-friendly ToString"]
  C -->|Use StringComparer.Ordinal| E["Consistent comparison"]
Loading

File Walkthrough

Relevant files
Enhancement
9 files
BrowsingContext.cs
Convert to record with ID-based equality                                 
+12/-6   
UserContext.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
Collector.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
Intercept.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
Handle.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
InternalId.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
PreloadScript.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
Realm.cs
Add ID-based equality and PrintMembers                                     
+18/-0   
Extension.cs
Add ID-based equality and PrintMembers                                     
+18/-0   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Dec 6, 2025
@nvborisenko nvborisenko changed the title [dotnet] [bidi] BrowsingCoontext type as record with equality [dotnet] [bidi] BrowsingContext type as record with equality Dec 6, 2025
@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: 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

Generic: Comprehensive Audit Trails

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

Status:
No audit logs: The new equality, hash code, and PrintMembers changes introduce no logging for critical
actions, but it's unclear whether such logging is applicable for these DTOs without
broader context.

Referred Code
public Task<Subscription> OnNavigationCommittedAsync(Func<NavigationInfo, Task> handler, ContextSubscriptionOptions? options = null)
{
    return BiDi.BrowsingContext.OnNavigationCommittedAsync(handler, options.WithContext(this));
}

public bool Equals(BrowsingContext? other)
{
    return other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal);
}

public override int GetHashCode()
{
    return Id is not null ? StringComparer.Ordinal.GetHashCode(Id) : 0;
}

// Includes Id only for brevity
private bool PrintMembers(StringBuilder builder)
{
    builder.Append($"Id = {Id}");
    return true;
}

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

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

@qodo-code-review
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Use a base record for common logic

To reduce code duplication, create an abstract base record to centralize the
identical Id-based equality, hash code, and PrintMembers logic currently
repeated across nine different record types.

Examples:

dotnet/src/webdriver/BiDi/Browser/UserContext.cs [51-66]
    public bool Equals(UserContext? other)
    {
        return other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal);
    }

    public override int GetHashCode()
    {
        return Id is not null ? StringComparer.Ordinal.GetHashCode(Id) : 0;
    }


 ... (clipped 6 lines)
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [234-249]
    public bool Equals(BrowsingContext? other)
    {
        return other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal);
    }

    public override int GetHashCode()
    {
        return Id is not null ? StringComparer.Ordinal.GetHashCode(Id) : 0;
    }


 ... (clipped 6 lines)

Solution Walkthrough:

Before:

// In UserContext.cs, Collector.cs, Intercept.cs, etc.
public sealed record UserContext
{
    internal string Id { get; }
    // ... other properties and methods

    public bool Equals(UserContext? other)
    {
        return other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal);
    }

    public override int GetHashCode()
    {
        return Id is not null ? StringComparer.Ordinal.GetHashCode(Id) : 0;
    }

    private bool PrintMembers(StringBuilder builder)
    {
        builder.Append($"Id = {Id}");
        return true;
    }
}

After:

// New base record
public abstract record BiDiEntity(string Id)
{
    public virtual bool Equals(BiDiEntity? other)
    {
        return other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal);
    }

    public override int GetHashCode() => StringComparer.Ordinal.GetHashCode(Id ?? string.Empty);

    protected virtual bool PrintMembers(StringBuilder builder)
    {
        builder.Append($"Id = {Id}");
        return true;
    }
}

// In UserContext.cs, Collector.cs, Intercept.cs, etc.
public sealed record UserContext(string Id) : BiDiEntity(Id);
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies significant code duplication across nine files and proposes a valid architectural improvement to centralize common logic, enhancing maintainability and adhering to the DRY principle.

High
Learned
best practice
Centralize equality helpers

These equality and PrintMembers implementations are duplicated across multiple
record types; extract a shared helper/base to centralize the logic and prevent
drift.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [234-249]

-public bool Equals(BrowsingContext? other)
-{
-    return other is not null && string.Equals(Id, other.Id, StringComparison.Ordinal);
-}
+// Example: use a shared static helper
+public bool Equals(BrowsingContext? other) => EqualityHelpers.IdEquals(Id, other?.Id);
 
-public override int GetHashCode()
-{
-    return Id is not null ? StringComparer.Ordinal.GetHashCode(Id) : 0;
-}
+public override int GetHashCode() => EqualityHelpers.IdHashCode(Id);
 
-// Includes Id only for brevity
-private bool PrintMembers(StringBuilder builder)
-{
-    builder.Append($"Id = {Id}");
-    return true;
-}
+private bool PrintMembers(StringBuilder builder) => EqualityHelpers.AppendId(builder, Id);
  • 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
General
Simplify GetHashCode implementation for conciseness

Simplify the GetHashCode implementation by using the null-coalescing operator
(??) with string.Empty for null Id values, instead of a ternary conditional
check.

dotnet/src/webdriver/BiDi/Browser/UserContext.cs [56-59]

 public override int GetHashCode()
 {
-    return Id is not null ? StringComparer.Ordinal.GetHashCode(Id) : 0;
+    return StringComparer.Ordinal.GetHashCode(Id ?? string.Empty);
 }
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion improves code conciseness by using the null-coalescing operator, which is a valid stylistic improvement, but it has a very low impact on the overall code quality.

Low
  • More

@nvborisenko nvborisenko merged commit 54f45d3 into SeleniumHQ:trunk Dec 6, 2025
13 checks passed
@nvborisenko nvborisenko deleted the bidi-dto-equlity branch December 6, 2025 18:27
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