Data tracks - Packet serialization + deserialialization, plus some error handling changes #1792
Conversation
For some reason typescript seems to not like accessing a static member on this in an abstract class - it should be possible but whatever. This is simple enough where I'm just going to relocate the implementation to the downstream classes.
The error wrapping as part of InvalidHandle is fairly nuanced and is probably the part of this I am most unsure about
🦋 Changeset detectedLatest commit: 95d52f4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a Throws-branded type system and throws-transformer toolchain (engine/cli/plugin/transformer/README) integrated into tsconfig and CI; implements a DataTrack packet (de)serialization stack (utils, handle/allocator, constants, extensions, errors, serializable, packet) with extensive tests and public re-exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Developer Editor
participant LS as TS Language Service
participant Plugin as throws-transformer Plugin
participant Engine as throws-transformer Engine
participant CLI as throws-transformer CLI
participant CI as CI Runner
Editor->>LS: request diagnostics for file
LS->>Plugin: getSemanticDiagnostics(sourceFile)
Plugin->>Engine: checkSourceFile(sourceFile, checker)
Engine->>Engine: analyze AST & resolve Throws<> types
Engine-->>Plugin: return CheckResult[] (diagnostics)
Plugin->>LS: append diagnostics (code 90001)
LS->>Editor: surface augmented diagnostics
CLI->>Engine: run checkSourceFile over file globs
Engine-->>CLI: return results
CLI->>CI: print results and set exit code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-01-28T02:22:46.510ZApplied to files:
🧬 Code graph analysis (1)src/room/data-track/packet/extensions.ts (5)
🪛 Biome (2.1.2)src/room/data-track/packet/extensions.ts[error] 224-224: Other switch clauses can erroneously access this declaration. The declaration is defined in this switch clause: Safe fix: Wrap the declaration in a block. (lint/correctness/noSwitchDeclarations) [error] 226-226: Other switch clauses can erroneously access this declaration. The declaration is defined in this switch clause: Safe fix: Wrap the declaration in a block. (lint/correctness/noSwitchDeclarations) 🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting Comment |
|
Just as additional context for other reviewers, the Rust implementation lives in this module. |
| @@ -0,0 +1,25 @@ | |||
| /** An abstract class implementing common behavior related to data track binary serialization. */ | |||
| export default abstract class Serializable { | |||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/room/data-track/packet/index.test.ts`:
- Around line 292-305: The test's expected payload omits the leading 0 byte
present in the actual deserialized/round‑trip data, which hides an offset bug;
update the assertion in the test that calls packet.toJSON() (and the similar
assertion around lines 539-552) to include the leading 0 in the expected payload
Uint8Array (i.e., make the payload new
Uint8Array([0,1,2,3,4,5,6,7,8,9]).buffer), or if the packet is meant to include
an extension-word byte instead, adjust the packet construction/header flags (the
fields used when creating the Packet object and its header such as frameNumber,
marker (FrameMarker.Single), sequence, timestamp, trackHandle, and any extension
flags) so the serialized bytes correctly reflect the absence of that 0; prefer
the first option unless you also change header/flags to explicitly remove the
extension-word.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/room/data-track/packet/index.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1792
File: tsconfig.json:29-29
Timestamp: 2026-01-23T20:04:34.181Z
Learning: In the livekit/client-sdk-js repository, the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path `"./throws-transformer/plugin.ts"` rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions.
🪛 ESLint
src/room/data-track/packet/index.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
🔇 Additional comments (1)
src/room/data-track/packet/index.test.ts (1)
2-2: Configure ESLint to handle test file imports for devDependencies.The
vitestimport triggersimport-x/no-unresolvedbecause the ESLint config has no override for test files to allow devDependency imports. Add a test file pattern override toeslint.config.mjsthat excludesimport-x/no-extraneous-dependenciesfor.test.tsand.test.jsfiles, or configure the import-x resolver to recognize devDependencies in test contexts.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/room/data-track/packet/index.test.ts`:
- Around line 31-33: The comment on the header byte 0x18 in the test arrays is
incorrect: 0x18 represents "Single" with no extensions, not "final, extension";
update the inline comment(s) that currently read things like "// Version 0,
final, extension" to accurately state "// Single, no extensions" (or similar)
wherever the 0x18 header byte is used in
src/room/data-track/packet/index.test.ts so the comment matches the encoded
marker/extension bits for the Uint8Array test fixtures.
- Line 2: The TypeScript config is missing "vitest" in the compiler "types"
list, causing import resolution failures for test globals like the vitest
imports (afterEach, beforeEach, describe, it, vi) in tests such as
index.test.ts; open your tsconfig.json and add "vitest" to the "types" array
(e.g., include "sdp-transform", "ua-parser-js", "events",
"dom-mediacapture-record", "vitest") so the compiler and eslint import-x
resolver recognize vitest types.
🧹 Nitpick comments (2)
src/room/data-track/packet/index.ts (2)
81-152: Add a capacity guard before writing the header.Right now an undersized
DataViewwill throw aRangeErrormid-write; an explicit length check makes failures deterministic and easier to debug.♻️ Proposed fix
toBinaryInto(dataView: DataView): number { + const totalLengthBytes = this.toBinaryLengthBytes(); + if (dataView.byteLength < totalLengthBytes) { + throw new Error( + `DataTrackPacketHeader.toBinaryInto: DataView too small (${dataView.byteLength} bytes) for ${totalLengthBytes} bytes`, + ); + } + let initial = SUPPORTED_VERSION << VERSION_SHIFT; @@ - const totalLengthBytes = this.toBinaryLengthBytes(); if (byteIndex !== totalLengthBytes) { throw new Error( `DataTrackPacketHeader.toBinaryInto: Wrote ${byteIndex} bytes but expected length was ${totalLengthBytes} bytes`, ); }
304-313: Prefer a bulk copy for payload serialization.Writing payload byte-by-byte is correct but slower and noisier than a single
Uint8Array#set.♻️ Proposed fix
- const payloadBytes = new Uint8Array(this.payload); - for (let index = 0; index < payloadBytes.length; index += 1) { - dataView.setUint8(byteIndex, payloadBytes[index]); - byteIndex += U8_LENGTH_BYTES; - } + const payloadBytes = new Uint8Array(this.payload); + new Uint8Array( + dataView.buffer, + dataView.byteOffset + byteIndex, + payloadBytes.length, + ).set(payloadBytes); + byteIndex += payloadBytes.length;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/room/data-track/packet/index.test.tssrc/room/data-track/packet/index.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1792
File: tsconfig.json:29-29
Timestamp: 2026-01-23T20:04:34.181Z
Learning: In the livekit/client-sdk-js repository, the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path `"./throws-transformer/plugin.ts"` rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions.
🧬 Code graph analysis (2)
src/room/data-track/packet/index.ts (5)
src/room/data-track/handle.ts (2)
DataTrackHandle(44-66)DataTrackHandleError(10-42)src/room/data-track/utils.ts (4)
WrapAroundUnsignedInt(9-51)U16_MAX_SIZE(1-1)DataTrackTimestamp(53-70)coerceToDataView(72-86)src/room/data-track/packet/extensions.ts (1)
DataTrackExtensions(131-253)src/room/data-track/packet/constants.ts (16)
BASE_HEADER_LEN(9-9)EXT_WORDS_INDICATOR_SIZE(23-23)SUPPORTED_VERSION(8-8)VERSION_SHIFT(12-12)FRAME_MARKER_INTER(20-20)FRAME_MARKER_FINAL(19-19)FRAME_MARKER_START(18-18)FRAME_MARKER_SINGLE(21-21)FRAME_MARKER_SHIFT(15-15)EXT_FLAG_SHIFT(24-24)U8_LENGTH_BYTES(2-2)U16_LENGTH_BYTES(3-3)U32_LENGTH_BYTES(4-4)VERSION_MASK(13-13)FRAME_MARKER_MASK(16-16)EXT_FLAG_MASK(25-25)src/room/data-track/packet/errors.ts (2)
DataTrackDeserializeErrorAll(76-82)DataTrackDeserializeError(13-74)
src/room/data-track/packet/index.test.ts (6)
src/room/data-track/packet/index.ts (2)
DataTrackPacketHeader(32-274)DataTrackPacket(289-343)src/index.ts (5)
DataTrackPacketHeader(152-152)DataTrackPacket(152-152)DataTrackExtensions(154-154)DataTrackUserTimestampExtension(155-155)DataTrackE2eeExtension(156-156)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(9-51)DataTrackTimestamp(53-70)src/room/data-track/packet/extensions.ts (3)
DataTrackExtensions(131-253)DataTrackUserTimestampExtension(18-68)DataTrackE2eeExtension(70-129)src/room/data-track/packet/constants.ts (1)
EXT_FLAG_SHIFT(24-24)
🪛 ESLint
src/room/data-track/packet/index.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/room/data-track/packet/index.ts (3)
62-79: Header length + padding math looks solid.The
extensionsMetrics()andtoBinaryLengthBytes()logic is clear and consistent with the RTP-style padding rules.
155-262: Deserialization path is clean and well-defended.Good use of version/length checks and error wrapping for invalid handles.
325-338: Payload slicing respects DataView offsets.The buffer slice uses
byteOffsetcorrectly, which is critical for sliced views.src/room/data-track/packet/index.test.ts (2)
259-519: Deserialization coverage is thorough and well-scoped.Nice spread of negative tests (too short, missing ext words, overrun, unsupported version) and extension parsing.
521-580: Round-trip test adds strong confidence.Good to see encode/decode symmetry validated on a representative packet.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@throws-transformer/README.md`:
- Around line 34-35: Fix the typo in the README: change "Maually" to "Manually"
in the sentence mentioning the Throws<T, E> type so it reads "Manually throwing
of `Error`s is left open as an escape hatch for implementing `panic!()`-like
behavior." Update the line that references the Throws<T, E> type accordingly.
- Around line 87-99: The README example uses the wrong transformer path; update
the JSON snippet so the "transform" value references the actual file
./throws-transformer/transformer.ts (remove the duplicate transformer/ segment)
so the plugin entry { "name": "throws-transformer/transformer", "transform":
"<correct path>" } points to ./throws-transformer/transformer.ts; ensure the
other plugin entry { "name": "./throws-transformer/plugin.ts" } remains
unchanged.
♻️ Duplicate comments (2)
throws-transformer/engine.ts (1)
648-670: Inverted logic in then-block handling.The
thenRethrowscondition collects narrowed types when the then-block throws, but the intention is to collect types that are handled (don't throw). Compare with the else-branch at lines 677-679 which correctly uses!containsThrowStatement().Proposed fix
- const thenRethrows = ts.isBlock(thenStatement) + const thenHandles = ts.isBlock(thenStatement) + ? !containsThrowStatement(thenStatement) + : !ts.isThrowStatement(thenStatement); + + if (thenHandles) { + // Get narrowed type in the then-block + const narrowedType = getNarrowedTypeInBlock( + thenStatement, + errorVarName, + checker, + ); - ? containsThrowStatement(thenStatement) - : ts.isThrowStatement(thenStatement); - - if (thenRethrows) { - // Get narrowed type in the then-block - const narrowedType = getNarrowedTypeInBlock( - thenStatement, - errorVarName, - checker, - );throws-transformer/README.md (1)
136-139: Add a language identifier to the fenced code block.The diagnostic output block is missing a language tag (MD040).
Suggested fix
-``` +```text Unhandled error(s) from 'fetchUser': NetworkError | NotFoundError. Catch these errors or add 'Throws<..., NetworkError | NotFoundError>' to your function's return type. ```
🧹 Nitpick comments (2)
throws-transformer/engine.ts (1)
1-8: File header appears to be copy-pasted from CLI.The shebang (
#!/usr/bin/env tsx) and doc comment describe a "CLI tool" with usage examples referencingsrc/cli.ts, but this isengine.ts— a library module, not an executable. Consider updating the header to describe the engine's purpose.Suggested header update
-#!/usr/bin/env tsx -/** - * CLI tool to check TypeScript files for unhandled Throws errors. - * - * Usage: - * npx ts-node src/cli.ts src/examples.ts - * npx ts-node src/cli.ts "src/*.ts" - */ +/** + * Core engine for checking TypeScript files for unhandled Throws<> errors. + * Used by the CLI, language service plugin, and transformer. + */src/room/data-track/packet/serializable.ts (1)
12-27: Resolve the FIXME to lock the return type.Please decide between
Uint8ArrayvsArrayBufferand update the method contract + comment accordingly, so the API surface is unambiguous.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/room/data-track/packet/errors.tssrc/room/data-track/packet/index.test.tssrc/room/data-track/packet/index.tssrc/room/data-track/packet/serializable.tsthrows-transformer/README.mdthrows-transformer/engine.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1792
File: tsconfig.json:29-29
Timestamp: 2026-01-23T20:04:34.181Z
Learning: In the livekit/client-sdk-js repository, the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path `"./throws-transformer/plugin.ts"` rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions.
📚 Learning: 2026-01-23T20:04:34.181Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1792
File: tsconfig.json:29-29
Timestamp: 2026-01-23T20:04:34.181Z
Learning: In the livekit/client-sdk-js repository, the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path `"./throws-transformer/plugin.ts"` rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions.
Applied to files:
throws-transformer/engine.tsthrows-transformer/README.md
🧬 Code graph analysis (4)
src/room/data-track/packet/serializable.ts (2)
src/utils/throws.ts (1)
Throws(12-12)src/room/data-track/packet/errors.ts (1)
DataTrackSerializeErrorAll(119-121)
src/room/data-track/packet/index.test.ts (6)
src/room/data-track/packet/index.ts (2)
DataTrackPacketHeader(38-286)DataTrackPacket(301-359)src/index.ts (5)
DataTrackPacketHeader(152-152)DataTrackPacket(152-152)DataTrackExtensions(154-154)DataTrackUserTimestampExtension(155-155)DataTrackE2eeExtension(156-156)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(9-51)DataTrackTimestamp(53-70)src/room/data-track/packet/extensions.ts (3)
DataTrackExtensions(131-253)DataTrackUserTimestampExtension(18-68)DataTrackE2eeExtension(70-129)src/room/data-track/packet/constants.ts (1)
EXT_FLAG_SHIFT(24-24)
src/room/data-track/packet/index.ts (4)
src/room/data-track/handle.ts (2)
DataTrackHandle(44-66)DataTrackHandleError(10-42)src/room/data-track/utils.ts (4)
WrapAroundUnsignedInt(9-51)U16_MAX_SIZE(1-1)DataTrackTimestamp(53-70)coerceToDataView(72-86)src/room/data-track/packet/extensions.ts (1)
DataTrackExtensions(131-253)src/room/data-track/packet/errors.ts (4)
DataTrackSerializeError(89-117)DataTrackDeserializeErrorAll(76-82)DataTrackDeserializeError(13-74)DataTrackSerializeErrorAll(119-121)
src/room/data-track/packet/errors.ts (1)
src/room/data-track/handle.ts (1)
DataTrackHandleError(10-42)
🪛 ESLint
src/room/data-track/packet/index.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
🪛 LanguageTool
throws-transformer/README.md
[grammar] ~34-~34: Ensure spelling is correct
Context: ... is meant to handle Error subclasses. Maually throwing of Errors is left open as an...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
throws-transformer/README.md
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (16)
throws-transformer/engine.ts (7)
30-80: LGTM!The
checkSourceFilefunction provides a clean entry point with a well-structured recursive AST traversal that correctly handles different node types (calls, awaits, throws, returns).
82-135: LGTM!The
checkThrowStatementfunction correctly validates thrown types againstThrows<>declarations with appropriate early exits for non-applicable cases.
137-196: LGTM!The
checkPromiseRejectimplementation correctly validates rejected types against declared errors. The known limitation around.catch()handler detection was previously discussed and acknowledged as future work.
198-256: LGTM!The return statement checker properly validates error propagation and provides clear diagnostic messages.
258-425: LGTM!The helper functions for extracting and comparing error types are well-structured with appropriate null checks and early returns.
427-491: LGTM!Good design decision to allow
Erroras an escape hatch for panic-like behavior. The base type resolution correctly handles class inheritance.
723-841: LGTM!The remaining utility functions are well-implemented with appropriate boundary checks and fallback handling.
throws-transformer/README.md (1)
1-271: Good documentation structure.The README provides clear setup instructions, usage patterns, and appropriately documents known limitations. The examples align well with the engine implementation.
src/room/data-track/packet/errors.ts (2)
4-82: Clear deserialize error taxonomy and factories.Enums, reason names, and static constructors are consistent and easy to consume.
84-121: Serialize error definitions look consistent.The enum + factory pattern mirrors the deserialize side cleanly.
src/room/data-track/packet/index.test.ts (3)
14-289: Good serialization coverage for header + payload layouts.Covers multiple marker/extension combinations plus size‑error paths.
292-552: Deserialization tests look comprehensive.Nice coverage of error conditions and extension parsing variants.
554-613: Round‑trip test provides solid regression coverage.src/room/data-track/packet/index.ts (3)
37-165: Header serialization path looks solid.Length computation, extension sizing, and capacity checks are well-structured.
167-274: Header deserialization and error mapping look correct.Clear version/flag handling and clean wrapping of handle errors.
300-358: Packet serialization/deserialization composition is clean.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…ore syntax I think this is a bit more flexible and allows an explicit reason to be given inline.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/room/data-track/packet/extensions.ts`:
- Line 94: The generic type parameter list on the toBinaryInto declaration is
using an unnecessary second type argument—change the signature of toBinaryInto
(currently declared as Throws<number, never>) to use only the single required
type parameter (Throws<number>) to remove the redundant `never` and resolve the
CI failure; update the declaration in the toBinaryInto function within
extensions.ts (same pattern as the fix at the earlier occurrence around line 38)
so only the needed type parameter remains.
- Line 38: The method signature for toBinaryInto currently uses the redundant
Throws<number, never> type; remove the explicit never type so it uses the
default error type (e.g. change Throws<number, never> to Throws<number> or
simply Throws<number>) in the toBinaryInto declaration to satisfy CI and keep
types correct; update the toBinaryInto method signature accordingly.
- Line 157: The Throws generic on toBinaryInto is declared with an unnecessary
second type parameter; update the signature from toBinaryInto(dataView:
DataView): Throws<number, never> to remove the redundant `never` (e.g.
toBinaryInto(dataView: DataView): Throws<number>) and apply the same fix to the
other occurrences noted (the other Throws usages referenced in this file) so all
Throws declarations use the correct arity.
In `@throws-transformer/README.md`:
- Around line 124-128: Replace the incorrect CLI examples "npx throws-check
src/myfile.ts" and "npx throws-check src/*.ts" in the README with the actual
available commands: show "npm run throws:check" (or "pnpm throws:check") for the
default check and "npx tsx ./throws-transformer/cli.ts src/myfile.ts" for
checking specific files; update the text around the examples so it references
these exact commands and notes that there is no bin entry for throws-check.
- Around line 253-258: The "Built-in Error Classes" section falsely claims
exports that don't exist; either implement and export concrete error classes
named NetworkError, NotFoundError, ValidationError, and ParseError from the
package (add definitions and export them from the module entry) or update the
README text to remove the claim and clearly state that NetworkError and
NotFoundError are only example snippets and that users must define their own
error types (and remove ValidationError/ParseError or mark them as
placeholders). Ensure references to NetworkError, NotFoundError,
ValidationError, and ParseError in the README match the actual exported symbols
(or are explicitly labeled as examples) so the docs and codebase stay in sync.
♻️ Duplicate comments (4)
src/room/data-track/packet/index.test.ts (2)
1-12: LGTM!Test imports are well-organized, covering all necessary types from the packet module and test utilities from vitest.
The ESLint
import-x/no-unresolvederror for vitest is a configuration issue — vitest needs to be added totsconfig.jsontypes array as noted in a previous review.
31-32: Fix misleading comment:0x18has no extension flag set.
0x18=0b00011000encodes Version 0, Single marker, with extension flag = 0. The comment says "extension" which is misleading since there are no extensions in this packet.🔧 Suggested fix
- 0x18, // Version 0, single, extension + 0x18, // Version 0, single, no extensionAlso applies to lines 298 and 571.
throws-transformer/README.md (1)
152-155: Add a language identifier to the fenced code block.The diagnostic output block is missing a language tag, which violates markdown linting rules (MD040).
✏️ Proposed fix
-``` +```text Unhandled error(s) from 'fetchUser': NetworkError | NotFoundError. Catch these errors or add 'Throws<..., NetworkError | NotFoundError>' to your function's return type.</details> </blockquote></details> <details> <summary>throws-transformer/engine.ts (1)</summary><blockquote> `662-684`: **Invert then-branch handling check to avoid misclassifying rethrows.** Line 662-684 currently collects narrowed types when the branch rethrows, which treats rethrown errors as handled and can suppress real diagnostics. <details> <summary>🛠️ Proposed fix</summary> ```diff - // Check if the then-block handles the error (doesn't re-throw) - const thenStatement = ifStmt.thenStatement; - const thenRethrows = ts.isBlock(thenStatement) - ? containsThrowStatement(thenStatement) - : ts.isThrowStatement(thenStatement); - - if (thenRethrows) { + // Check if the then-block handles the error (doesn't re-throw) + const thenStatement = ifStmt.thenStatement; + const thenHandles = ts.isBlock(thenStatement) + ? !containsThrowStatement(thenStatement) + : !ts.isThrowStatement(thenStatement); + + if (thenHandles) {
🧹 Nitpick comments (7)
src/room/data-track/packet/extensions.ts (2)
78-84: Consider validating IV length in constructor.The comment notes IV should be 12 bytes, but no validation enforces this. If
iv.length !== 12, serialization will panic at the length check (lines 111-117) with a confusing message rather than a clear validation error.🛠️ Suggested validation
constructor(keyIndex: number, iv: Uint8Array) { super(); + if (iv.length !== 12) { + throw new Error(`DataTrackE2eeExtension: IV must be exactly 12 bytes, got ${iv.length}`); + } this.keyIndex = keyIndex; this.iv = iv; }
216-233: Wrap switch case in braces to satisfy Biome linter.Biome flags
const keyIndexandconst ivdeclarations (lines 221, 223) as accessible from other switch clauses. While functionally safe here, wrapping in braces is cleaner.🔧 Suggested fix
- case DataTrackExtensionTag.E2ee: + case DataTrackExtensionTag.E2ee: { if (dataView.byteLength - byteIndex < DataTrackE2eeExtension.lengthBytes) { throw DataTrackDeserializeError.malformedExt(tag); } const keyIndex = dataView.getUint8(byteIndex); const iv = new Uint8Array(12); for (let i = 0; i < iv.length; i += 1) { let byteIndexForIv = byteIndex; byteIndexForIv += U8_LENGTH_BYTES; // key index byteIndexForIv += i * U8_LENGTH_BYTES; // Index into iv array iv[i] = dataView.getUint8(byteIndexForIv); } e2ee = new DataTrackE2eeExtension(keyIndex, iv); byteIndex += lengthBytes; break; + }src/room/data-track/packet/index.ts (2)
264-274: Non-null assertion is safe but could be clearer.The
trackHandle!assertion at line 267 is safe because the try-catch guarantees eithertrackHandleis assigned or an exception is thrown. However, restructuring could eliminate the assertion.🛠️ Optional: Eliminate non-null assertion
- let trackHandle: DataTrackHandle | undefined; - try { - trackHandle = DataTrackHandle.fromNumber(dataView.getUint16(byteIndex)); - } catch (e) { + let trackHandle: DataTrackHandle; + const rawHandle = dataView.getUint16(byteIndex); + try { + trackHandle = DataTrackHandle.fromNumber(rawHandle); + } catch (e) { if ( e instanceof DataTrackHandleError && (e.isReason(DataTrackHandleErrorReason.Reserved) || e.isReason(DataTrackHandleErrorReason.TooLarge)) ) { throw DataTrackDeserializeError.invalidHandle(e); - } else { - throw e; } + throw e; } byteIndex += U16_LENGTH_BYTES; // ...later... - trackHandle: trackHandle!, + trackHandle,
326-330: Consider usingUint8Array.set()for payload copy.The byte-by-byte loop works correctly but is slower than using the built-in
Uint8Array.set()method for bulk copy.🛠️ Suggested optimization
- const payloadBytes = new Uint8Array(this.payload); - for (let index = 0; index < payloadBytes.length; index += 1) { - dataView.setUint8(byteIndex, payloadBytes[index]); - byteIndex += U8_LENGTH_BYTES; - } + const payloadBytes = new Uint8Array(this.payload); + new Uint8Array(dataView.buffer, dataView.byteOffset + byteIndex).set(payloadBytes); + byteIndex += payloadBytes.length;throws-transformer/README.md (2)
34-34: Minor style refinement: simplify "inside of" to "inside".The phrase "throw inside of a function" can be simplified to "throw inside a function" for more concise writing.
✏️ Proposed fix
-If you have a situation where you would like to throw inside of a function annotated with a `Throws` +If you have a situation where you would like to throw inside a function annotated with a `Throws`
283-283: Clarify the scope of the "VS Code only" limitation.The statement "VS Code only: The language service plugin is VS Code specific" is misleading. While the language service plugin for real-time squiggles is VS Code specific, the README also describes ts-patch integration (lines 92-118) and a CLI checker (lines 119-129), both of which work outside VS Code. Consider clarifying that only the editor integration is VS Code specific, while build-time and CLI checking work in any environment.
📝 Proposed clarification
-3. **VS Code only**: The language service plugin is VS Code specific (other editors may vary) +3. **Editor integration**: The language service plugin is VS Code specific (other editors may vary). However, build-time checking via ts-patch and the CLI checker work in any environment.throws-transformer/engine.ts (1)
625-631: Skip method/accessor/constructor declarations when scanning for rethrows.The “nested functions” guard omits methods/accessors/constructors, so throws inside those nested scopes can incorrectly mark the catch as rethrowing.
♻️ Suggested adjustment
- if ( - ts.isFunctionDeclaration(node) || - ts.isFunctionExpression(node) || - ts.isArrowFunction(node) - ) { + if ( + ts.isFunctionDeclaration(node) || + ts.isFunctionExpression(node) || + ts.isArrowFunction(node) || + ts.isMethodDeclaration(node) || + ts.isConstructorDeclaration(node) || + ts.isGetAccessorDeclaration(node) || + ts.isSetAccessorDeclaration(node) + ) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/room/data-track/packet/extensions.tssrc/room/data-track/packet/index.test.tssrc/room/data-track/packet/index.tssrc/room/data-track/packet/serializable.tsthrows-transformer/README.mdthrows-transformer/engine.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/room/data-track/packet/serializable.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1792
File: tsconfig.json:29-29
Timestamp: 2026-01-23T20:04:34.181Z
Learning: In the livekit/client-sdk-js repository, the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path `"./throws-transformer/plugin.ts"` rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions.
📚 Learning: 2026-01-23T20:04:34.181Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1792
File: tsconfig.json:29-29
Timestamp: 2026-01-23T20:04:34.181Z
Learning: In the livekit/client-sdk-js repository, the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path `"./throws-transformer/plugin.ts"` rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions.
Applied to files:
throws-transformer/engine.tsthrows-transformer/README.md
🧬 Code graph analysis (2)
src/room/data-track/packet/index.ts (5)
src/room/data-track/handle.ts (2)
DataTrackHandle(44-66)DataTrackHandleError(10-42)src/room/data-track/utils.ts (4)
WrapAroundUnsignedInt(9-51)U16_MAX_SIZE(1-1)DataTrackTimestamp(53-70)coerceToDataView(72-86)src/room/data-track/packet/extensions.ts (1)
DataTrackExtensions(133-256)src/room/data-track/packet/constants.ts (16)
BASE_HEADER_LEN(9-9)EXT_WORDS_INDICATOR_SIZE(23-23)SUPPORTED_VERSION(8-8)VERSION_SHIFT(12-12)FRAME_MARKER_INTER(20-20)FRAME_MARKER_FINAL(19-19)FRAME_MARKER_START(18-18)FRAME_MARKER_SINGLE(21-21)FRAME_MARKER_SHIFT(15-15)EXT_FLAG_SHIFT(24-24)U8_LENGTH_BYTES(2-2)U16_LENGTH_BYTES(3-3)U32_LENGTH_BYTES(4-4)VERSION_MASK(13-13)FRAME_MARKER_MASK(16-16)EXT_FLAG_MASK(25-25)src/room/data-track/packet/errors.ts (3)
DataTrackSerializeError(89-117)DataTrackDeserializeErrorAll(76-82)DataTrackDeserializeError(13-74)
src/room/data-track/packet/extensions.ts (5)
src/index.ts (3)
DataTrackUserTimestampExtension(155-155)DataTrackE2eeExtension(156-156)DataTrackExtensions(154-154)src/room/data-track/packet/constants.ts (4)
U16_LENGTH_BYTES(3-3)U64_LENGTH_BYTES(5-5)U8_LENGTH_BYTES(2-2)EXT_TAG_PADDING(27-27)src/utils/throws.ts (1)
Throws(12-12)src/room/rpc.ts (1)
byteLength(150-153)src/room/data-track/packet/errors.ts (1)
DataTrackDeserializeError(13-74)
🪛 Biome (2.1.2)
src/room/data-track/packet/extensions.ts
[error] 221-221: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 223-223: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 ESLint
src/room/data-track/packet/index.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
🪛 GitHub Actions: Test
src/room/data-track/packet/extensions.ts
[error] 38-38: eslint: This is the default value for this type parameter, so it can be omitted. (no-unnecessary-type-arguments)
🪛 GitHub Check: test
src/room/data-track/packet/extensions.ts
[failure] 157-157:
This is the default value for this type parameter, so it can be omitted
[failure] 94-94:
This is the default value for this type parameter, so it can be omitted
[failure] 38-38:
This is the default value for this type parameter, so it can be omitted
🪛 LanguageTool
throws-transformer/README.md
[style] ~34-~34: This phrase is redundant. Consider using “inside”.
Context: ...situation where you would like to throw inside of a function annotated with a Throws ty...
(OUTSIDE_OF)
🪛 markdownlint-cli2 (0.18.1)
throws-transformer/README.md
152-152: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (13)
src/room/data-track/packet/extensions.ts (2)
1-69: LGTM!The
DataTrackUserTimestampExtensionimplementation is well-structured with proper static properties, clear serialization logic, and appropriate panic checks for invariant violations.
133-181: LGTM!The
DataTrackExtensionscontainer properly aggregates extensions, handles optional fields, and maintains serialization order consistency betweentoBinaryIntoandtoBinaryLengthBytes.src/room/data-track/packet/index.ts (5)
1-36: LGTM!Clean imports with proper organization. The use of
coerceToDataViewfrom utils ensures proper handling ofUint8Arrayslice offsets.
208-221: Good error wrapping pattern.The catch-and-rethrow pattern properly converts
DataTrackHandleErrortoDataTrackDeserializeErrorwhile preserving the cause chain. This provides clear error semantics at the API boundary.
343-356: LGTM!The
fromBinarymethod correctly usescoerceToDataViewfor proper slice handling and computes payload bounds using absolute offsets (dataView.byteOffset + ...).
87-166: LGTM!The
toBinaryIntomethod properly validates buffer capacity, handles extension word encoding with RTP-oriented semantics (length = words - 1), and includes a panic check for invariant validation.
289-299: LGTM!Clear enum documentation for
FrameMarkervalues explaining each packet position within a frame.src/room/data-track/packet/index.test.ts (6)
14-57: LGTM!Serialization test for single packet correctly validates header fields, binary length, and exact byte layout.
58-164: LGTM!Comprehensive test for final packet with both e2ee and userTimestamp extensions. Validates extension encoding, word alignment, padding, and payload placement.
258-289: LGTM!Good error case coverage testing serialization failures when the DataView is too small for header or payload.
340-376: LGTM!Error case tests properly validate deserialization failures for too-short buffers, missing extension indicators, header overruns, and unsupported versions.
413-427: LGTM!Good parameterized test using
it.eachto verify extension padding is properly skipped for different extension word counts.
554-613: LGTM!Round-trip test validates serialization-then-deserialization consistency, ensuring data integrity through the full encode/decode cycle.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
question: is the plan to move this outside of this repo eventually?
There was a problem hiding this comment.
I think it probably makes sense, yea. For now, I kept it vendored in here just to increase iteration speed and because the repo you made with this was private / I didn't have permission to make it public. But I could see it becoming a helper package like @livekit/mutex / etc once it is a bit more stable.
If you have a stronger opinion about the timeline of that move though feel free to mention it here. My preference in a vaccum I think would be to wait to break it out until the static analysis checking script is a bit more robust and some other folks (at least you) have been able to give ita try and weigh in on how it works.
|
It would be nice to include an example that makes use of the data tracks in the |
@lukasIO The packet serialization / deserialization code isn't something that a user would call directly, so I don't think it makes sense to add an example using it. That being said, once all the data tracks code is complete, I 100% agree, there should be an example deomonstrating end to end use, but I will include that in a follow up change once I've made more progress on the abstractions and it is all integrated with |
…p format And fix an off by one error along the way.
Implements binary serialization and deserialization for data track packets. For now, this is being introduced in isolation to reduce the review burden, and further data tracks work will be done in follow up pull requests which build on top of this.
Included in this change also is some relatively early stage error handling infrastructure to attempt to further iterate on the typesafe error handling pull requests that lukas has been working on. I think we both still aren't really satisfied with the current state of things (namely,
TypedPromisedoesn't really support synchronous results, and introducing a new separate synchronousResulttype of some sort makes interop between these two types a challenge). A LLM suggested we explore thisThrowsbranded type approach (see./throws-transformer/README.mdfor more info about how it works) which seems to have some fairly early promise, so for now I've opted to build the data tracks error handling logic on top. That is part of what is resulting in the diff size of this change being so large 😬 .Summary by CodeRabbit
New Features
Tools
Errors & Utilities
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.