[js] Add serialization and domain layer - #17927
Open
pujagani wants to merge 1 commit into
Open
Conversation
selenium-ci
added
C-nodejs
Contributor
PR Summary by QodoAdd JavaScript BiDi domain and serialization foundation
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
Contributor
Code Review by Qodo
1. Callback transport methods missing
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+37
to
+39
| * @returns {{method: string, type: ({fromWire(payload: unknown): unknown}|undefined)}} | ||
| */ | ||
| function event(method, type) { |
Contributor
There was a problem hiding this comment.
1. event jsdoc lacks description 📘 Rule violation ✧ Quality
The exported event function's JSDoc contains no free-text summary, and its @returns tag has no description. This leaves the public API documentation incomplete.
Agent Prompt
## Issue description
Complete the JSDoc for the exported `event` function with a summary and a descriptive `@returns` tag.
## Issue Context
PR Compliance 389257 requires every exported function to have a complete JSDoc block immediately before its declaration.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/domain.js[29-39]
- javascript/selenium-webdriver/bidi/domain.d.ts[23-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return response?.result | ||
| } | ||
|
|
||
| async addCallback(descriptor, handler) { |
Contributor
There was a problem hiding this comment.
2. domain callbacks lack jsdoc 📘 Rule violation ✧ Quality
The exported Domain class adds public addCallback and removeCallback methods without immediately preceding JSDoc blocks. Their parameters and non-void promise return values are therefore undocumented.
Agent Prompt
## Issue description
Add complete JSDoc blocks for `Domain.addCallback` and `Domain.removeCallback`, including summaries, typed parameters, and described return values.
## Issue Context
`Domain` is exported through `module.exports`, and both methods are public in its TypeScript declaration.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/domain.js[66-73]
- javascript/selenium-webdriver/bidi/domain.d.ts[32-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
+24
| /** | ||
| * @param {string} name Schema type name, e.g. 'network.InterceptPhase'. | ||
| * @param {string[]} values | ||
| */ | ||
| function defineEnum(name, values) { |
Contributor
There was a problem hiding this comment.
3. defineenum jsdoc incomplete 📘 Rule violation ✧ Quality
The exported defineEnum function's JSDoc has no free-text description and omits documentation for its non-void return value. Consumers are not told that the function returns the registered enum entry.
Agent Prompt
## Issue description
Add a summary and a descriptive typed `@returns` tag to the `defineEnum` JSDoc block.
## Issue Context
The function is exported from the module and returns `entry`, so complete return documentation is required.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/enum.js[20-24]
- javascript/selenium-webdriver/bidi/serialization/enum.d.ts[23-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+142
to
+147
| /** | ||
| * @param {string} name Schema type name, e.g. 'network.AddInterceptParameters'. | ||
| * @param {Array<{name: string, wire: string, required: boolean, type: object}>} fields | ||
| * @param {{extensible?: boolean}} [options] | ||
| */ | ||
| function defineRecord(name, fields, options = {}) { |
Contributor
There was a problem hiding this comment.
4. definerecord jsdoc incomplete 📘 Rule violation ✧ Quality
The exported defineRecord function's JSDoc lacks a free-text description and a typed, described @returns tag. The generated record class returned by this public factory is undocumented.
Agent Prompt
## Issue description
Add a concise summary and a typed, descriptive `@returns` tag to `defineRecord`'s JSDoc.
## Issue Context
`defineRecord` is exported and returns the generated `Record` class at the end of the function.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[142-147]
- javascript/selenium-webdriver/bidi/serialization/record.d.ts[56-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+42
to
+47
| /** | ||
| * @param {string} name Schema type name, e.g. 'session.ProxyConfiguration'. | ||
| * @param {object} selector The schema's `selector` node for this union. | ||
| * @param {{objectOnly?: boolean}} [options] | ||
| */ | ||
| function defineUnion(name, selector, options = {}) { |
Contributor
There was a problem hiding this comment.
5. defineunion jsdoc incomplete 📘 Rule violation ✧ Quality
The exported defineUnion function's JSDoc lacks a free-text description and documentation for its non-void return value. The returned union API is therefore not completely documented.
Agent Prompt
## Issue description
Add a concise summary and a typed, descriptive `@returns` tag to the `defineUnion` JSDoc block.
## Issue Context
The exported factory returns `union`, whose public declaration is `UnionClass<T>`.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/union.js[42-47]
- javascript/selenium-webdriver/bidi/serialization/union.d.ts[27-27]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+128
to
+140
| if (typeNode.union !== undefined) { | ||
| const errors = [] | ||
| for (const variant of typeNode.union) { | ||
| try { | ||
| validateValue(variant, value, path, direction) | ||
| return | ||
| } catch (err) { | ||
| errors.push(err.message) | ||
| } | ||
| } | ||
| throw new ValidationError(`${path}: value did not match any variant (${errors.join('; ')})`) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
9. Inline records skip validation 🐞 Bug ≡ Correctness
The schema projector emits inline field types as {record: [...]}, but validateValue() has no
record branch and silently accepts them. Missing required members, invalid member types, and
undeclared members in inline records consequently pass both inbound and outbound validation.
Agent Prompt
## Issue description
Inline record TypeNodes emitted by the schema projector fall through `validateValue()` without any validation.
## Issue Context
Inline records contain projected `FieldSpec` entries and need the same directional required-field, extra-field, and nested-value handling as named records. Add the missing TypeScript representation as well.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[28-140]
- javascript/selenium-webdriver/bidi/serialization/record.d.ts[19-36]
- javascript/selenium-webdriver/project_bidi_schema.mjs[195-204]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+103
to
+106
| if (direction === 'inbound') { | ||
| referenced.RecordClass.fromWire(value) | ||
| } else { | ||
| new referenced.RecordClass(value) |
Contributor
There was a problem hiding this comment.
10. Nested parsing result discarded 🐞 Bug ≡ Correctness
Nested record and union references are parsed only for validation, after which fromWire() assigns the original raw object to the parent. Nested fields therefore remain plain mutable objects and retain undeclared properties that their nested fromWire() call had dropped.
Agent Prompt
## Issue description
Nested record and union parsing results are discarded, leaving raw wire objects in otherwise typed parent records.
## Issue Context
`validateValue()` currently returns no transformed value. Refactor validation/parsing so inbound nested refs return and assign the `fromWire()` result, while preserving outbound behavior.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[96-117]
- javascript/selenium-webdriver/bidi/serialization/record.js[203-214]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+165
to
+168
| const value = data[field.wire] | ||
| validateValue(field.type, value, `${name}.${field.wire}`, 'outbound') | ||
| this[field.name] = value | ||
| } |
Contributor
There was a problem hiding this comment.
11. Validated collections remain mutable 🐞 Bug ☼ Reliability
Record construction stores caller-owned arrays and objects directly and only freezes the outer instance. A caller can mutate a validated list or map afterward, including inserting schema-invalid values that are then serialized to the wire.
Agent Prompt
## Issue description
Validated arrays, maps, and nested objects remain mutable after a record is frozen, allowing post-validation corruption.
## Issue Context
The outer `Object.freeze()` does not freeze or copy values assigned from input data. Store immutable validated copies or deeply freeze the supported value graph.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[65-80]
- javascript/selenium-webdriver/bidi/serialization/record.js[158-187]
- javascript/selenium-webdriver/bidi/serialization/record.js[203-237]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+165
to
+167
| const value = data[field.wire] | ||
| validateValue(field.type, value, `${name}.${field.wire}`, 'outbound') | ||
| this[field.name] = value |
Contributor
There was a problem hiding this comment.
12. Wire names serialize incorrectly 🐞 Bug ≡ Correctness
The constructor reads each value using field.wire but stores it under field.name, and no wire-key conversion occurs before the transport calls JSON.stringify(). Whenever name !== wire, the command sends the JS-facing name instead of the protocol key.
Agent Prompt
## Issue description
Outbound records expose JS property names directly to JSON serialization instead of mapping them back to their declared wire names.
## Issue Context
The wire contract test already defines a record where `name` and `wire` differ, but only checks inbound parsing. Add an explicit outbound representation method and cover this fixture through JSON serialization.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[147-188]
- javascript/selenium-webdriver/bidi/domain.js[58-63]
- javascript/selenium-webdriver/test/bidi/serialization/wire_contract_test.js[100-114]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+34
to
+42
| 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}`) | ||
| } | ||
| // `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}`) |
Contributor
There was a problem hiding this comment.
13. Nonfinite numbers pass validation 🐞 Bug ≡ Correctness
A number node accepts NaN and both infinities because validation only checks `typeof value === 'number'. When the transport JSON-stringifies these values they become null`, silently sending a value different from the validated record.
Agent Prompt
## Issue description
Primitive number validation accepts nonfinite JavaScript numbers that cannot be represented as BiDi JSON numbers.
## Issue Context
Require `Number.isFinite()` for numeric primitives in addition to the existing integer-specific check, and test both inbound and outbound paths.
## Fix Focus Areas
- javascript/selenium-webdriver/bidi/serialization/record.js[34-44]
- javascript/selenium-webdriver/test/bidi/serialization/record_test.js[127-146]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Add Domain and Serialization class, foundation for CDDL generation.
💥 What does this PR do?
Introduces Domain base class (modules will be built on top of this for generator) and bunch of serialization base classes (
defineRecord/defineEnum/defineUnion etc) in alignment with low-level behavioral ADR.
🔧 Implementation Notes
🤖 AI assistance
💡 Additional Considerations
🔄 Types of changes