Skip to content

[dotnet] [bidi] Improve json discriminator determination - #17309

Merged
nvborisenko merged 1 commit into
SeleniumHQ:trunkfrom
nvborisenko:bidi-discriminator-improvement
Apr 7, 2026
Merged

nvborisenko merged 1 commit into
SeleniumHQ:trunkfrom
nvborisenko:bidi-discriminator-improvement

Conversation

@nvborisenko

Copy link
Copy Markdown
Member

Just small improvement.

💥 What does this PR do?

This pull request refines the logic for extracting a property value from a JSON object using a Utf8JsonReader. The main improvement is making the property name comparison more efficient and clarifying the reader's movement through the JSON structure.

JSON parsing improvements:

  • Replaced string comparison for property names with the more efficient ValueTextEquals method, improving performance and reliability when matching the property name.
  • Clarified and corrected the advancement of the JSON reader: added comments and ensured the reader correctly skips over property values (including nested objects or arrays) to move to the next property or end of the object.

🔄 Types of changes

  • Cleanup (formatting, renaming)

Copilot AI review requested due to automatic review settings April 7, 2026 09:09
@qodo-code-review

Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Optimize JSON discriminator extraction with ValueTextEquals

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Optimize JSON property matching using ValueTextEquals method
• Improve code clarity with detailed reader movement comments
• Streamline discriminator extraction logic flow

Grey Divider

File Changes

1. dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs ✨ Enhancement +6/-7

Improve JSON discriminator extraction efficiency

• Replaced string comparison with ValueTextEquals for more efficient property name matching
• Added clarifying comments explaining Utf8JsonReader token advancement
• Reorganized reader movement logic to skip property values correctly
• Simplified conditional flow by moving Read() call inside the matching condition

dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs


Grey Divider

Qodo Logo

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Apr 7, 2026
@qodo-code-review

qodo-code-review Bot commented Apr 7, 2026 •

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0)   📘 Rule violations (2)   📎 Requirement gaps (0)   🎨 UX Issues (0)
📘\ ☼ Reliability (1) ⚙ Maintainability (1)

Grey Divider


Remediation recommended

1. Reader comments restate code 📘 ⚙
Description
New inline comments explain reader movement step-by-step rather than the rationale, making the code
harder to maintain long-term. Per the checklist, comments should focus on intent/why instead of
narrating what the code already states.
Code

dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs[R36-49]

+        readerClone.Read(); // move past StartObject to first PropertyName
        while (readerClone.TokenType == JsonTokenType.PropertyName)
        {
-            string? propertyName = readerClone.GetString();
-            readerClone.Read();
-
-            if (propertyName == name)
+            if (readerClone.ValueTextEquals(name))
            {
+                readerClone.Read(); // move to the property value
                discriminator = readerClone.GetString();

                break;
            }

-            readerClone.Skip();
-            readerClone.Read();
+            readerClone.Read(); // move to the property value
+            readerClone.Skip(); // skip the value (including nested objects/arrays)
+            readerClone.Read(); // move to the next PropertyName or EndObject
Evidence
The added inline comments (e.g., moving to property values, skipping values, moving to next token)
describe the mechanics of Utf8JsonReader advancement rather than the intent behind the approach,
which conflicts with the comment guidance in the checklist.

AGENTS.md
dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs[36-49]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Inline comments added in `GetDiscriminator` primarily narrate token-by-token reader movement (what the code does) rather than explaining the rationale/intent (why this sequence is required).

## Issue Context
The compliance checklist requires comments to explain intent/why, not restate code mechanics.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs[36-49]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. GetDiscriminator lacks unit tests 📘 ☼
Description
The PR changes JSON parsing/discriminator detection logic but does not add or update tests to lock
in the expected behavior. This increases regression risk for BiDi message parsing changes.
Code

dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs[R36-50]

+        readerClone.Read(); // move past StartObject to first PropertyName
        while (readerClone.TokenType == JsonTokenType.PropertyName)
        {
-            string? propertyName = readerClone.GetString();
-            readerClone.Read();
-
-            if (propertyName == name)
+            if (readerClone.ValueTextEquals(name))
            {
+                readerClone.Read(); // move to the property value
                discriminator = readerClone.GetString();

                break;
            }

-            readerClone.Skip();
-            readerClone.Read();
+            readerClone.Read(); // move to the property value
+            readerClone.Skip(); // skip the value (including nested objects/arrays)
+            readerClone.Read(); // move to the next PropertyName or EndObject
        }
Evidence
The diff shows behavior-affecting changes to discriminator extraction (property matching and reader
advancement/skip behavior), but there are no corresponding test changes in the provided PR diff.

AGENTS.md
dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs[36-50]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GetDiscriminator` parsing logic was modified, but there are no accompanying unit tests to validate discriminator extraction across typical JSON shapes.

## Issue Context
This logic is used to select derived types/events from BiDi JSON payloads; regressions can break message handling.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Json/JsonExtensions.cs[36-50]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refines the Utf8JsonReader-based discriminator extraction used by the .NET BiDi implementation to make property-name matching more efficient and to make reader advancement/skip behavior clearer and more consistent.

Changes:

  • Replace GetString() + string equality with Utf8JsonReader.ValueTextEquals(...) when comparing property names.
  • Make reader movement explicit by reading to the property value before GetString()/Skip(), and adding clarifying inline comments.

@nvborisenko
nvborisenko merged commit a9285c8 into SeleniumHQ:trunk Apr 7, 2026
21 checks passed
@nvborisenko
nvborisenko deleted the bidi-discriminator-improvement branch April 7, 2026 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-dotnet .NET Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants