[js][bidi] Add BiDi connection-level event subscription

[pujagani]

🔗 Related Issues

💥 What does this PR do?

Domain.addCallback previously shipped as a placeholder with no real subscription
lifecycle. The JS generator gives every generated domain class an event API
that funnels solely through Domain.addCallback. Added connection level event subscription management.

🔧 Implementation Notes

🤖 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

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Add connection-scoped BiDi event subscription lifecycle

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Manage BiDi callbacks by server-assigned subscription IDs at the connection layer.
• Isolate event handler failures and preserve independent same-event subscriptions.
• Correct null primitive and nested-union serialization for generated domain payloads.

Diagram

sequenceDiagram
  actor Caller
  participant Domain as Generated Domain
  participant Connection as BiDi Connection
  participant Serializer as Type Parser
  participant Browser as Remote Browser
  Caller->>Domain: add callback
  Domain->>Connection: register dispatcher
  Connection->>Connection: attach listener
  Connection->>Browser: session.subscribe
  Browser-->>Connection: subscription ID
  Connection-->>Caller: subscription handle
  Browser-->>Connection: event payload
  Connection->>Serializer: parse payload
  Serializer-->>Caller: typed event
  Caller->>Connection: unsubscribe ID
  Connection->>Browser: session.unsubscribe
  Browser-->>Connection: removal confirmed
  Connection->>Connection: detach listener

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Client-side event ref-counting

• ➕ Could share one remote subscription among identical event listeners.
• ➕ May reduce session.subscribe calls for heavily shared events.
• ➖ Requires complex ownership and retry bookkeeping.
• ➖ Weakens independent cancellation semantics already provided by protocol subscription IDs.
• ➖ Complicates future context-scoped subscriptions.

2. Retain Domain-managed subscribe/on lifecycle

• ➕ Keeps changes localized to generated domain support.
• ➕ Reuses the existing subscribe and unsubscribe methods.
• ➖ Event-name unsubscription can remove unrelated registrations.
• ➖ Duplicates lifecycle state across domain instances.
• ➖ Cannot reliably preserve independently cancellable same-event callbacks.

Recommendation: Keep the connection-level, server-ID-keyed design. It centralizes lifecycle state where events are dispatched, preserves protocol-defined independent cancellation, and avoids fragile client-side ref-counting; the legacy name-scoped API can remain only for compatibility until hand-written domains migrate.

Files changed (9) +616 / -107

Enhancement (2) +179 / -22

domain.d.tsExpose subscription IDs from domain callbacks +7/-7

Expose subscription IDs from domain callbacks

• Updates the typed callback handle to include the server-assigned subscription ID. Documentation now identifies the connection as the lifecycle owner.

javascript/selenium-webdriver/bidi/domain.d.ts

index.jsManage BiDi callbacks by subscription ID +172/-15

Manage BiDi callbacks by subscription ID

• Adds connection-level callback registration, server-ID bookkeeping, precise unsubscription, rollback, and close cleanup. Event dispatch now isolates listener failures and safely reports protocol or handler errors without blocking sibling listeners.

javascript/selenium-webdriver/bidi/index.js

Bug fix (3) +29 / -19

domain.jsDelegate domain callback lifecycle to the connection +7/-16

Delegate domain callback lifecycle to the connection

• Replaces direct subscribe/on/off/unsubscribe handling with a call to the connection-level callback API. Domain remains responsible only for descriptor-based payload parsing.

javascript/selenium-webdriver/bidi/domain.js

record.jsValidate bare null primitive fields correctly +10/-1

Validate bare null primitive fields correctly

• Accepts null for schema nodes explicitly typed as the null primitive and rejects every non-null value.

javascript/selenium-webdriver/bidi/serialization/record.js

union.jsSupport unions resolving to nested unions +12/-2

Support unions resolving to nested unions

• Delegates outbound construction and inbound parsing when a selected variant is itself a union rather than a record.

javascript/selenium-webdriver/bidi/serialization/union.js

Tests (4) +408 / -66

domain_test.jsVerify domain callback delegation and parsing +18/-60

Verify domain callback delegation and parsing

• Refactors the fake connection around addCallback and verifies descriptor method forwarding, typed and untyped delivery, and unchanged subscription handles.

javascript/selenium-webdriver/test/bidi/domain_test.js

index_test.jsCover connection-level callback lifecycle end to end +329/-6

Cover connection-level callback lifecycle end to end

• Adds WebSocket integration tests for subscription IDs, independent listeners, early events, retries, close cleanup, malformed responses, and isolated handler failures. Also validates typed Domain dispatch over a real connection.

javascript/selenium-webdriver/test/bidi/index_test.js

record_test.jsCover null primitive serialization validation +27/-0

Cover null primitive serialization validation

• Verifies bare null fields accept null and reject non-null values in both outbound and inbound paths.

javascript/selenium-webdriver/test/bidi/serialization/record_test.js

union_test.jsCover nested default-union dispatch +34/-0

Cover nested default-union dispatch

• Models a discriminated union whose default arm is another union and verifies outbound and inbound recursive selection.

javascript/selenium-webdriver/test/bidi/serialization/union_test.js

[qodo-code-review]

Code Review by Qodo

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

1. Concurrent unsubscribe sends duplicates ✓ Resolved 📘 Rule violation ☼ Reliability ⭐ New

Description

Two overlapping calls to the same subscription handle's unsubscribe() can both observe the
callback entry and send session.unsubscribe for the same subscription ID before either deletes it,
allowing a conforming remote to reject the duplicate and making ordinary repeated cleanup
timing-dependent. The focused tests establish only sequential idempotency and do not cover this
concurrent race, leaving the new public subscription behavior unreliable and violating the
focused-test requirement.

Code

javascript/selenium-webdriver/bidi/index.js[R459-462]

+    const response = await this.send({
+      method: 'session.unsubscribe',
+      params: { subscriptions: [subscriptionId] },
+    })

Evidence

Rule 5 requires focused coverage of changed behavior. removeCallback() reads _callbacks before
awaiting the network request and does not mark the entry as being removed, so concurrent calls both
pass the guard and reach send(); the callback is deleted only after a successful response. The
returned handle invokes this method on every unsubscribe() call, while the current tests establish
repeat unsubscribe as a no-op only when calls are sequential and the first has already settled,
leaving overlapping calls untested.

AGENTS.md: Provide Focused Tests and Avoid Contract-Misrepresenting Mocks
javascript/selenium-webdriver/bidi/index.js[453-468]
javascript/selenium-webdriver/test/bidi/index_test.js[376-380]
javascript/selenium-webdriver/bidi/index.js[423-425]

Agent prompt

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

## Issue description
Concurrent calls to the same subscription handle's `unsubscribe()` can each send a separate `session.unsubscribe` for the same subscription ID, causing a valid repeated call to reject after the first request removes the remote subscription.

## Issue Context
The callback remains in `_callbacks` while the first remote unsubscribe request is pending, so another call cannot distinguish an active subscription from one already being removed. Track and return or share an in-flight removal promise so concurrent callers await the same result; remove the callback after success, but clear the in-flight marker and retain callback state after rejection so a later retry remains possible. Add a focused concurrent-unsubscribe test alongside the existing sequential idempotency and retry tests.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[453-468]
- javascript/selenium-webdriver/test/bidi/index_test.js[376-437]

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

---

2. Nested union defaults crash ✓ Resolved 🐞 Bug ≡ Correctness

Description

defineUnion().fromWire() assumes every selected ref is a record and calls
variant.RecordClass.fromWire(), but generated discriminated selectors may use another union as
their default arm. Valid payloads selecting that arm therefore throw a TypeError instead of being
parsed.

Code

javascript/selenium-webdriver/bidi/serialization/union.js[R83-84]

+      const variant = resolve(ref)
+      return variant.RecordClass.fromWire(payload)

Evidence

The projector documents and emits union refs in selector.default; union registry entries expose
build/fromWire, not RecordClass, while both runtime paths unconditionally dereference
RecordClass.

javascript/selenium-webdriver/project_bidi_schema.mjs[392-421]
javascript/selenium-webdriver/bidi/serialization/union.js[55-56]
javascript/selenium-webdriver/bidi/serialization/union.js[68-84]

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 selected union variant may itself be a union, but the serializer always treats it as a record.

## Issue Context
The schema projector explicitly emits nested unions as discriminated-selector defaults.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/union.js[68-84]
- javascript/selenium-webdriver/project_bidi_schema.mjs[392-421]

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

---

3. Unsubscribe failure loses callback ✓ Resolved 🐞 Bug ≡ Correctness

Description

removeCallback() removes the local subscription entry and event listener before it knows whether
session.unsubscribe succeeded, and it doesn’t inspect the unsubscribe response for protocol
errors. As a result, transport failures, timeouts, or protocol rejections can leave the remote
subscription active while the returned handle can no longer retry because subsequent calls find no
local entry and no-op, falsely indicating cleanup succeeded while events may continue.

Code

javascript/selenium-webdriver/bidi/index.js[R443-453]

+    this._callbacks.delete(subscriptionId)
+    this.off(entry.method, entry.handler)
+
+    if (this._closed) {
+      return
+    }
+
+    await this.send({
+      method: 'session.unsubscribe',
+      params: { subscriptions: [subscriptionId] },
+    })

Evidence

The cited implementation deletes the local callback map entry and listener before awaiting the
protocol unsubscribe, and it also short-circuits when the entry is missing, so once the first
attempt removes local state any later retry from the same handle becomes an immediate no-op without
contacting the browser. Additionally, the shared send()/response-dispatch path resolves pending
requests with the raw response payload (including error-shaped payloads) rather than throwing, and
other call sites like addCallback() (and Domain.send()) explicitly check response.error;
removeCallback() does not perform that validation and effectively discards the unsubscribe
response, so protocol-level unsubscribe rejections can be treated as success even though the remote
subscription remains active.

javascript/selenium-webdriver/bidi/index.js[403-412]
javascript/selenium-webdriver/bidi/index.js[437-453]
javascript/selenium-webdriver/bidi/index.js[245-270]
javascript/selenium-webdriver/bidi/index.js[437-444]
javascript/selenium-webdriver/bidi/index.js[450-453]
javascript/selenium-webdriver/bidi/index.js[118-124]
javascript/selenium-webdriver/bidi/index.js[403-409]

Agent prompt

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

## Issue description
`removeCallback()` currently clears local subscription bookkeeping (map entry and listener) before confirming that `session.unsubscribe` actually succeeded, and it does not validate the unsubscribe response for a protocol-level `error`. This can leave a remote subscription active after a transport failure, timeout, rejected send, or BiDi error response, while making the returned handle unable to retry because subsequent calls see no local entry and return without contacting the browser.

## Issue Context
The request/response plumbing (`Index.send()` and the shared response dispatcher) resolves pending sends with the raw protocol payload, including error responses, instead of throwing. Other code paths (e.g., `addCallback()` and `Domain.send()`) explicitly inspect `response.error` to detect protocol failures, but `removeCallback()` does not check the unsubscribe response at all. Because `removeCallback()` also deletes local state up front and has an early return when no map entry exists, a failed first unsubscribe attempt becomes irrecoverable and later retries become silent no-ops, potentially leaving the browser still producing events.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[437-454]
- javascript/selenium-webdriver/bidi/index.js[245-270]
- javascript/selenium-webdriver/bidi/index.js[403-409]

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

---

View action required (2)

4. Legacy unsubscribe cancels callbacks ✗ Dismissed 🐞 Bug ≡ Correctness

Description

addCallback() creates a subscription for an event that the existing name-scoped unsubscribe()
API can cancel remotely; closing a hand-written inspector for the same event therefore silently
stops the new callback while its handle and local listener remain active. This makes the new API
unreliable when mixed with existing BiDi modules on the same connection.

Code

javascript/selenium-webdriver/bidi/index.js[R399-402]

+      const response = await this.send({
+        method: 'session.subscribe',
+        params: { events: [method] },
+      })

Evidence

The new code creates an independent session.subscribe registration, but the existing unsubscribe
implementation sends event names. Existing inspectors invoke that path for the same event methods,
and the added documentation itself states that name-scoped unsubscribe can affect addCallback
subscriptions.

javascript/selenium-webdriver/bidi/index.js[274-284]
javascript/selenium-webdriver/bidi/index.js[328-366]
javascript/selenium-webdriver/bidi/logInspector.js[342-344]
javascript/selenium-webdriver/bidi/network.js[407-415]

Agent prompt

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

## Issue description
Connection-level callbacks can be remotely cancelled by the legacy event-name unsubscribe path.

## Issue Context
Existing inspectors call `Index.unsubscribe()` by event name, while `addCallback()` independently subscribes to the same events by subscription ID.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[289-366]
- javascript/selenium-webdriver/bidi/index.js[395-420]
- javascript/selenium-webdriver/bidi/logInspector.js[342-344]
- javascript/selenium-webdriver/bidi/network.js[407-415]

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

---

5. Generated events bypass lifecycle ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The generated public domain event methods still call the old event-name-scoped subscribe() and
attach an EventEmitter listener directly; none call the newly added Domain#addCallback().
Consequently generated event users receive no subscription handle and remain subject to the old
API's documented cross-subscription cancellation behavior instead of the lifecycle this PR adds.

Code

javascript/selenium-webdriver/bidi/domain.js[R81-83]

+  async addCallback(descriptor, handler) {
+    const dispatch = descriptor.type === undefined ? handler : (params) => handler(descriptor.type.fromWire(params))
+    return this.#bidi.addCallback(descriptor.method, dispatch)

Evidence

The new connection code explicitly identifies addCallback() as the replacement for the imprecise
legacy API, but the generator still emits only calls to that legacy API and direct on() listeners.
Since generated classes are produced by this template, adding an unused Domain method does not
change their behavior.

javascript/selenium-webdriver/bidi/domain.js[68-84]
javascript/selenium-webdriver/bidi/index.js[274-284]
javascript/selenium-webdriver/generate_bidi.mjs[1007-1034]
javascript/selenium-webdriver/generate_bidi.mjs[1085-1101]

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 new `Domain#addCallback()` is not used by generated domain event methods, so the generated API continues to use the legacy event-name-scoped subscription path and does not expose per-subscription unsubscription.

## Issue Context
`generate_bidi.mjs` is the source for every generated domain class. Its event-method template must construct/use the new event descriptor and delegate through the Domain lifecycle API (or otherwise call `Index#addCallback`) and return the unsubscribe handle with the generated event's typed payload.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/domain.js[81-83]
- javascript/selenium-webdriver/generate_bidi.mjs[1007-1048]
- javascript/selenium-webdriver/generate_bidi.mjs[1085-1101]

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

---

6. Declarations omitted from package ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

The production package glob adds only bidi/serialization/*.js, so the new serialization .d.ts
files—and likewise the new domain.d.ts—are absent from the published npm artifact. TypeScript
consumers of these new public modules consequently receive no declarations despite the PR defining
them.

Code

javascript/selenium-webdriver/BUILD.bazel[153]

+        "bidi/serialization/*.js",

Evidence

prod-src-files is included in the publishable npm target and selects only .js files under the
new paths; the repository's only declaration files are the four introduced here, and package.json
provides no alternate declaration entry.

javascript/selenium-webdriver/BUILD.bazel[133-180]
javascript/selenium-webdriver/package.json[17-17]
javascript/selenium-webdriver/bidi/domain.d.ts[18-55]
javascript/selenium-webdriver/bidi/serialization/record.d.ts[18-70]

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 npm packaging source set excludes the newly added declaration files.

## Issue Context
The package is built from `prod-src-files`; its globs select JavaScript only.

## Fix Focus Areas
- javascript/selenium-webdriver/BUILD.bazel[133-180]
- javascript/selenium-webdriver/bidi/domain.d.ts[18-55]
- javascript/selenium-webdriver/bidi/serialization/record.d.ts[18-70]
- javascript/selenium-webdriver/bidi/serialization/union.d.ts[18-36]
- javascript/selenium-webdriver/bidi/serialization/enum.d.ts[18-30]

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

---

7. Redundant _callbacks map comment ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The inline comment on _callbacks restates what the code already makes clear (a map from
subscription id to { method, handler }), instead of explaining rationale. This adds maintenance
noise and violates the guidance to focus comments on intent/why.

Code

javascript/selenium-webdriver/bidi/index.js[R39-40]

+    // subscriptionId -> { method, handler }, used by addCallback/removeCallback.
+    this._callbacks = new Map()

Evidence

PR Compliance ID 7 requires comments to explain rationale rather than restating obvious behavior.
The added comment on _callbacks simply describes the map structure and usage, which is already
evident from the variable name and surrounding methods (addCallback/removeCallback).

AGENTS.md: Comments should explain why, not what (prefer well-named methods over redundant comments)
javascript/selenium-webdriver/bidi/index.js[39-40]

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 comment added in `bidi/index.js` repeats what the code already conveys (the shape/purpose of `_callbacks`) rather than explaining rationale.

## Issue Context
Compliance requires comments to explain *why*, not *what*.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[39-40]

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

---

8. Once listeners fire repeatedly ✓ Resolved 🐞 Bug ≡ Correctness

Description

The event dispatcher now invokes callbacks returned by listeners() directly instead of using
emit(), bypassing EventEmitter's wrapper that removes a once() listener. Consumers registering
bidi.once(method, handler) will therefore receive every subsequent event rather than only the
first.

Code

javascript/selenium-webdriver/bidi/index.js[R106-108]

+            for (const listener of this.listeners(payload.method)) {
+              try {
+                listener(payload.params)

Evidence

Index inherits from EventEmitter, but the changed path obtains listeners and calls each function
itself; unlike the removed this.emit(payload.method, ...) path, this does not execute
EventEmitter's registered once-wrapper lifecycle.

javascript/selenium-webdriver/bidi/index.js[23-23]
javascript/selenium-webdriver/bidi/index.js[99-113]

Agent prompt

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

## Issue description
Manual listener invocation bypasses inherited one-shot listener removal.

## Issue Context
The dispatcher needs per-listener exception isolation without changing EventEmitter listener semantics.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[99-113]

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

---

View review recommended (1)

9. Null schema types accept anything ✓ Resolved 🐞 Bug ≡ Correctness

Description

validateValue() has no case for the projector's primitive: 'null' nodes, so a non-null value
falls through and is accepted. In addition, projectRef() turns an all-null type into `primitive:
'unknown'`, which follows the same unchecked path; fields and aliases constrained to null therefore
do not validate their wire value.

Code

javascript/selenium-webdriver/bidi/serialization/record.js[R38-59]

+  if (typeNode.primitive !== undefined) {
+    const expected = { string: 'string', integer: 'number', number: 'number', boolean: 'boolean' }[typeNode.primitive]
+    if (expected && typeof value !== expected) {
+      throw new ValidationError(`${path}: expected ${typeNode.primitive}, got ${typeof value}`)
+    }
+    // JSON has no representation for NaN/±Infinity — reject them for both numeric
+    // primitives before the integer-specific check narrows further. (Number.isInteger
+    // already excludes them too, so this is only load-bearing for a bare `number`.)
+    if ((typeNode.primitive === 'integer' || typeNode.primitive === 'number') && !Number.isFinite(value)) {
+      throw new ValidationError(`${path}: expected a finite ${typeNode.primitive}, got ${value}`)
+    }
+    // `number` admits any JSON number; `integer` rejects a fractional value
+    // (5.7) while still accepting one written 5.0 (Number.isInteger(5.0) is true).
+    if (typeNode.primitive === 'integer' && !Number.isInteger(value)) {
+      throw new ValidationError(`${path}: expected an integer, got ${value}`)
+    }
+    // An inline literal choice (project_bidi_schema.mjs's enumNode()) carries both
+    // `primitive` and `enum` — the primitive check above narrows the type, but the
+    // closed vocabulary below still needs to run, so only return early when there
+    // is no `enum` to fall through to.
+    if (typeNode.enum === undefined) return value
+  }

Evidence

The projector explicitly emits null primitive nodes, while the added validator recognizes only
string, integer, number, and boolean and returns after an unrecognized primitive. Its
nullable-reference projection also calls projectEntry(undefined) when every alternative is null,
producing unchecked unknown.

javascript/selenium-webdriver/project_bidi_schema.mjs[89-108]
javascript/selenium-webdriver/project_bidi_schema.mjs[169-194]
javascript/selenium-webdriver/bidi/serialization/record.js[32-59]
javascript/selenium-webdriver/project_bidi_schema.mjs[353-362]

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 serialization runtime must reject every non-null value for a schema node whose projected primitive is `null`. Ensure the projector preserves an all-null reference as a null primitive instead of converting it to unchecked `unknown`.

## Issue Context
The schema projector defines `null`/`nil` as a primitive and uses null primitive nodes in discriminator analysis. The new validator only maps four primitive names, so unrecognized primitive names return the input without validation.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[32-59]
- javascript/selenium-webdriver/project_bidi_schema.mjs[89-108]
- javascript/selenium-webdriver/project_bidi_schema.mjs[169-194]

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

---

Context sources

Review mode: ⚖️ Balanced: This behavioral change alters asynchronous subscription-removal lifecycle and concurrency semantics in a public BiDi API, creating real correctness risk despite being localized to one implementation path.

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 4d7e6dd

Results up to commit 90c486e 🧠 Deep

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

1. Generated events bypass lifecycle ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The generated public domain event methods still call the old event-name-scoped subscribe() and
attach an EventEmitter listener directly; none call the newly added Domain#addCallback().
Consequently generated event users receive no subscription handle and remain subject to the old
API's documented cross-subscription cancellation behavior instead of the lifecycle this PR adds.

Code

javascript/selenium-webdriver/bidi/domain.js[R81-83]

+  async addCallback(descriptor, handler) {
+    const dispatch = descriptor.type === undefined ? handler : (params) => handler(descriptor.type.fromWire(params))
+    return this.#bidi.addCallback(descriptor.method, dispatch)

Evidence

The new connection code explicitly identifies addCallback() as the replacement for the imprecise
legacy API, but the generator still emits only calls to that legacy API and direct on() listeners.
Since generated classes are produced by this template, adding an unused Domain method does not
change their behavior.

javascript/selenium-webdriver/bidi/domain.js[68-84]
javascript/selenium-webdriver/bidi/index.js[274-284]
javascript/selenium-webdriver/generate_bidi.mjs[1007-1034]
javascript/selenium-webdriver/generate_bidi.mjs[1085-1101]

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 new `Domain#addCallback()` is not used by generated domain event methods, so the generated API continues to use the legacy event-name-scoped subscription path and does not expose per-subscription unsubscription.

## Issue Context
`generate_bidi.mjs` is the source for every generated domain class. Its event-method template must construct/use the new event descriptor and delegate through the Domain lifecycle API (or otherwise call `Index#addCallback`) and return the unsubscribe handle with the generated event's typed payload.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/domain.js[81-83]
- javascript/selenium-webdriver/generate_bidi.mjs[1007-1048]
- javascript/selenium-webdriver/generate_bidi.mjs[1085-1101]

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

---

2. Unsubscribe failure loses callback ✓ Resolved 🐞 Bug ≡ Correctness

Description

removeCallback() removes the local subscription entry and event listener before it knows whether
session.unsubscribe succeeded, and it doesn’t inspect the unsubscribe response for protocol
errors. As a result, transport failures, timeouts, or protocol rejections can leave the remote
subscription active while the returned handle can no longer retry because subsequent calls find no
local entry and no-op, falsely indicating cleanup succeeded while events may continue.

Code

javascript/selenium-webdriver/bidi/index.js[R443-453]

+    this._callbacks.delete(subscriptionId)
+    this.off(entry.method, entry.handler)
+
+    if (this._closed) {
+      return
+    }
+
+    await this.send({
+      method: 'session.unsubscribe',
+      params: { subscriptions: [subscriptionId] },
+    })

Evidence

The cited implementation deletes the local callback map entry and listener before awaiting the
protocol unsubscribe, and it also short-circuits when the entry is missing, so once the first
attempt removes local state any later retry from the same handle becomes an immediate no-op without
contacting the browser. Additionally, the shared send()/response-dispatch path resolves pending
requests with the raw response payload (including error-shaped payloads) rather than throwing, and
other call sites like addCallback() (and Domain.send()) explicitly check response.error;
removeCallback() does not perform that validation and effectively discards the unsubscribe
response, so protocol-level unsubscribe rejections can be treated as success even though the remote
subscription remains active.

javascript/selenium-webdriver/bidi/index.js[403-412]
javascript/selenium-webdriver/bidi/index.js[437-453]
javascript/selenium-webdriver/bidi/index.js[245-270]
javascript/selenium-webdriver/bidi/index.js[437-444]
javascript/selenium-webdriver/bidi/index.js[450-453]
javascript/selenium-webdriver/bidi/index.js[118-124]
javascript/selenium-webdriver/bidi/index.js[403-409]

Agent prompt

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

## Issue description
`removeCallback()` currently clears local subscription bookkeeping (map entry and listener) before confirming that `session.unsubscribe` actually succeeded, and it does not validate the unsubscribe response for a protocol-level `error`. This can leave a remote subscription active after a transport failure, timeout, rejected send, or BiDi error response, while making the returned handle unable to retry because subsequent calls see no local entry and return without contacting the browser.

## Issue Context
The request/response plumbing (`Index.send()` and the shared response dispatcher) resolves pending sends with the raw protocol payload, including error responses, instead of throwing. Other code paths (e.g., `addCallback()` and `Domain.send()`) explicitly inspect `response.error` to detect protocol failures, but `removeCallback()` does not check the unsubscribe response at all. Because `removeCallback()` also deletes local state up front and has an early return when no map entry exists, a failed first unsubscribe attempt becomes irrecoverable and later retries become silent no-ops, potentially leaving the browser still producing events.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[437-454]
- javascript/selenium-webdriver/bidi/index.js[245-270]
- javascript/selenium-webdriver/bidi/index.js[403-409]

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

---

3. Legacy unsubscribe cancels callbacks ✗ Dismissed 🐞 Bug ≡ Correctness

Description

addCallback() creates a subscription for an event that the existing name-scoped unsubscribe()
API can cancel remotely; closing a hand-written inspector for the same event therefore silently
stops the new callback while its handle and local listener remain active. This makes the new API
unreliable when mixed with existing BiDi modules on the same connection.

Code

javascript/selenium-webdriver/bidi/index.js[R399-402]

+      const response = await this.send({
+        method: 'session.subscribe',
+        params: { events: [method] },
+      })

Evidence

The new code creates an independent session.subscribe registration, but the existing unsubscribe
implementation sends event names. Existing inspectors invoke that path for the same event methods,
and the added documentation itself states that name-scoped unsubscribe can affect addCallback
subscriptions.

javascript/selenium-webdriver/bidi/index.js[274-284]
javascript/selenium-webdriver/bidi/index.js[328-366]
javascript/selenium-webdriver/bidi/logInspector.js[342-344]
javascript/selenium-webdriver/bidi/network.js[407-415]

Agent prompt

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

## Issue description
Connection-level callbacks can be remotely cancelled by the legacy event-name unsubscribe path.

## Issue Context
Existing inspectors call `Index.unsubscribe()` by event name, while `addCallback()` independently subscribes to the same events by subscription ID.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[289-366]
- javascript/selenium-webdriver/bidi/index.js[395-420]
- javascript/selenium-webdriver/bidi/logInspector.js[342-344]
- javascript/selenium-webdriver/bidi/network.js[407-415]

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

---

View action required (1)

4. Nested union defaults crash ✓ Resolved 🐞 Bug ≡ Correctness

Description

defineUnion().fromWire() assumes every selected ref is a record and calls
variant.RecordClass.fromWire(), but generated discriminated selectors may use another union as
their default arm. Valid payloads selecting that arm therefore throw a TypeError instead of being
parsed.

Code

javascript/selenium-webdriver/bidi/serialization/union.js[R83-84]

+      const variant = resolve(ref)
+      return variant.RecordClass.fromWire(payload)

Evidence

The projector documents and emits union refs in selector.default; union registry entries expose
build/fromWire, not RecordClass, while both runtime paths unconditionally dereference
RecordClass.

javascript/selenium-webdriver/project_bidi_schema.mjs[392-421]
javascript/selenium-webdriver/bidi/serialization/union.js[55-56]
javascript/selenium-webdriver/bidi/serialization/union.js[68-84]

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 selected union variant may itself be a union, but the serializer always treats it as a record.

## Issue Context
The schema projector explicitly emits nested unions as discriminated-selector defaults.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/union.js[68-84]
- javascript/selenium-webdriver/project_bidi_schema.mjs[392-421]

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

---

5. Null schema types accept anything ✓ Resolved 🐞 Bug ≡ Correctness

Description

validateValue() has no case for the projector's primitive: 'null' nodes, so a non-null value
falls through and is accepted. In addition, projectRef() turns an all-null type into `primitive:
'unknown'`, which follows the same unchecked path; fields and aliases constrained to null therefore
do not validate their wire value.

Code

javascript/selenium-webdriver/bidi/serialization/record.js[R38-59]

+  if (typeNode.primitive !== undefined) {
+    const expected = { string: 'string', integer: 'number', number: 'number', boolean: 'boolean' }[typeNode.primitive]
+    if (expected && typeof value !== expected) {
+      throw new ValidationError(`${path}: expected ${typeNode.primitive}, got ${typeof value}`)
+    }
+    // JSON has no representation for NaN/±Infinity — reject them for both numeric
+    // primitives before the integer-specific check narrows further. (Number.isInteger
+    // already excludes them too, so this is only load-bearing for a bare `number`.)
+    if ((typeNode.primitive === 'integer' || typeNode.primitive === 'number') && !Number.isFinite(value)) {
+      throw new ValidationError(`${path}: expected a finite ${typeNode.primitive}, got ${value}`)
+    }
+    // `number` admits any JSON number; `integer` rejects a fractional value
+    // (5.7) while still accepting one written 5.0 (Number.isInteger(5.0) is true).
+    if (typeNode.primitive === 'integer' && !Number.isInteger(value)) {
+      throw new ValidationError(`${path}: expected an integer, got ${value}`)
+    }
+    // An inline literal choice (project_bidi_schema.mjs's enumNode()) carries both
+    // `primitive` and `enum` — the primitive check above narrows the type, but the
+    // closed vocabulary below still needs to run, so only return early when there
+    // is no `enum` to fall through to.
+    if (typeNode.enum === undefined) return value
+  }

Evidence

The projector explicitly emits null primitive nodes, while the added validator recognizes only
string, integer, number, and boolean and returns after an unrecognized primitive. Its
nullable-reference projection also calls projectEntry(undefined) when every alternative is null,
producing unchecked unknown.

javascript/selenium-webdriver/project_bidi_schema.mjs[89-108]
javascript/selenium-webdriver/project_bidi_schema.mjs[169-194]
javascript/selenium-webdriver/bidi/serialization/record.js[32-59]
javascript/selenium-webdriver/project_bidi_schema.mjs[353-362]

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 serialization runtime must reject every non-null value for a schema node whose projected primitive is `null`. Ensure the projector preserves an all-null reference as a null primitive instead of converting it to unchecked `unknown`.

## Issue Context
The schema projector defines `null`/`nil` as a primitive and uses null primitive nodes in discriminator analysis. The new validator only maps four primitive names, so unrecognized primitive names return the input without validation.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[32-59]
- javascript/selenium-webdriver/project_bidi_schema.mjs[89-108]
- javascript/selenium-webdriver/project_bidi_schema.mjs[169-194]

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

---

6. Declarations omitted from package ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

The production package glob adds only bidi/serialization/*.js, so the new serialization .d.ts
files—and likewise the new domain.d.ts—are absent from the published npm artifact. TypeScript
consumers of these new public modules consequently receive no declarations despite the PR defining
them.

Code

javascript/selenium-webdriver/BUILD.bazel[153]

+        "bidi/serialization/*.js",

Evidence

prod-src-files is included in the publishable npm target and selects only .js files under the
new paths; the repository's only declaration files are the four introduced here, and package.json
provides no alternate declaration entry.

javascript/selenium-webdriver/BUILD.bazel[133-180]
javascript/selenium-webdriver/package.json[17-17]
javascript/selenium-webdriver/bidi/domain.d.ts[18-55]
javascript/selenium-webdriver/bidi/serialization/record.d.ts[18-70]

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 npm packaging source set excludes the newly added declaration files.

## Issue Context
The package is built from `prod-src-files`; its globs select JavaScript only.

## Fix Focus Areas
- javascript/selenium-webdriver/BUILD.bazel[133-180]
- javascript/selenium-webdriver/bidi/domain.d.ts[18-55]
- javascript/selenium-webdriver/bidi/serialization/record.d.ts[18-70]
- javascript/selenium-webdriver/bidi/serialization/union.d.ts[18-36]
- javascript/selenium-webdriver/bidi/serialization/enum.d.ts[18-30]

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

---

7. Once listeners fire repeatedly ✓ Resolved 🐞 Bug ≡ Correctness

Description

The event dispatcher now invokes callbacks returned by listeners() directly instead of using
emit(), bypassing EventEmitter's wrapper that removes a once() listener. Consumers registering
bidi.once(method, handler) will therefore receive every subsequent event rather than only the
first.

Code

javascript/selenium-webdriver/bidi/index.js[R106-108]

+            for (const listener of this.listeners(payload.method)) {
+              try {
+                listener(payload.params)

Evidence

Index inherits from EventEmitter, but the changed path obtains listeners and calls each function
itself; unlike the removed this.emit(payload.method, ...) path, this does not execute
EventEmitter's registered once-wrapper lifecycle.

javascript/selenium-webdriver/bidi/index.js[23-23]
javascript/selenium-webdriver/bidi/index.js[99-113]

Agent prompt

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

## Issue description
Manual listener invocation bypasses inherited one-shot listener removal.

## Issue Context
The dispatcher needs per-listener exception isolation without changing EventEmitter listener semantics.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[99-113]

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

---

View review recommended (1)

8. Redundant _callbacks map comment ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The inline comment on _callbacks restates what the code already makes clear (a map from
subscription id to { method, handler }), instead of explaining rationale. This adds maintenance
noise and violates the guidance to focus comments on intent/why.

Code

javascript/selenium-webdriver/bidi/index.js[R39-40]

+    // subscriptionId -> { method, handler }, used by addCallback/removeCallback.
+    this._callbacks = new Map()

Evidence

PR Compliance ID 7 requires comments to explain rationale rather than restating obvious behavior.
The added comment on _callbacks simply describes the map structure and usage, which is already
evident from the variable name and surrounding methods (addCallback/removeCallback).

AGENTS.md: Comments should explain why, not what (prefer well-named methods over redundant comments)
javascript/selenium-webdriver/bidi/index.js[39-40]

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 comment added in `bidi/index.js` repeats what the code already conveys (the shape/purpose of `_callbacks`) rather than explaining rationale.

## Issue Context
Compliance requires comments to explain *why*, not *what*.

## Fix Focus Areas
- javascript/selenium-webdriver/bidi/index.js[39-40]

ⓘ 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 8a7b11a

[qodo-code-review]

Code review by qodo was updated up to the latest commit ff45d8a

[qodo-code-review]

No code changes since the last review — review skipped