Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 23, 2025

User description

Follow-up on 4c0eb7f#r171146028

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Replaces build-time resource generation with C# source generator

  • Adds Microsoft.CodeAnalysis NuGet dependencies for code generation

  • Removes legacy generated_resource_utilities Bazel rule

  • Integrates source generator into WebDriver compilation pipeline


Diagram Walkthrough

flowchart LR
  A["Build-time Resource Generation"] -->|replaced by| B["C# Source Generator"]
  C["JavaScript/JSON Files"] -->|processed by| B
  B -->|generates| D["ResourceUtilities.g.cs"]
  D -->|compiled into| E["WebDriver Assembly"]
Loading

File Walkthrough

Relevant files
Enhancement
1 files
ResourceUtilitiesAtomGenerator.cs
New incremental source generator for resource utilities   
+142/-0 
Configuration changes
7 files
Selenium.WebDriver.SourceGenerator.csproj
Project file for source generator NuGet package                   
+24/-0   
BUILD.bazel
Bazel build configuration for source generator library     
+25/-0   
.analyzerconfig
Analyzer configuration for extended rules enforcement       
+1/-0     
defs.bzl
Remove generated_resource_utilities Bazel rule export       
+0/-2     
Selenium.sln
Add source generator project to Visual Studio solution     
+6/-0     
BUILD.bazel
Replace resource generation with source generator dependency
+33/-21 
Selenium.WebDriver.csproj
Add source generator analyzer and AdditionalFiles references
+13/-6   
Dependencies
2 files
paket.dependencies
Add Microsoft.CodeAnalysis NuGet package dependencies       
+3/-0     
paket.nuget.bzl
Update NuGet package definitions with CodeAnalysis packages
+8/-1     

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations labels Nov 23, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 23, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Out-of-bounds read

Description: The kebab-to-Pascal case conversion reads the next character after a non-letter without
bounds checking, which can read past the end of the string when the input ends with a
non-letter (e.g., trailing dash), risking an IndexOutOfRangeException during generation.
ResourceUtilitiesAtomGenerator.cs [118-141]

Referred Code
private static string KebabCaseToPascalCase(string v)
{
    Span<char> newValues = new char[v.Length];
    int newValuesOffset = 0;
    for (int i = 0; i < v.Length; i++)
    {
        if (i == 0)
        {
            newValues[i - newValuesOffset] = char.ToUpperInvariant(v[i]);
        }
        else if (char.IsLetter(v[i]))
        {
            newValues[i - newValuesOffset] = v[i];
        }
        else
        {
            i++;
            newValuesOffset++;
            newValues[i - newValuesOffset] = char.ToUpperInvariant(v[i]);
        }
    }


 ... (clipped 3 lines)
Ticket Compliance
🟡
🎫 #5678
🔴 Investigate and resolve ChromeDriver connection failures showing "Error: ConnectFailure
(Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04
with Chrome 65/ChromeDriver 2.35 using Selenium 3.9.0.
Provide a fix or guidance so subsequent ChromeDriver instantiations do not log the
ConnectFailure error.
Validate behavior across multiple instantiations and ensure console remains free of the
reported error after the first instance.
🟡
🎫 #1234
🔴 Ensure WebDriver click() triggers JavaScript in link href as it did in 2.47.1, fixing
regressions observed in 2.48.x on Firefox 42.
Provide tests validating that clicking executes the JS handler in Firefox.
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 audit logs: The new source generator processes files and emits diagnostics but does not log any
critical security-relevant actions, which may be acceptable for build-time tooling but
cannot be verified from the diff.

Referred Code
public void Initialize(IncrementalGeneratorInitializationContext context)
{
    var jsFiles = context.AdditionalTextsProvider
        .Where(static (text) => text.Path.EndsWith(".js") || text.Path.EndsWith(".json"))
        .WithTrackingName("AtomFiles")
        .Select(static (data, token) =>
        {
            var name = Path.GetFileName(data.Path);
            var code = GenerateAtom(data, token, out var diagnostics);

            return (name, code, diagnostics);
        });

    context.RegisterSourceOutput(jsFiles, static (context, pair) =>
    {
        foreach (var diagnostic in pair.diagnostics)
        {
            context.ReportDiagnostic(diagnostic);
        }
        if (pair.code is not null)
        {


 ... (clipped 10 lines)

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:
Limited errors: The generator reports diagnostics for unreadable sources and unknown files but lacks
handling for null/empty AdditionalFiles inputs and potential index bounds in
KebabCaseToPascalCase, which might cause build-time failures without clear context.

Referred Code
private static string KebabCaseToPascalCase(string v)
{
    Span<char> newValues = new char[v.Length];
    int newValuesOffset = 0;
    for (int i = 0; i < v.Length; i++)
    {
        if (i == 0)
        {
            newValues[i - newValuesOffset] = char.ToUpperInvariant(v[i]);
        }
        else if (char.IsLetter(v[i]))
        {
            newValues[i - newValuesOffset] = v[i];
        }
        else
        {
            i++;
            newValuesOffset++;
            newValues[i - newValuesOffset] = char.ToUpperInvariant(v[i]);
        }
    }


 ... (clipped 3 lines)

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:
Input validation: The generator accepts arbitrary .js/.json AdditionalFiles and embeds their contents into
constants without explicit size or content validation, which is likely fine for build-time
controlled inputs but cannot be fully assessed from the diff.

Referred Code
var jsFiles = context.AdditionalTextsProvider
    .Where(static (text) => text.Path.EndsWith(".js") || text.Path.EndsWith(".json"))
    .WithTrackingName("AtomFiles")
    .Select(static (data, token) =>
    {
        var name = Path.GetFileName(data.Path);
        var code = GenerateAtom(data, token, out var diagnostics);

        return (name, code, diagnostics);
    });

context.RegisterSourceOutput(jsFiles, static (context, pair) =>
{
    foreach (var diagnostic in pair.diagnostics)
    {
        context.ReportDiagnostic(diagnostic);
    }
    if (pair.code is not null)
    {
        var propertyName = GetPropertyNameFromFilePath(pair.name, out _, out var diag);
        if (diag is not null)


 ... (clipped 6 lines)

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-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 23, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
The Bazel build configuration is incorrect

The Bazel build configuration in dotnet/src/webdriver/BUILD.bazel is incorrect.
It uses the resources attribute to provide files to the new source generator,
but it should use additional_files to ensure the generator receives them
correctly during a Bazel build.

Examples:

dotnet/src/webdriver/BUILD.bazel [43-49]
    resources = [
        "//javascript/atoms/fragments:find-elements.js",
        "//javascript/atoms/fragments:is-displayed.js",
        "//javascript/cdp-support:mutation-listener.js",
        "//javascript/webdriver/atoms:get-attribute.js",
        "//third_party/js/selenium:webdriver_json",
    ],

Solution Walkthrough:

Before:

# In dotnet/src/webdriver/BUILD.bazel
csharp_library(
    name = "webdriver-netstandard2.0",
    srcs = [ ... ],
    # ...
    resources = [
        "//javascript/atoms/fragments:find-elements.js",
        "//javascript/atoms/fragments:is-displayed.js",
        "//javascript/cdp-support:mutation-listener.js",
        "//javascript/webdriver/atoms:get-attribute.js",
        "//third_party/js/selenium:webdriver_json",
    ],
    # ...
    deps = [
        # ...
        "//dotnet/private/Selenium.WebDriver.SourceGenerator:resource-utilities-generator",
    ],
)

After:

# In dotnet/src/webdriver/BUILD.bazel
csharp_library(
    name = "webdriver-netstandard2.0",
    srcs = [ ... ],
    # ...
    additional_files = [
        "//javascript/atoms/fragments:find-elements.js",
        "//javascript/atoms/fragments:is-displayed.js",
        "//javascript/cdp-support:mutation-listener.js",
        "//javascript/webdriver/atoms:get-attribute.js",
        "//third_party/js/selenium:webdriver_json",
    ],
    # ...
    deps = [
        # ...
        "//dotnet/private/Selenium.WebDriver.SourceGenerator:resource-utilities-generator",
    ],
)
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical flaw in the Bazel build configuration where resources are used instead of the required additional_files attribute, which will cause the source generator to fail and break the build.

High
Possible issue
Remove duplicate additional file entry

Remove the duplicate entry for get-attribute.js in Selenium.WebDriver.csproj.
This duplication would cause the source generator to fail and break the build.

dotnet/src/webdriver/Selenium.WebDriver.csproj [90-94]

-<AdditionalFiles Include="$(ProjectDir)..\..\..\bazel-bin\javascript\webdriver\atoms\get-attribute.js" />
 <AdditionalFiles Include="$(ProjectDir)..\..\..\bazel-bin\javascript\webdriver\atoms\get-attribute.js" />
 <AdditionalFiles Include="$(ProjectDir)..\..\..\bazel-bin\javascript\atoms\fragments\is-displayed.js" />
 <AdditionalFiles Include="$(ProjectDir)..\..\..\bazel-bin\javascript\atoms\fragments\find-elements.js" />
 <AdditionalFiles Include="$(ProjectDir)..\..\..\javascript\cdp-support\mutation-listener.js" />
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a duplicate AdditionalFiles entry for get-attribute.js which would cause a build failure due to the source generator creating duplicate code. This is a critical fix to make the build succeed.

High
Prevent out-of-bounds array access

Refactor the KebabCaseToPascalCase method to prevent a potential
IndexOutOfRangeException when the input string ends with a non-letter character.
The suggested implementation using string.Split is safer and more robust.

dotnet/private/Selenium.WebDriver.SourceGenerator/ResourceUtilitiesAtomGenerator.cs [118-141]

 private static string KebabCaseToPascalCase(string v)
 {
-    Span<char> newValues = new char[v.Length];
-    int newValuesOffset = 0;
-    for (int i = 0; i < v.Length; i++)
+    if (string.IsNullOrEmpty(v))
     {
-        if (i == 0)
+        return string.Empty;
+    }
+
+    var parts = v.Split(new[] { '-' }, StringSplitOptions.RemoveEmptyEntries);
+    var builder = new StringBuilder();
+    foreach (var part in parts)
+    {
+        if (part.Length > 0)
         {
-            newValues[i - newValuesOffset] = char.ToUpperInvariant(v[i]);
-        }
-        else if (char.IsLetter(v[i]))
-        {
-            newValues[i - newValuesOffset] = v[i];
-        }
-        else
-        {
-            i++;
-            newValuesOffset++;
-            newValues[i - newValuesOffset] = char.ToUpperInvariant(v[i]);
+            builder.Append(char.ToUpperInvariant(part[0]));
+            if (part.Length > 1)
+            {
+                builder.Append(part.AsSpan(1));
+            }
         }
     }
-
-    return newValues.Slice(0, v.Length - newValuesOffset).ToString();
+    return builder.ToString();
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies an IndexOutOfRangeException bug in the KebabCaseToPascalCase method for certain inputs, and the proposed fix is more robust, readable, and idiomatic.

Medium
Learned
best practice
Validate inputs and cancellation

Check for token cancellation explicitly and return early with a diagnostic, and
validate file content isn't empty to avoid generating invalid sources.

dotnet/private/Selenium.WebDriver.SourceGenerator/ResourceUtilitiesAtomGenerator.cs [50-56]

 var sourceText = additionalText.GetText(token);
-if (sourceText is null)
+if (token.IsCancellationRequested)
 {
-    var d = Diagnostic.Create(new DiagnosticDescriptor("WRG1001", "Failed to read atom", "Atom '{0}' could not be read", "WebDriverResourceGenerator", DiagnosticSeverity.Error, true), Location.None, additionalText.Path);
-    diagnostics = [d];
+    diagnostics = [Diagnostic.Create(new DiagnosticDescriptor("WRG1003", "Generation canceled", "Generation canceled while reading '{0}'", "WebDriverResourceGenerator", DiagnosticSeverity.Info, true), Location.None, additionalText.Path)];
+    return null;
+}
+if (sourceText is null || sourceText.Length == 0)
+{
+    diagnostics = [Diagnostic.Create(new DiagnosticDescriptor("WRG1001", "Failed to read atom", "Atom '{0}' could not be read or was empty", "WebDriverResourceGenerator", DiagnosticSeverity.Error, true), Location.None, additionalText.Path)];
     return null;
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Guard external I/O operations with validation and clear error handling.

Low
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-dotnet .NET Bindings Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants