[dotnet] [bidi] Don't warn if there are no subscribers

[nvborisenko]

Any neighbor is able to enable subscription in the browser. Just don't warn, it is legal case.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated:
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Cleanup (formatting, renaming)

[qodo-code-review]

PR Summary by Qodo

[dotnet][bidi] Avoid warning when BiDi events have no subscribers

🐞 Bug fix 🕐 Less than 10 minutes

AI Description

• Stop emitting WARN logs when a BiDi event has no local subscribers/mapping.
• Always attempt event deserialization/dispatch and silently ignore unhandled events.
• Reduce log noise for valid cross-session subscription scenarios.

Diagram

graph TD
  A["BiDi Broker"] --> B["ProcessReceivedMessage"] --> C{Type == event?} -->|Yes| D["EventDispatcher"] --> E["TryDeserializeAndDispatch"]
  C -->|No| F["Other message handling"]

  subgraph Legend
    direction LR
    _proc["Processor"] ~~~ _dec{"Decision"} ~~~ _comp["Component"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Downgrade to DEBUG/TRACE instead of removing WARN

• ➕ Keeps a breadcrumb for diagnosing unexpected event methods
• ➖ Still adds noise in valid scenarios where events are intentionally unhandled/no-subscribed

2. Warn only for explicitly unknown/invalid methods (schema-level)

• ➕ Preserves signal for truly suspicious events while avoiding benign cases
• ➖ Requires clearer distinction between “no subscribers”, “no mapping”, and “invalid event”, which may not exist today

Recommendation: Current approach is appropriate if “unhandled event” is a normal/benign condition in BiDi (e.g., other sessions enabling subscriptions). If future diagnostics are needed, prefer a DEBUG-level counter/metric rather than WARN logs.

Files changed (1) +1 / -7

Bug fix (1) +1 / -7

Broker.csRemove WARN logging for unhandled BiDi events +1/-7

Remove WARN logging for unhandled BiDi events

• Eliminates the warning log emitted when TryDeserializeAndDispatch returns false. Event dispatch is still attempted, but unhandled events are now silently ignored to avoid noisy logs in valid subscription scenarios.

dotnet/src/webdriver/BiDi/Broker.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 15 rules

1. Dispatch condition inverted ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

EventDispatcher.DeserializeAndDispatch() runs deserialization/dispatch only when the event method is
NOT found in _events, so mapped events are never delivered and unmapped events can hit a
NullReferenceException by dereferencing a null slot. This breaks all BiDi event subscriptions and
turns “unknown event” into an exception path.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[R116-119]

        if (!_events.TryGetValue(method, out var slot))
        {
-            return false;
-        }
-
-        var eventArgs = (EventArgs)(JsonSerializer.Deserialize(ref paramsReader, slot.JsonTypeInfo)
+            var eventArgs = (EventArgs)(JsonSerializer.Deserialize(ref paramsReader, slot.JsonTypeInfo)
            ?? throw new BiDiException("Remote end returned null event args in the 'params' property."));

Evidence

The code currently deserializes and dispatches inside if (!_events.TryGetValue(...)), but uses
slot.JsonTypeInfo and iterates slot.GetSnapshot(). Broker invokes this method for each received
event, so the inverted condition drops all mapped events and makes unknown events error-prone.

dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-139]
dotnet/src/webdriver/BiDi/Broker.cs[307-317]

Agent prompt

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

### Issue description
`DeserializeAndDispatch()` currently enters the dispatch block when `_events.TryGetValue()` **fails**, but the block uses `slot.JsonTypeInfo` and `slot.GetSnapshot()`.

This results in:
- **Known** (registered) event methods: block is skipped → event is never dispatched.
- **Unknown** event methods: `slot` is null/default → dereference leads to runtime exception.

### Issue Context
`Broker.ProcessReceivedMessage()` calls `EventDispatcher.DeserializeAndDispatch()` for every incoming BiDi event, so this inverted condition prevents the BiDi subscription mechanism from functioning.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-139]

### Expected change (one valid approach)
- Change to:
 - `if (!_events.TryGetValue(method, out var slot)) return;`
 - Then deserialize and deliver **outside** that guard (or invert the condition to `if (_events.TryGetValue(...)) { ... }`).
- Ensure the “unknown event method” path does not dereference `slot` and does not throw.

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

---

2. DeserializeAndDispatch missing XML docs 📘 Rule violation ✧ Quality ⭐ New

Description

The modified public method DeserializeAndDispatch has no XML documentation comment with a
non-empty <summary> block. This violates the requirement to document all public API members in the
change set.

Code

dotnet/src/webdriver/BiDi/EventDispatcher.cs[114]

+    public void DeserializeAndDispatch(string method, ref Utf8JsonReader paramsReader, Dictionary<string, JsonElement>? additionalMessageData = null)

Evidence

PR Compliance ID 389245 requires XML documentation with a <summary> for all public members in the
diff. The method public void DeserializeAndDispatch(...) appears without any preceding `///
<summary>` comment.

Rule 389245: Require XML documentation with <summary> for all public API members
dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-115]

Agent prompt

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

## Issue description
A public method was modified/introduced without required XML documentation.

## Issue Context
Per compliance requirements, all `public` members in the diff must have an XML doc comment block (///) immediately preceding the declaration, including a non-empty `<summary>` element.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-115]

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

---

3. Warn log loses stacktrace ✓ Resolved 🐞 Bug ◔ Observability ⭐ New

Description

Broker logs event dispatch failures using only ex.Message, which omits exception type and stack
trace, making deserialization/dispatch failures significantly harder to debug. Given ILogger has no
exception-parameter overload, the message must include ex.ToString() (or interpolate the exception
itself) to preserve details.

Code

dotnet/src/webdriver/BiDi/Broker.cs[R314-316]

+                catch (Exception ex)
+                {
+                    _logger.Warn($"Failed to deserialize and dispatch '{method}' event: {ex.Message}. Message content: {System.Text.Encoding.UTF8.GetString(data.ToArray())}");

Evidence

Broker’s catch block interpolates only ex.Message. The project’s ILogger interface does not
provide an overload that accepts an Exception, so the only way to preserve stack trace is to include
ex/ex.ToString() in the message.

dotnet/src/webdriver/BiDi/Broker.cs[310-317]
dotnet/src/webdriver/Internal/Logging/ILogger.cs[65-76]

Agent prompt

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

### Issue description
The catch block logs only `ex.Message`, which drops stack trace and exception type.

### Issue Context
`OpenQA.Selenium.Internal.Logging.ILogger` only exposes `Warn(string)` / interpolated-string-handler overloads, so you can’t pass an `Exception` separately; the exception details must be included in the formatted message.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[310-316]

### Suggested fix
- Change the warning message to include the full exception (e.g., `{ex}`) rather than `{ex.Message}`:
 - `... event: {ex}. ...`
 - (or explicitly `... event: {ex.ToString()}. ...`)

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

---

4. No test for removed warning ⊘ Outdated 📘 Rule violation ▣ Testability

Description

The change removes the warning/logging behavior for unhandled BiDi events, but the PR does not
include any corresponding test update to prevent regressions. This violates the requirement that bug
fixes and behavior changes add or update automated tests.

Code

dotnet/src/webdriver/BiDi/Broker.cs[310]

+                _bidi.EventDispatcher.TryDeserializeAndDispatch(method, ref paramsReader, additionalMessageData);

Evidence

PR Compliance ID 389273 requires adding/updating tests for behavior changes and bug fixes. The diff
removes the conditional warning path and now unconditionally calls TryDeserializeAndDispatch,
changing observable behavior without any accompanying test change in this PR.

Rule 389273: Require tests for all new functionality and bug fixes
dotnet/src/webdriver/BiDi/Broker.cs[307-312]

Agent prompt

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

## Issue description
A behavior change in BiDi event handling removed the warning previously emitted when an incoming event could not be deserialized/handled, but no automated test was added/updated to lock in the intended behavior.

## Issue Context
`Broker.ProcessReceivedMessage` now calls `EventDispatcher.TryDeserializeAndDispatch(...)` without checking the return value and without warning logs. Add a regression test that would fail on the previous implementation (where a warning was emitted) and pass now.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[307-312]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[154-210]

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

---

View more (1)

5. Silent unhandled events ✓ Resolved 🐞 Bug ◔ Observability

Description

Broker.ProcessReceivedMessage no longer checks the boolean return from
EventDispatcher.TryDeserializeAndDispatch(), so events whose method has no registered slot are now
ignored without any log at this call site. This reduces diagnosability of
unexpected/protocol-mismatch events compared to the prior behavior which emitted a warning when
dispatch returned false.

Code

dotnet/src/webdriver/BiDi/Broker.cs[310]

+                _bidi.EventDispatcher.TryDeserializeAndDispatch(method, ref paramsReader, additionalMessageData);

Evidence

The PR change removes the conditional branch that logged when dispatch failed. The dispatch method
returns false when no event slot exists for the received method, so those events now produce no log
from Broker.

dotnet/src/webdriver/BiDi/Broker.cs[307-313]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-143]

Agent prompt

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

### Issue description
`Broker.ProcessReceivedMessage` used to emit a warning when `EventDispatcher.TryDeserializeAndDispatch()` returned `false`. The PR now ignores the return value entirely, meaning events whose `method` has no registered slot are dropped with no diagnostic log from this call site.

### Issue Context
`EventDispatcher.TryDeserializeAndDispatch()` returns `false` specifically when the event `method` is not present in `_events`. The prior behavior warned and included the full message content; the new behavior always calls the method and does nothing if it returns `false`.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[307-313]
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-143]

### Suggested fix
Re-introduce handling of the `false` return, but log at a lower level (e.g., Debug/Trace) or with a less alarming message (e.g., "Ignoring BiDi event '{method}' with no local subscriptions") to meet the PR goal of avoiding Warn-level noise while retaining a diagnostic breadcrumb for unexpected events.

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit 8e1f790

Results up to commit 3a61d57 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. No test for removed warning ⊘ Outdated 📘 Rule violation ▣ Testability

Description

The change removes the warning/logging behavior for unhandled BiDi events, but the PR does not
include any corresponding test update to prevent regressions. This violates the requirement that bug
fixes and behavior changes add or update automated tests.

Code

dotnet/src/webdriver/BiDi/Broker.cs[310]

+                _bidi.EventDispatcher.TryDeserializeAndDispatch(method, ref paramsReader, additionalMessageData);

Evidence

PR Compliance ID 389273 requires adding/updating tests for behavior changes and bug fixes. The diff
removes the conditional warning path and now unconditionally calls TryDeserializeAndDispatch,
changing observable behavior without any accompanying test change in this PR.

Rule 389273: Require tests for all new functionality and bug fixes
dotnet/src/webdriver/BiDi/Broker.cs[307-312]

Agent prompt

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

## Issue description
A behavior change in BiDi event handling removed the warning previously emitted when an incoming event could not be deserialized/handled, but no automated test was added/updated to lock in the intended behavior.

## Issue Context
`Broker.ProcessReceivedMessage` now calls `EventDispatcher.TryDeserializeAndDispatch(...)` without checking the return value and without warning logs. Add a regression test that would fail on the previous implementation (where a warning was emitted) and pass now.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[307-312]
- dotnet/test/webdriver/BiDi/SessionUnitTests.cs[154-210]

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

---

2. Silent unhandled events ✓ Resolved 🐞 Bug ◔ Observability

Description

Broker.ProcessReceivedMessage no longer checks the boolean return from
EventDispatcher.TryDeserializeAndDispatch(), so events whose method has no registered slot are now
ignored without any log at this call site. This reduces diagnosability of
unexpected/protocol-mismatch events compared to the prior behavior which emitted a warning when
dispatch returned false.

Code

dotnet/src/webdriver/BiDi/Broker.cs[310]

+                _bidi.EventDispatcher.TryDeserializeAndDispatch(method, ref paramsReader, additionalMessageData);

Evidence

The PR change removes the conditional branch that logged when dispatch failed. The dispatch method
returns false when no event slot exists for the received method, so those events now produce no log
from Broker.

dotnet/src/webdriver/BiDi/Broker.cs[307-313]
dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-143]

Agent prompt

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

### Issue description
`Broker.ProcessReceivedMessage` used to emit a warning when `EventDispatcher.TryDeserializeAndDispatch()` returned `false`. The PR now ignores the return value entirely, meaning events whose `method` has no registered slot are dropped with no diagnostic log from this call site.

### Issue Context
`EventDispatcher.TryDeserializeAndDispatch()` returns `false` specifically when the event `method` is not present in `_events`. The prior behavior warned and included the full message content; the new behavior always calls the method and does nothing if it returns `false`.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Broker.cs[307-313]
- dotnet/src/webdriver/BiDi/EventDispatcher.cs[114-143]

### Suggested fix
Re-introduce handling of the `false` return, but log at a lower level (e.g., Debug/Trace) or with a less alarming message (e.g., "Ignoring BiDi event '{method}' with no local subscriptions") to meet the PR goal of avoiding Warn-level noise while retaining a diagnostic breadcrumb for unexpected events.

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

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 451f737

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8e1f790