Data tracks packetizer and depacketizer #1798
Conversation
🦋 Changeset detectedLatest commit: e98d4d3 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 DataTrack packetizer and depacketizer implementations, tests, and related frame/packet/type changes; changes DataTrackPacket payloads to Uint8Array; enhances timestamp/clock and wrap-around utilities; expands Throws types and extractor behavior; adds logger enum entry, side-effect index imports, and a changeset entry. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Pkt as DataTrackPacketizer
participant Clock as DataTrackClock
participant Packet as DataTrackPacket
rect rgba(200,200,255,0.5)
App->>Pkt: *packetize*(frame, options?)
Pkt->>Clock: now() / at()
Clock-->>Pkt: DataTrackTimestamp
Pkt->>Packet: construct header + payload (iterative)
Packet-->>Pkt: packet
Pkt-->>App: yields DataTrackPacket (Single/Start/Inter/Final)
end
sequenceDiagram
participant Net as Network/Application
participant Dpkt as DataTrackDepacketizer
participant Packet as DataTrackPacket
participant Partial as PartialFrame
participant Err as DropError
rect rgba(200,255,200,0.5)
loop incoming packets
Net->>Dpkt: push(packet, options?)
alt marker == Single
Dpkt->>Partial: frameFromSingle()
Partial-->>Dpkt: {payload, extensions}
Dpkt-->>Net: frame
else marker == Start
Dpkt->>Partial: beginPartial()
else marker == Inter or Final
Dpkt->>Partial: pushToPartial()
alt Final && complete
Partial->>Partial: finalize()
Partial-->>Dpkt: {payload, extensions}
Dpkt-->>Net: frame
else incomplete / error
Dpkt-->>Err: throw DropError(reason)
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
1egoman
force-pushed
the
data-tracks-packetizer
branch
from
0e7268e to
464e011
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@src/room/data-track/depacketizer.test.ts`:
- Around line 164-208: The test fails because the Inter packets reuse the same
sequence as the Start packet; update the loop that constructs interPacket so
sequences begin after the Start packet's sequence (e.g., start from Start
sequence + 1) by using WrapAroundUnsignedInt.u16 with an offset instead of
WrapAroundUnsignedInt.u16(i), ensuring DataTrackPacketHeader.sequence for
FrameMarker.Inter does not collide with the Start packet's sequence; verify the
extraInterPacket sequence remains one past
DataTrackDepacketizer.MAX_BUFFER_PACKETS as originally intended.
- Around line 29-78: The test creates overlapping sequence numbers between the
Start packet and the Inter packets; update the Inter and Final packet sequence
values so they follow the Start packet (Start sequence=0, Inter sequences start
at 1, Final = interPacketCount+1). Concretely, in the loop where Inter packets
are created change sequence: WrapAroundUnsignedInt.u16(i) to
WrapAroundUnsignedInt.u16(i + 1), and set the Final packet sequence to
WrapAroundUnsignedInt.u16(interPacketCount + 1) so DataTrackDepacketizer sees
monotonically increasing sequences for DataTrackPacketHeader/FrameMarker
handling.
In `@src/room/data-track/depacketizer.ts`:
- Around line 234-266: The finalize logic incorrectly compares
sequencePointer.value < endSequence and computes expected packets linearly,
which fails across the u16 wrap boundary; update the loop to use a wrap-aware
comparison (e.g., iterate until sequencePointer.value equals endSequence using
the existing sequencePointer.increment(1) and break/return after processing the
endSequence packet) and compute expectedPackets with modular arithmetic
(expected = (endSequence - partial.startSequence.value + 1 + 65536) % 65536,
treating 0 as 65536 if needed) before calling
DataTrackDepacketizerDropError.incomplete; reference sequencePointer,
partial.startSequence, endSequence, and
DataTrackDepacketizerDropError.incomplete when applying the change.
In `@src/room/data-track/packet/extensions.ts`:
- Around line 216-242: The E2ee switch case has two issues: the declarations
keyIndex and iv must be scoped inside a block to avoid leaking into other cases,
and byteIndex is advanced incorrectly after parsing — since lengthBytes
represents actualLength - 1, increment byteIndex by lengthBytes + 1 (not
lengthBytes) to keep parsing aligned. Wrap the DataTrackExtensionTag.E2ee case
body in braces, keep the reads using DataTrackE2eeExtension / U8_LENGTH_BYTES /
DataTrackDeserializeError.malformedExt as-is, and replace byteIndex +=
lengthBytes with byteIndex += lengthBytes + 1 so the parser advances correctly.
In `@src/room/data-track/packetizer.ts`:
- Around line 41-42: Update the JSDoc in packetizer.ts to reference the correct
type: replace the incorrect {`@link` DataTrackPacketizerFrame} with {`@link`
DataTrackFrame} so the comment for the packetizer (which mentions producing
{`@link` DataTrackPacket}s) links to the actual frame type; ensure the rest of the
sentence and braces remain unchanged (i.e., keep the {`@link` DataTrackPacket}s
link as-is).
- Around line 94-109: The current check only throws when
headerSerializedLengthBytes > this.mtuSizeBytes which allows
headerSerializedLengthBytes === this.mtuSizeBytes to produce maxPayloadSizeBytes
= 0, packetCount = Infinity and an infinite loop; update the guard so that when
headerSerializedLengthBytes >= this.mtuSizeBytes you throw
DataTrackPacketizerError.mtuTooShort() (or explicitly compute
maxPayloadSizeBytes and throw if maxPayloadSizeBytes <= 0) before computing
packetCount and entering the for loop that uses indexBytes to advance.
In `@src/room/data-track/utils.ts`:
- Around line 173-179: The conversion in durationInMsToTicks is using
milliseconds * 1000 (microseconds) but treats the value as nanoseconds; update
the math so durationNanoseconds = durationMilliseconds * 1_000_000 (or rename
variables and adjust divisors consistently) and ensure the subsequent
calculation uses nanoseconds with rateInHz and /1_000_000_000 as intended;
modify the durationInMsToTicks function to produce correct tick counts by fixing
the multiplier or aligning the units and variable names.
- Around line 158-171: The elapsed calculation in the at(timestamp: Date) method
is reversed; change the elapsedMs computation to subtract epoch from the
provided timestamp so elapsedMs = timestamp.getTime() - this.epoch.getTime();
keep the rest of the logic (calling DataTrackClock.durationInMsToTicks, cloning
this.base, calling result.wrappingAdd, and the monotonicity check against
this.previous) unchanged so future timestamps produce increasing tick offsets.
In `@throws-transformer/engine.ts`:
- Around line 664-689: In visitIfStatement, the then-branch uses thenRethrows
(computed from containsThrowStatement) to decide to collect "handled" types, but
that is inverted: it should collect types only when the then-block does NOT
rethrow. Change the condition that pushes into narrowedTypes to use the negation
of thenRethrows (i.e., !thenRethrows) so getNarrowedTypeInBlock(errorVarName,
checker) is only added for branches that handle the error; keep handling of
union constituents and the use of isAnyOrUnknownType the same.
- Around line 36-57: The visit function currently analyzes call expressions
twice: once when the CallExpression node is visited (via checkCallExpression)
and again in the await-specific branch, causing duplicate diagnostics for
awaited calls; remove the await-specific block that checks
ts.isAwaitExpression(node) && ts.isCallExpression(node.expression) (the lines
that call checkCallExpression on node.expression and push to results) so that
checkCallExpression is only invoked once per CallExpression, while keeping the
existing Promise.reject handling (checkPromiseReject) intact.
🧹 Nitpick comments (17)
src/utils/TypedPromise.ts (1)
17-22: Consider using standard method overload syntax for consistency.The other static methods (
reject,all,race) use standard method syntax, whileresolveuses a static property with an arrow function. Both approaches work correctly, but using method overloads would be more consistent:♻️ Alternative using method overload syntax
- static resolve: { - (): TypedPromise<void, never>; - <V>(value: V): TypedPromise<Awaited<V>, never>; - } = <V>(value?: V): TypedPromise<Awaited<V>, never> => { - return super.resolve(value) as TypedPromise<Awaited<V>, never>; - }; + static resolve(): TypedPromise<void, never>; + static resolve<V>(value: V): TypedPromise<Awaited<V>, never>; + static resolve<V>(value?: V): TypedPromise<Awaited<V>, never> { + return super.resolve(value) as TypedPromise<Awaited<V>, never>; + }src/utils/throws.ts (1)
21-26: Consider the fallback behavior ofExtractSuccess.
ExtractSuccess<T>falls back toTwhenTis not aThrowstype (line 26). This is likely intentional for ergonomics, but it means misuse (applyingExtractSuccessto a non-Throws type) won't produce a type error—it will silently return the original type. If stricter checking is desired, you could returnneverfor non-Throws inputs.src/room/data-track/handle.ts (2)
44-66: Public constructor bypasses validation.The
constructoris public, allowing direct instantiation with invalid values (e.g.,new DataTrackHandle(0)ornew DataTrackHandle(100000)), which bypasses the validation infromNumber(). Consider making the constructor private to enforce validation through the factory method.♻️ Proposed fix
export class DataTrackHandle { - public value: number; + public readonly value: number; static fromNumber( raw: number, ): Throws< DataTrackHandle, | DataTrackHandleError<DataTrackHandleErrorReason.TooLarge> | DataTrackHandleError<DataTrackHandleErrorReason.Reserved> > { if (raw === 0) { throw DataTrackHandleError.reserved(raw); } if (raw > U16_MAX_SIZE) { throw DataTrackHandleError.tooLarge(); } return new DataTrackHandle(raw); } - constructor(raw: number) { + private constructor(raw: number) { this.value = raw; } }Note: If the public constructor is intentional (e.g., for
DataTrackHandleAllocator), consider adding a static factory method likestatic unsafeFromNumber(raw: number)with clear naming to indicate it bypasses validation, or makeDataTrackHandleAllocatora friend class pattern.
68-79: Static allocator state cannot be reset.
DataTrackHandleAllocatoruses static mutable state with no reset mechanism. This could cause issues in testing scenarios where you need to reset the allocator between tests. Consider adding areset()method for testing purposes.♻️ Proposed addition for testability
/** Manage allocating new handles which don't conflict over the lifetime of the client. */ export class DataTrackHandleAllocator { static value = 0; /** Returns a unique track handle for the next publication, if one can be obtained. */ static get(): DataTrackHandle | null { this.value += 1; if (this.value > U16_MAX_SIZE) { return null; } return new DataTrackHandle(this.value); } + + /** `@internal` Reset the allocator. Only for testing purposes. */ + static reset(): void { + this.value = 0; + } }package.json (1)
52-52: Consider addingtsxas a devDependency.The script uses
pnpm dlx tsxwhich downloads tsx on-demand. For faster local development and CI runs, consider addingtsxto devDependencies and usingpnpm tsxinstead.src/room/data-track/handle.test.ts (2)
1-2: Remove unused imports.
afterEach,beforeEach, andviare imported but not used in this test file.Suggested fix
-/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest';
6-12: Consider adding boundary value tests.The test covers basic cases well. For more robust coverage, consider testing the boundary at
U16_MAX_SIZE(65535):
fromNumber(65535)should succeed (max valid value)fromNumber(65536)should throw "too large"src/room/data-track/utils.test.ts (1)
1-2: Remove unused imports.
afterEach,beforeEach, andviare imported but not used.Suggested fix
-/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest';src/room/data-track/frame.ts (1)
1-2: Useimport typefor JSDoc-only imports.
DataTrackPacketizeris only referenced in the JSDoc comment, not at runtime. Consider usingimport typeto clarify intent and potentially improve tree-shaking.Suggested fix
import { DataTrackExtensions } from './packet/extensions'; -import { DataTrackPacketizer } from './packetizer'; +import type { DataTrackPacketizer } from './packetizer';throws-transformer/README.md (2)
34-37: Minor style improvement.Consider simplifying "inside of" to "inside" for cleaner prose.
Suggested 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`
152-155: Add language specifier to fenced code block.The markdown linter suggests adding a language specifier. Since this is plain text output, use
textorplaintext.Suggested fix
-``` +```text Unhandled error(s) from 'fetchUser': NetworkError | NotFoundError. Catch these errors or add 'Throws<..., NetworkError | NotFoundError>' to your function's return type. ```throws-transformer/cli.ts (1)
24-25: Handle case when no files match the glob patterns.If the glob patterns don't match any files,
fileswill be an empty array. The program will silently succeed with "All Throws errors are handled correctly!" which may hide configuration issues.Suggested fix
// Resolve file paths const files = args.flatMap((f) => globSync(f)); + + if (files.length === 0) { + console.error("No files matched the provided patterns."); + process.exit(1); + }src/index.ts (1)
152-159: Resolve FIXME comments before merging.The FIXME comments on lines 158-159 indicate uncertainty about whether
DataTrackPacketizerandDataTrackDepacketizershould be part of the public API. This decision should be resolved before merging since removing exports later would be a breaking change.Consider:
- If these are internal utilities, keep them unexported
- If they're intended for advanced users, document the use case and remove the FIXME comments
Would you like me to open an issue to track the API surface decision?
throws-transformer/transformer.ts (1)
42-49: Usemapinstead offlatMapwhen returning a single item.
flatMapis typically used when the callback returns an array that should be flattened. Here, each result maps to exactly oneDiagnostic, makingmapthe idiomatic choice.Proposed fix
- const diagnostics = results.flatMap((result): ts.Diagnostic => ({ + const diagnostics = results.map((result): ts.Diagnostic => ({ file: result.sourceFile, start: result.start, length: result.length, messageText: result.message, category: ts.DiagnosticCategory.Error, code: 90001, // Custom error code }));Apply same change at line 105.
Also applies to: 105-112
throws-transformer/plugin.ts (1)
53-86: SameflatMap→mapsuggestion applies here.Also, the error handling is well implemented—catching exceptions and falling back to original diagnostics ensures the plugin doesn't break the editor experience.
Proposed fix for flatMap
- const diagnostics = results.flatMap((result): ts.Diagnostic => ({ + const diagnostics = results.map((result): ts.Diagnostic => ({ file: result.sourceFile, start: result.start, length: result.length, messageText: result.message, category: ts.DiagnosticCategory.Error, code: 90001, // Custom error code }));src/room/data-track/packet/serializable.ts (1)
12-27: Resolve the toBinary return-type FIXME before the API hardens.
Line 27 leaves an open contract question (Uint8Array vs ArrayBuffer). Please finalize the return type and align docs/usages to avoid ambiguity for callers.src/room/data-track/packet/index.ts (1)
326-329: Consider usingUint8Array.prototype.setfor payload copy.The byte-by-byte loop works correctly but is less efficient than using the built-in
setmethod for copying typed arrays.Suggested optimization
- for (let index = 0; index < this.payload.length; index += 1) { - dataView.setUint8(byteIndex, this.payload[index]); - byteIndex += U8_LENGTH_BYTES; - } + new Uint8Array(dataView.buffer, dataView.byteOffset + byteIndex, this.payload.length).set(this.payload); + byteIndex += this.payload.length;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.changeset/busy-ends-shake.md.github/workflows/test.yamlpackage.jsonsrc/index.tssrc/logger.tssrc/room/data-track/depacketizer.test.tssrc/room/data-track/depacketizer.tssrc/room/data-track/frame.tssrc/room/data-track/handle.test.tssrc/room/data-track/handle.tssrc/room/data-track/packet/constants.tssrc/room/data-track/packet/errors.tssrc/room/data-track/packet/extensions.tssrc/room/data-track/packet/index.test.tssrc/room/data-track/packet/index.tssrc/room/data-track/packet/serializable.tssrc/room/data-track/packetizer.test.tssrc/room/data-track/packetizer.tssrc/room/data-track/utils.test.tssrc/room/data-track/utils.tssrc/room/errors.tssrc/utils/TypedPromise.tssrc/utils/throws.tsthrows-transformer/README.mdthrows-transformer/cli.tsthrows-transformer/engine.tsthrows-transformer/plugin.tsthrows-transformer/transformer.tstsconfig.json
🧰 Additional context used
🧠 Learnings (1)
📚 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/transformer.tstsconfig.jsonthrows-transformer/README.mdpackage.jsonthrows-transformer/engine.tsthrows-transformer/cli.tsthrows-transformer/plugin.ts
🧬 Code graph analysis (10)
src/room/data-track/utils.test.ts (1)
src/room/data-track/utils.ts (1)
WrapAroundUnsignedInt(10-76)
src/room/data-track/packet/serializable.ts (2)
src/utils/throws.ts (1)
Throws(14-16)src/room/data-track/packet/errors.ts (1)
DataTrackSerializeErrorAll(119-121)
src/room/data-track/packet/index.test.ts (5)
src/room/data-track/packet/index.ts (2)
DataTrackPacketHeader(38-287)DataTrackPacket(302-363)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(10-76)DataTrackTimestamp(78-113)src/room/data-track/packet/extensions.ts (3)
DataTrackExtensions(133-256)DataTrackUserTimestampExtension(18-69)DataTrackE2eeExtension(71-131)src/room/data-track/packet/constants.ts (1)
EXT_FLAG_SHIFT(24-24)
src/room/data-track/depacketizer.test.ts (3)
src/room/data-track/depacketizer.ts (1)
DataTrackDepacketizer(90-268)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (1)
WrapAroundUnsignedInt(10-76)
throws-transformer/transformer.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
src/room/data-track/packetizer.test.ts (4)
src/room/data-track/packetizer.ts (1)
DataTrackPacketizer(43-128)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (1)
DataTrackTimestamp(78-113)src/room/data-track/frame.ts (1)
DataTrackFrame(5-8)
src/room/data-track/packet/errors.ts (1)
src/room/data-track/handle.ts (1)
DataTrackHandleError(10-42)
src/room/data-track/depacketizer.ts (4)
src/index.ts (3)
getLogger(120-120)LoggerNames(103-103)DataTrackExtensions(154-154)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(10-76)U16_MAX_SIZE(1-1)src/utils/throws.ts (1)
Throws(14-16)src/room/data-track/frame.ts (1)
DataTrackFrame(5-8)
src/room/data-track/handle.test.ts (1)
src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)
throws-transformer/cli.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
🪛 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/utils.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/packet/index.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/depacketizer.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/packetizer.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/handle.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
🪛 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 (58)
.changeset/busy-ends-shake.md (1)
1-5: LGTM!The changeset correctly declares a patch release and provides a clear, concise description of the added functionality.
tsconfig.json (2)
12-13: LGTM!Adding
ES2022.Errorenables theError.causeproperty for proper error chaining, which aligns well with the new error handling infrastructure being introduced.
28-29: LGTM!The throws-transformer plugin configuration uses a relative file path, which is the intended approach for this repository's workspace TypeScript setup. Based on learnings: "the throws-transformer TypeScript language service plugin is configured in tsconfig.json using a relative file path... rather than a package name. This file path configuration is intentional and valid for VS Code when using workspace TypeScript versions."
src/logger.ts (1)
24-24: LGTM!The new
DataTrackslogger name follows the existing naming convention and will be automatically included in thelivekitLoggersarray, enabling proper log level control for the new data-tracks functionality.src/utils/throws.ts (2)
14-16: LGTM!The
Throwstype cleverly uses a phantom__throwsproperty for compile-time error tracking without runtime overhead. The| Extract<T, null | undefined>correctly preserves nullable types in the union.
31-33: LGTM!The recursive
CombineErrorstype correctly unions error types from a tuple and terminates withneverfor the empty case..github/workflows/test.yaml (1)
29-30: LGTM!The
throws:checkCI step is appropriately placed in the workflow after linting and formatting, ensuring static error analysis runs before tests. This enforces the newThrowstype checking as part of the CI pipeline.package.json (1)
102-102: LGTM!The
globdependency is appropriately placed in devDependencies since it's only used by the throws-transformer CLI tool during development and CI.src/room/data-track/utils.test.ts (1)
5-27: LGTM! Comprehensive wrap-around tests.The tests effectively cover initialization, edge cases, and bidirectional wrap-around behavior. The implementation from
utils.tsshows there's also au32()factory method—if u32 wrapping is used elsewhere, consider adding a similar test case for it.src/room/data-track/frame.ts (1)
4-8: LGTM!Clean and well-documented type definition. The
DataTrackFrametype clearly represents the input to the packetizer with payload bytes and associated extensions.throws-transformer/README.md (1)
1-263: Well-documented tooling.The README provides comprehensive coverage of the throws-transformer: problem statement, solution, IDE integration, CLI usage, handling patterns, and limitations. The examples are practical and the escape hatch (
@throws-transformer ignore) is well-documented.throws-transformer/cli.ts (2)
34-61: LGTM! Robust tsconfig handling.The fallback compiler options are sensible defaults, and the tsconfig parsing properly handles errors with clear messages. Using
ts.parseJsonConfigFileContentcorrectly resolves extended configs.
78-95: LGTM! Clear output formatting.The error output follows standard conventions with file:line:column format, and the ANSI color codes provide good visual feedback. Exit codes are appropriate (0 for success, 1 for errors).
src/room/data-track/packetizer.test.ts (4)
1-8: LGTM on imports and setup.The ESLint disable for unused-vars is appropriate for test files, and the vitest import error from static analysis is a false positive since vitest is configured as a devDependency.
11-83: LGTM on large payload packetization test.The test validates correct segmentation of a 300-byte payload across 4 packets with proper frame markers (Start → Inter → Inter → Final), consistent headers, and accurate payload sizes (88 + 88 + 88 + 36 = 300 bytes).
85-119: LGTM on parameterized edge case tests.Good coverage of boundary conditions including zero payload, single packet fit, and multi-packet scenarios with different MTU sizes. The assertions properly validate frame markers, header consistency, and extension equality.
121-130: LGTM on computeFrameMarker utility tests.The test cases correctly cover the four key scenarios: single packet, start of multi-packet, intermediate, and final packet.
src/room/data-track/depacketizer.test.ts (4)
11-27: LGTM on single packet depacketization test.The test correctly validates that a single-marker packet produces a complete frame with empty payload and default extensions.
80-112: LGTM on interrupted frame test.The test correctly validates that receiving a new Start packet while a frame is in progress triggers an "Interrupted" error when
errorOnPartialFramesis enabled.
114-144: LGTM on incomplete frame test.The test correctly validates that a Final packet arriving with gaps in the sequence (expected 4 packets, received 2) triggers an "incomplete" error with an accurate count.
146-162: LGTM on unknown frame test.The test correctly validates that receiving an Inter packet without a preceding Start packet triggers an "unknownFrame" error.
src/room/data-track/packet/index.test.ts (6)
1-13: LGTM on imports.Comprehensive imports covering packet classes, extensions, and test utilities. The static analysis vitest error is a false positive.
14-164: LGTM on serialization tests.Thorough byte-level validation of packet serialization covering:
- Single packet without extensions
- Final packet with both e2ee and userTimestamp extensions
- Proper extension padding to word alignment
The inline comments documenting each byte improve maintainability.
165-289: LGTM on start packet and error case tests.Good coverage of:
- Start packet with only e2ee extension
- Buffer too small for header error
- Buffer too small for payload error
292-411: LGTM on deserialization tests.Comprehensive deserialization coverage including:
- Valid single packet parsing
- Too short buffer detection
- Missing extension word indicator
- Header overrun detection
- Unsupported version rejection
- Base header parsing
413-551: LGTM on extension handling tests.Good validation of:
- Extension padding skip behavior
- E2ee extension deserialization
- UserTimestamp extension deserialization
- Unknown extension forward compatibility (skipping)
- Word alignment enforcement
554-615: LGTM on round-trip test.The round-trip serialization + deserialization test validates data integrity through the encode/decode cycle.
throws-transformer/transformer.ts (3)
1-28: LGTM on documentation and config interface.Clear documentation explaining the transformer's purpose and ts-patch integration. The
TransformerConfiginterface provides a clean extension point for future options.
66-80: LGTM on reportDiagnostic helper.Clean diagnostic formatting with proper line/character position handling and fallback for diagnostics without file context.
86-113: LGTM on standalone checkFile function.Provides a useful entry point for running checks without ts-patch, with sensible default compiler options.
throws-transformer/plugin.ts (3)
1-26: LGTM on documentation and imports.Clear setup instructions for VS Code integration. The relative file path in the example aligns with the intentional configuration approach for this repository.
30-51: LGTM on plugin initialization and proxy setup.The proxy pattern correctly wraps the existing language service while preserving all original methods through proper binding.
88-94: LGTM on module export.Clean export of the plugin module factory.
throws-transformer/engine.ts (2)
82-645: Core diagnostics and propagation helpers look consistent.
The throw/reject/return checks and the related propagation/handling utilities are coherent and readable.
747-861: Helper utilities look good.
The remaining narrowing helpers and utility functions are clean and easy to follow.src/room/data-track/depacketizer.ts (1)
24-221: LGTM for marker routing and partial-frame handling.
The marker-based routing and partial buffering logic is clear and well-structured.src/room/data-track/packet/constants.ts (1)
1-27: LGTM for serialization constants.
The sizing, bit masks, and marker constants are clear and cohesive.src/room/data-track/packetizer.ts (2)
14-39: LGTM for error type plumbing.
The error class and reason enum are clean and consistent with the rest of the error hierarchy.
59-71: LGTM for frame-marker selection.
Marker selection for single/start/inter/final packets looks correct.src/room/data-track/packet/errors.ts (1)
4-117: LGTM for error taxonomy and factories.
The deserialize/serialize error families are well-structured and consistent with existing error patterns.src/room/errors.ts (5)
3-18: LGTM!The
causeproperty addition follows the standard Error cause convention. The conditional assignment on lines 15-17 correctly handles the optional cause parameter, and the typo fix in the default message is appreciated.
21-29: LGTM!The
LivekitReasonedError<Reason>abstraction provides a clean generic base for errors that carry typed reason information. The abstract members ensure subclasses implementreasonandreasonNameconsistently.
108-132: LGTM!
ConnectionErrornow correctly extendsLivekitReasonedError<Variant['reason']>, and all existing functionality (static factory methods, variant types) is preserved.
254-266: LGTM!
SignalRequestErrorcorrectly extends the new base class and implements the required abstract members.
292-304: LGTM!
DataStreamErrorcorrectly extends the new base class and implements the required abstract members.src/room/data-track/utils.ts (3)
1-76: LGTM!The
WrapAroundUnsignedIntclass correctly implements wrap-around behavior for unsigned integers. The validation in the constructor, the clamp logic, and the utility methods (increment,decrement,getThenIncrement) are well-implemented.
78-113: LGTM!The
DataTrackTimestampclass provides a clean abstraction for RTP-style timestamps with wrap-around semantics. The static factory methods and instance methods are straightforward.
182-196: LGTM!The
coerceToDataViewfunction correctly handles all three input types and properly accounts forbyteOffsetandbyteLengthwhen constructing aDataViewfrom aUint8Array.src/room/data-track/packet/index.ts (6)
1-36: LGTM!The imports are well-organized and cover all necessary dependencies for the packet header and packet serialization/deserialization.
37-86: LGTM!The
DataTrackPacketHeaderclass is well-structured with clear field definitions. TheextensionsMetrics()helper correctly calculates word-aligned padding, andtoBinaryLengthBytes()properly accounts for the extension section overhead.
87-166: LGTM!The
toBinaryIntomethod correctly serializes the header with proper byte alignment, extension handling, and final length validation. The "panic" pattern for internal consistency checks (lines 159-163) is appropriate for catching programming errors.
168-275: LGTM!The
fromBinarydeserialization logic is thorough with proper bounds checking, version validation, and error mapping for invalid handles. The extension parsing correctly accounts for the RTP-oriented length encoding.
289-299: LGTM!The
FrameMarkerenum clearly documents each marker's semantics.
342-363: LGTM!The
DataTrackPacket.fromBinarymethod correctly deserializes the header first, then extracts the remaining bytes as payload. ThetoJSONmethod provides a useful debugging representation.src/room/data-track/packet/extensions.ts (4)
1-69: LGTM!The
DataTrackUserTimestampExtensionclass correctly implements serialization with the RTP-oriented length encoding (lengthBytes - 1) and provides proper JSON representation.
71-131: LGTM!The
DataTrackE2eeExtensionclass correctly serializes the keyIndex and IV fields. The comment noting the expected 12-byte IV length is helpful.
133-181: LGTM!The
DataTrackExtensionscontainer correctly aggregates the individual extensions and handles serialization in a consistent order.
245-255: LGTM!The method correctly returns the full
dataView.byteLengthas the consumed byte count, and thetoJSONrepresentation is clean.
✏️ 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: 5
🤖 Fix all issues with AI agents
In `@src/room/data-track/packetizer.ts`:
- Around line 85-126: packetCount can be 0 for zero-length frame.payload which
causes the loop to never run and drops empty frames/extensions; change
packetCount to at least 1 (e.g. packetCount = Math.max(1, Math.ceil(...))) and
switch the loop to iterate by packet index (for let index = 0; index <
packetCount; index++) computing indexBytes = index * maxPayloadSizeBytes so you
still generate a packet for empty payloads (produce a zero-length Uint8Array for
packetPayload) and keep using
DataTrackPacketHeader/computeFrameMarker/sequence/frameNumber as before so
extensions and headers are emitted.
In `@src/room/data-track/utils.ts`:
- Around line 110-112: isBefore currently does a raw numeric comparison which
fails across u32 wraparound; update DataTrackTimestamp<RateInHz>.isBefore to
perform a wraparound-aware comparison by computing the signed distance in 32-bit
modular space (treat the difference as a signed 32-bit value) and return true
only if that signed difference indicates this.timestamp is earlier than
other.timestamp. Replace the simple "<" check with a modular signed-diff test so
the monotonicity check in the timestamp advancement code (the monotonicity check
referenced in the review) behaves correctly across 0xFFFFFFFF -> 0x00000000
wraparounds.
In `@throws-transformer/README.md`:
- Around line 34-36: Change the phrasing "inside of a function annotated with a
`Throws`" to "inside a function annotated with a `Throws`" in the README
sentence that explains throwing plain `Error`s for assertion/panic cases so the
language is tightened and reads "inside a function annotated with a `Throws`".
- Around line 152-155: The fenced code block containing the message "Unhandled
error(s) from 'fetchUser': NetworkError | NotFoundError." needs a language tag
to satisfy markdownlint MD040; update that fenced block to start with ```text
(or ```txt) instead of just ``` so the block is marked as plain text and the
lint warning is resolved, locating the block by the exact string "Unhandled
error(s) from 'fetchUser': NetworkError | NotFoundError." in the README.md.
- Around line 241-243: The README snippet for the Throws type is out of sync
with the real definition; update the doc example for the type alias Throws to
match the implementation by including the default type parameter (E extends
Error = never) and the null/undefined extraction (use NonNullable<T> or explicit
Exclude<T, null | undefined>) so the documented signature matches the actual
declaration (reference the Throws type alias).
🧹 Nitpick comments (17)
src/utils/TypedPromise.ts (1)
17-22: Consider using standard static method overloads for consistency.The
resolvemethod is defined as an arrow function property while the adjacentrejectmethod (line 24-26) uses a regular static method. Standard TypeScript method overloads would be more conventional and consistent:♻️ Suggested refactor using method overloads
- static resolve: { - (): TypedPromise<void, never>; - <V>(value: V): TypedPromise<Awaited<V>, never>; - } = <V>(value?: V): TypedPromise<Awaited<V>, never> => { - return super.resolve(value) as TypedPromise<Awaited<V>, never>; - }; + static resolve(): TypedPromise<void, never>; + static resolve<V>(value: V): TypedPromise<Awaited<V>, never>; + static resolve<V>(value?: V): TypedPromise<Awaited<V>, never> { + return super.resolve(value) as TypedPromise<Awaited<V>, never>; + }throws-transformer/transformer.ts (3)
42-49: Use.map()instead of.flatMap()when returning a single element.The callback returns a single
ts.Diagnosticobject, not an array. Using.flatMap()here works due to TypeScript's type coercion but is semantically misleading.Suggested fix
- const diagnostics = results.flatMap((result): ts.Diagnostic => ({ + const diagnostics = results.map((result): ts.Diagnostic => ({ file: result.sourceFile, start: result.start, length: result.length, messageText: result.message, category: ts.DiagnosticCategory.Error, code: 90001, // Custom error code }));
33-36: Unusedconfigparameter.The
configparameter (includingstrictCatchHandling) is declared but never used. Consider removing it until needed, or document the intended future use.
105-112: Use.map()instead of.flatMap()here as well.Same issue as in the transformer function—the callback returns a single diagnostic, not an array.
Suggested fix
- return results.flatMap((result): ts.Diagnostic => ({ + return results.map((result): ts.Diagnostic => ({ file: result.sourceFile, start: result.start, length: result.length, messageText: result.message, category: ts.DiagnosticCategory.Error, code: 90001, // Custom error code }));throws-transformer/cli.ts (1)
24-26: Consider warning when no files match the provided patterns.If the glob patterns don't match any files, the CLI silently reports success. This could mask user mistakes (e.g., typos in patterns).
Suggested enhancement
// Resolve file paths const files = args.flatMap((f) => globSync(f)); + + if (files.length === 0) { + console.error("No files matched the provided patterns."); + process.exit(1); + }throws-transformer/plugin.ts (1)
72-79: Use.map()instead of.flatMap()for consistency.The callback returns a single
ts.Diagnostic, not an array. This is the same pattern as intransformer.ts.Suggested fix
- const diagnostics = results.flatMap((result): ts.Diagnostic => ({ + const diagnostics = results.map((result): ts.Diagnostic => ({ file: result.sourceFile, start: result.start, length: result.length, messageText: result.message, category: ts.DiagnosticCategory.Error, code: 90001, // Custom error code }));src/room/data-track/handle.test.ts (1)
1-2: Consider removing unused imports.
afterEach,beforeEach, andviare imported but not used in this test file. While the ESLint rule is disabled, removing unused imports keeps the code cleaner.Proposed fix
-/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest';src/room/data-track/frame.ts (1)
2-2: Unused import for JSDoc reference.
DataTrackPacketizeris imported but only referenced in the JSDoc{@link}. Consider usingimport typeto make it clear this is a type-only import for documentation purposes, or remove it if the link isn't essential.Proposed fix
import { DataTrackExtensions } from './packet/extensions'; -import { DataTrackPacketizer } from './packetizer'; +import type { DataTrackPacketizer } from './packetizer';src/room/data-track/utils.test.ts (1)
1-3: Consider removing unused imports.Same as in
handle.test.ts:afterEach,beforeEach, andviare imported but unused.Proposed fix
-/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest';src/room/data-track/packetizer.test.ts (1)
1-2: Consider removing unused imports.
afterEach,beforeEach, andviare imported but not used.Proposed fix
-/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest';src/index.ts (1)
158-159: Address FIXME comments before merging.The comments question whether
DataTrackPacketizerandDataTrackDepacketizershould be part of the public API. This decision should be resolved before the PR is finalized, as it affects the library's public surface area and versioning commitments.Would you like me to help evaluate the use cases for these exports to determine if they should remain public or be kept internal?
src/room/data-track/packet/serializable.ts (1)
12-28: Resolve the FIXME on thetoBinaryreturn type.The method already returns
Uint8Array; leaving a FIXME in a base class makes the API ambiguous. Either document the choice explicitly and remove the FIXME, or change the return type intentionally.🧹 Suggested cleanup
- /** Encodes the instance as binary and returns the data as a Uint8Array. */ + /** Encodes the instance as binary and returns the data as a Uint8Array. */ toBinary(): Throws<Uint8Array, DataTrackSerializeErrorAll> { const lengthBytes = this.toBinaryLengthBytes(); const output = new ArrayBuffer(lengthBytes); const view = new DataView(output); @@ - return new Uint8Array(output); // FIXME: return uint8array here? Or the arraybuffer? + return new Uint8Array(output); }src/room/data-track/handle.ts (1)
47-60: Consider guarding against negative handle inputs.
fromNumberaccepts negative values today, which would create an invalid u16 handle. If this is a public API surface, consider adding a guard (and a corresponding error reason) to keep handle invariants intact.src/room/data-track/packet/index.ts (1)
326-329: Consider usingUint8Array.set()for payload serialization.The byte-by-byte loop works correctly but
Uint8Array.set()would be more efficient and idiomatic for copying typed arrays.♻️ Suggested improvement
- for (let index = 0; index < this.payload.length; index += 1) { - dataView.setUint8(byteIndex, this.payload[index]); - byteIndex += U8_LENGTH_BYTES; - } + new Uint8Array(dataView.buffer, dataView.byteOffset + byteIndex, this.payload.length).set(this.payload); + byteIndex += this.payload.length;src/room/data-track/utils.ts (1)
158-171: Potential aliasing issue with returned timestamp.When
result.isBefore(this.previous)is true,resultis reassigned tothis.previous, and thenthis.previous = resultkeeps the same reference. The returned timestamp is the same object asthis.previous, meaning if the caller mutates it (viawrappingAdd), it will corrupt the clock's internal state.Consider returning a clone:
♻️ Suggested fix
// Enforce monotonicity in RTP wraparound space if (result.isBefore(this.previous)) { result = this.previous; } this.previous = result; - return result; + return result.clone();src/room/data-track/depacketizer.ts (2)
210-210: Use strict equality (===) instead of loose equality (==).♻️ Minor fix
- if (this.partial.payloads.size == DataTrackDepacketizer.MAX_BUFFER_PACKETS) { + if (this.partial.payloads.size === DataTrackDepacketizer.MAX_BUFFER_PACKETS) {
172-181: Consider cloningstartSequenceto prevent aliasing.Line 177 stores
packet.header.sequencedirectly. If the sameDataTrackPacketobject is reused or mutated externally, thestartSequenceinpartialcould be corrupted.♻️ Suggested fix
const startSequence = packet.header.sequence; const payloadLenBytes = packet.payload.length; this.partial = { frameNumber: packet.header.frameNumber.value, - startSequence, + startSequence: startSequence.clone(), extensions: packet.header.extensions, payloads: new Map([[startSequence.value, packet.payload]]), payloadLenBytes, };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.changeset/busy-ends-shake.md.changeset/tiny-lamps-hang.md.github/workflows/test.yamlpackage.jsonsrc/index.tssrc/logger.tssrc/room/data-track/depacketizer.test.tssrc/room/data-track/depacketizer.tssrc/room/data-track/frame.tssrc/room/data-track/handle.test.tssrc/room/data-track/handle.tssrc/room/data-track/packet/constants.tssrc/room/data-track/packet/errors.tssrc/room/data-track/packet/extensions.tssrc/room/data-track/packet/index.test.tssrc/room/data-track/packet/index.tssrc/room/data-track/packet/serializable.tssrc/room/data-track/packetizer.test.tssrc/room/data-track/packetizer.tssrc/room/data-track/utils.test.tssrc/room/data-track/utils.tssrc/room/errors.tssrc/utils/TypedPromise.tssrc/utils/throws.tsthrows-transformer/README.mdthrows-transformer/cli.tsthrows-transformer/engine.tsthrows-transformer/plugin.tsthrows-transformer/transformer.tstsconfig.json
🧰 Additional context used
🧠 Learnings (1)
📚 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/transformer.tsthrows-transformer/README.mdpackage.jsonthrows-transformer/engine.tstsconfig.jsonthrows-transformer/plugin.tsthrows-transformer/cli.ts
🧬 Code graph analysis (10)
throws-transformer/transformer.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
src/room/data-track/packet/index.test.ts (5)
src/room/data-track/packet/index.ts (2)
DataTrackPacketHeader(38-287)DataTrackPacket(302-363)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(10-76)DataTrackTimestamp(78-113)src/room/data-track/packet/extensions.ts (3)
DataTrackExtensions(133-256)DataTrackUserTimestampExtension(18-69)DataTrackE2eeExtension(71-131)src/room/data-track/packet/constants.ts (1)
EXT_FLAG_SHIFT(24-24)
src/room/data-track/depacketizer.test.ts (3)
src/room/data-track/depacketizer.ts (1)
DataTrackDepacketizer(90-268)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (1)
WrapAroundUnsignedInt(10-76)
throws-transformer/plugin.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
src/room/data-track/packetizer.test.ts (4)
src/room/data-track/packetizer.ts (1)
DataTrackPacketizer(43-128)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (1)
DataTrackTimestamp(78-113)src/room/data-track/frame.ts (1)
DataTrackFrame(5-8)
src/room/data-track/handle.ts (2)
src/utils/throws.ts (1)
Throws(14-16)src/room/data-track/utils.ts (1)
U16_MAX_SIZE(1-1)
src/room/data-track/handle.test.ts (1)
src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)
src/room/data-track/utils.test.ts (1)
src/room/data-track/utils.ts (1)
WrapAroundUnsignedInt(10-76)
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 (3)
WrapAroundUnsignedInt(10-76)U16_MAX_SIZE(1-1)coerceToDataView(182-196)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)
DataTrackSerializeError(89-117)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(14-16)src/room/data-track/packet/errors.ts (1)
DataTrackDeserializeError(13-74)src/room/data-track/utils.ts (1)
coerceToDataView(182-196)
🪛 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)
src/room/data-track/depacketizer.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/packetizer.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/handle.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/utils.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
🪛 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 (45)
package.json (1)
52-52: Looks good for the new throws check wiring.
The script entry and supporting dependency addition align with the tooling flow.Also applies to: 102-102
tsconfig.json (1)
12-13: Compiler options update looks good.Also applies to: 28-29
.github/workflows/test.yaml (1)
29-31: CI step addition looks good.Also applies to: 38-38
src/utils/throws.ts (1)
14-42: Type utility definitions are clear and cohesive.throws-transformer/cli.ts (1)
14-96: LGTM!The CLI implementation is well-structured with proper tsconfig handling, clear error messages, and appropriate exit codes. The color-coded output is a nice touch for usability.
throws-transformer/plugin.ts (1)
42-51: LGTM!The proxy creation pattern using
Object.keysand binding methods is the standard approach for TypeScript language service plugins. The error handling that falls back to original diagnostics (lines 82-84) is a good defensive pattern.src/room/errors.ts (5)
3-19: LGTM!Good addition of the
causeproperty following the standard Error cause pattern with proper MDN documentation reference. The conditional assignment on line 15-17 correctly handles theundefinedcheck.
21-29: Clean abstraction for reasoned errors.The
LivekitReasonedError<Reason>abstract class provides a consistent pattern for errors that carry a typed reason. The abstractreasonandreasonNameproperties ensure implementing classes provide both the enum value and its string representation.
108-132: LGTM!The
ConnectionErrorclass correctly extendsLivekitReasonedErrorwith the discriminated union pattern. The static factory methods provide type-safe construction for each variant.
254-266: LGTM!
SignalRequestErrorcorrectly implements the reasoned error pattern with proper handling of both string ('TimeoutError') and enum reasons via the conditional on line 264.
292-304: LGTM!
DataStreamErrorfollows the same consistent pattern as the other reasoned errors..changeset/busy-ends-shake.md (1)
1-5: LGTM!Changeset correctly documents the patch-level addition of data track packet serialization and deserialization logic.
.changeset/tiny-lamps-hang.md (1)
1-5: LGTM!Changeset correctly documents the packetizer and depacketizer implementations as a separate patch entry.
src/logger.ts (1)
24-24: LGTM!The new
DataTrackslogger name follows the existing naming convention and will be properly included in thelivekitLoggerscollection for log level management.src/room/data-track/handle.test.ts (1)
6-12: LGTM!The test correctly validates the
DataTrackHandle.fromNumberbehavior for valid inputs and both error conditions (reserved value0and values exceedingU16_MAX_SIZE).src/room/data-track/frame.ts (1)
4-8: LGTM!The
DataTrackFrametype is well-defined with appropriate types for binary payload handling and extension data.src/room/data-track/utils.test.ts (1)
5-27: LGTM!Tests comprehensively cover
WrapAroundUnsignedIntbehavior including initialization, positive wrap-around at the upper bound, and negative wrap-around at zero. The wrap-around arithmetic is correctly verified.src/room/data-track/packetizer.test.ts (3)
11-83: LGTM!Excellent test coverage for the packetizer. The test correctly validates:
- Payload splitting (300 bytes → 4 packets of 88, 88, 88, 36 bytes)
- Frame marker progression (Start → Inter → Inter → Final)
- Header field consistency (sequence, frameNumber, timestamp, trackHandle, extensions)
85-119: LGTM!Good parameterized test coverage for edge cases including zero payload, single packet, and multi-packet scenarios with different MTU sizes. The header field validation loop ensures consistent behavior across all cases.
121-130: LGTM!The
computeFrameMarkerutility is well-tested with all marker types: Single (1 packet), Start (first of many), Inter (middle), and Final (last of many).src/index.ts (1)
152-157: LGTM!The new data track exports are well-organized with proper type-only exports for
DataTrackPacketHeader,DataTrackExtensions,DataTrackUserTimestampExtension, andDataTrackE2eeExtension.src/room/data-track/depacketizer.test.ts (6)
10-27: Single‑packet depacketization test looks solid.
80-112: Interrupted‑frame error case is well asserted.
114-144: Incomplete‑frame error path is validated clearly.
146-162: Unknown‑frame guard test is solid.
164-208: Buffer‑full scenario is exercised properly.
1-6: No action required.vitestis properly installed in devDependencies (^3.0.0) and present in the lockfile, making it resolvable by the import-x resolver. No ESLint configuration changes are needed.Likely an incorrect or invalid review comment.
src/room/data-track/packet/errors.ts (1)
1-121: Error enums and factory helpers are consistent and clear.src/room/data-track/packet/index.test.ts (4)
14-290: Serialization test vectors are thorough.
292-552: Deserialization coverage and error cases look solid.
554-613: Round‑trip test confirms end‑to‑end parity.
1-13: No action needed.vitestis properly configured in devDependencies ("vitest": "^3.0.0"). The ESLint configuration disablesimport-x/no-extraneous-dependenciesfor the entire project (viastrictness = 'off'), so there is no unresolved import error for vitest imports in test files.Likely an incorrect or invalid review comment.
src/room/data-track/handle.ts (2)
5-42: Handle error taxonomy is well‑structured.
68-79: Allocator logic is straightforward and aligned with u16 bounds.src/room/data-track/packetizer.ts (1)
14-71: Packetizer error type and frame‑marker helper look good.src/room/data-track/packet/constants.ts (1)
1-27: LGTM!The constants are well-organized and correctly defined for the binary serialization protocol. The bitfield masks, shifts, and frame marker values align with typical RTP-style header encoding conventions.
src/room/data-track/packet/index.ts (4)
38-66: LGTM!The
DataTrackPacketHeaderconstructor and field definitions are well-structured. The default emptyDataTrackExtensionsfor the optionalextensionsparameter is a sensible default.
87-166: LGTM!The
toBinaryIntomethod correctly implements the binary serialization:
- Proper buffer size validation upfront
- Correct bitfield encoding for version, marker, and extension flag
- RTP-oriented extension length encoding is well-documented
- Final length validation provides a good invariant check
168-275: LGTM!The
fromBinarydeserialization correctly:
- Validates minimum buffer length before parsing
- Handles version checking with forward compatibility (
>instead of!==)- Properly converts
DataTrackHandleErrortoDataTrackDeserializeError- Reverses the RTP-oriented extension length encoding
289-299: LGTM!The
FrameMarkerenum provides clear semantic values for internal use, with proper mapping to/from wire format constants in the serialization methods.src/room/data-track/utils.ts (2)
10-76: LGTM!The
WrapAroundUnsignedIntclass correctly implements wrap-around arithmetic for JavaScript's lack of fixed-size integers. The validation in the constructor ensures safe bounds, and theclamp()method properly handles both overflow and underflow.
182-196: LGTM!The
coerceToDataViewhelper correctly handles all three input types, properly accounting forUint8Array'sbyteOffsetandbyteLengthwhen creating theDataView.src/room/data-track/packet/extensions.ts (2)
18-69: LGTM!The
DataTrackUserTimestampExtensioncorrectly implements the RTP-style length encoding where the wire format storeslengthBytes - 1. The serialization and length calculations are consistent.
71-131: LGTM!The
DataTrackE2eeExtensioncorrectly serializes the key index and 12-byte IV with proper length encoding.src/room/data-track/depacketizer.ts (1)
24-74: LGTM!The
DataTrackDepacketizerDropErrorclass provides clear, structured error reporting with specific reasons and factory methods. IncludingframeNumberin the error is helpful for debugging.
✏️ 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: 7
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 152-159: Decide whether DataTrackPacketizer and
DataTrackDepacketizer should be part of the public API and update the exports
accordingly: if they are intended to be public, remove the FIXME comments and
add a clear JSDoc/TSDoc on the source symbols (DataTrackPacketizer,
DataTrackDepacketizer) describing stability and usage so the index export is
intentional; if they should remain internal, remove these two export lines from
src/index.ts and instead keep the classes exported only from their module files
with an `@internal` TSDoc tag (or mark them non-exported) so they are not
re-exported from the public barrel. Ensure you reference the exact symbols
DataTrackPacketizer and DataTrackDepacketizer when making the change so the
public surface is explicitly decided.
In `@src/room/data-track/packet/extensions.ts`:
- Around line 235-242: The default branch that skips unknown extensions advances
byteIndex by lengthBytes but misses the RTP encoding offset (wire length =
actualLength - 1); update the increment in that default case to advance by
lengthBytes + 1 (or lengthBytes + U8_LENGTH_BYTES if that constant exists) so
byteIndex correctly skips the full extension payload, and keep the existing
malformedExt(tag) check intact (reference the default switch branch, byteIndex,
lengthBytes, and DataTrackDeserializeError.malformedExt).
In `@src/room/data-track/packet/serializable.ts`:
- Around line 13-27: The toBinary() implementation currently returns new
Uint8Array(output) but has a FIXME asking whether to return a Uint8Array or
ArrayBuffer; decide and make the choice consistent: if the public API should be
bytes, keep returning Uint8Array and ensure the signature (toBinary():
Throws<Uint8Array, DataTrackSerializeErrorAll>) and JSDoc state "returns a
Uint8Array" and remove the FIXME; if the API should expose the underlying buffer
for zero-copy, change the implementation to return the ArrayBuffer (and update
the signature to Throws<ArrayBuffer, DataTrackSerializeErrorAll> and JSDoc
accordingly); if undecided, move the question to a tracked issue and add a short
TODO comment referencing that issue while keeping the current return and
signature consistent. Ensure to update references to toBinary(), and document
the behavior of toBinaryLengthBytes() / toBinaryInto() expectations in the same
JSDoc.
In `@src/room/data-track/packetizer.test.ts`:
- Around line 85-119: The test doesn't explicitly assert zero-length payloads
produce no packets, so add an explicit check after creating packets that if
payloadSizeBytes === 0 then assert packets.length === 0 (with a clear message)
and return/exit the test early; locate this change around the
DataTrackPacketizer.packetize invocation and the DataTrackFrame construction to
ensure zero-length payloads are asserted before iterating packets.
In `@throws-transformer/cli.ts`:
- Around line 24-33: The CLI currently collects matches into the files array
(const files = args.flatMap((f) => globSync(f))) but doesn't fail when that
array is empty; add a guard right after files is computed that checks if
files.length === 0, prints a clear error (e.g., console.error or
processLogger.error) stating that no files matched the provided globs/args, and
exit with a non‑zero status (process.exit(1)); keep this check near the files
declaration so it runs before attempting to locate tsconfig (tsconfigPath /
ts.findConfigFile) or proceeding further.
In `@throws-transformer/README.md`:
- Line 241: Rename the type alias Primatives to Primitives and update all
references to it (e.g., the type declaration "type Primatives" and any usages
such as the reference on the later line that currently uses Primatives) so the
identifier is spelled "Primitives"; ensure you update the definition and the
usage(s) (including the place mentioned on line 246) to match the corrected
name.
In `@throws-transformer/transformer.ts`:
- Around line 25-36: The TransformerConfig.strictCatchHandling flag is declared
but never read; update the transformer factory (export default function
transformer(program, config)) to read config?.strictCatchHandling (e.g., const
strictCatchHandling = config?.strictCatchHandling ?? false) and pass that
boolean into the internal transform engine/visitor so the behavior is honored
(or, if there is no engine entrypoint, thread the flag into the inner
transformer closure that visits SourceFiles/Nodes). Alternatively, if you prefer
not to support the option yet, remove strictCatchHandling from the
TransformerConfig and any docs to avoid a misleading API; reference
TransformerConfig.strictCatchHandling and the transformer(program, config)
signature when making the change.
🧹 Nitpick comments (6)
src/room/data-track/handle.test.ts (1)
1-3: Unused imports can be removed.
afterEach,beforeEach, andviare imported but not used in this test file. Consider removing them for cleaner code.Proposed fix
/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { DataTrackHandle } from './handle';src/room/data-track/utils.test.ts (1)
1-3: Unused imports can be removed.
afterEach,beforeEach, andviare imported but not used. Consider cleaning up for consistency.Proposed fix
/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { WrapAroundUnsignedInt } from './utils';src/room/data-track/depacketizer.test.ts (1)
1-8: Unused imports can be removed.
afterEach,beforeEach, andviare imported but not used.Proposed fix
/* eslint-disable `@typescript-eslint/no-unused-vars` */ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { DataTrackDepacketizer } from './depacketizer';src/room/data-track/handle.ts (1)
47-61: Consider rejecting non‑uint16 inputs infromNumber.
Only0and> U16_MAX_SIZEare rejected (Line 54-58), so negative or non‑integer values could produce invalid handles if this API is called with arbitrary input. Consider adding a lower‑bound/Number.isIntegerguard or documenting the uint16 requirement.src/room/data-track/utils.ts (1)
41-48: Consider using modulo for clamping instead of while loops.The
clamp()method uses while loops which could iterate multiple times for large values. While the input bounds limit practical risk, modulo arithmetic would be more efficient and clearer.Suggested optimization
clamp() { - while (this.value > this.maxSize) { - this.value -= this.maxSize + 1; - } - while (this.value < 0) { - this.value += this.maxSize + 1; - } + const range = this.maxSize + 1; + this.value = ((this.value % range) + range) % range; }src/room/data-track/depacketizer.ts (1)
210-212: Use strict equality operator.Line 210 uses
==instead of===for the size comparison. While functionally equivalent here (both are numbers), strict equality is preferred for consistency.- if (this.partial.payloads.size == DataTrackDepacketizer.MAX_BUFFER_PACKETS) { + if (this.partial.payloads.size === DataTrackDepacketizer.MAX_BUFFER_PACKETS) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.changeset/busy-ends-shake.md.changeset/tiny-lamps-hang.md.github/workflows/test.yamlpackage.jsonsrc/index.tssrc/logger.tssrc/room/data-track/depacketizer.test.tssrc/room/data-track/depacketizer.tssrc/room/data-track/frame.tssrc/room/data-track/handle.test.tssrc/room/data-track/handle.tssrc/room/data-track/packet/constants.tssrc/room/data-track/packet/errors.tssrc/room/data-track/packet/extensions.tssrc/room/data-track/packet/index.test.tssrc/room/data-track/packet/index.tssrc/room/data-track/packet/serializable.tssrc/room/data-track/packetizer.test.tssrc/room/data-track/packetizer.tssrc/room/data-track/utils.test.tssrc/room/data-track/utils.tssrc/room/errors.tssrc/utils/TypedPromise.tssrc/utils/throws.tsthrows-transformer/README.mdthrows-transformer/cli.tsthrows-transformer/engine.tsthrows-transformer/plugin.tsthrows-transformer/transformer.tstsconfig.json
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:11.307Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:28.111Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:46.510Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
📚 Learning: 2026-01-28T02:36:11.307Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:11.307Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Applied to files:
.changeset/tiny-lamps-hang.mdsrc/room/data-track/packetizer.test.tssrc/room/data-track/depacketizer.test.tssrc/index.ts.changeset/busy-ends-shake.mdsrc/room/data-track/packet/index.test.tssrc/room/data-track/packet/serializable.tssrc/room/data-track/packetizer.tssrc/room/data-track/packet/index.tssrc/room/data-track/frame.tssrc/room/data-track/depacketizer.tssrc/room/data-track/packet/extensions.tssrc/room/data-track/utils.tssrc/room/data-track/packet/constants.tssrc/room/data-track/packet/errors.ts
📚 Learning: 2026-01-28T02:37:28.111Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:28.111Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Applied to files:
src/room/data-track/handle.test.tssrc/room/data-track/depacketizer.test.tssrc/room/data-track/utils.test.tssrc/room/data-track/packet/serializable.tssrc/room/data-track/packetizer.tssrc/room/data-track/depacketizer.tssrc/room/data-track/packet/extensions.tssrc/room/data-track/handle.tssrc/room/data-track/packet/constants.tssrc/room/data-track/packet/errors.ts
📚 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/plugin.tsthrows-transformer/transformer.tsthrows-transformer/README.mdsrc/utils/throws.tsthrows-transformer/engine.tstsconfig.jsonthrows-transformer/cli.tspackage.json
📚 Learning: 2026-01-28T02:22:46.510Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:46.510Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
Applied to files:
src/room/data-track/depacketizer.test.tssrc/index.tssrc/room/data-track/packet/index.test.tssrc/room/data-track/packetizer.tssrc/room/data-track/packet/index.tssrc/room/data-track/frame.tssrc/room/data-track/depacketizer.tssrc/room/data-track/packet/extensions.tssrc/room/data-track/utils.tssrc/room/data-track/packet/constants.tssrc/room/data-track/packet/errors.ts
📚 Learning: 2026-01-28T02:37:28.111Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:28.111Z
Learning: In src/room/data-track/utils.ts, the isBefore() method of DataTrackTimestamp intentionally uses simple numeric comparison and is not wraparound-resilient, matching the approach of the Rust implementation being ported. When reviewing related changes, treat this as an intentional design choice rather than a bugfix; ensure tests and downstream code reflect that wraparound handling is not expected here, and avoid introducing wraparound-aware logic for this timestamp type unless the design is explicitly changed. This guideline applies specifically to this file (not a broad project-wide change).
Applied to files:
src/room/data-track/utils.ts
🧬 Code graph analysis (10)
src/room/data-track/handle.test.ts (1)
src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)
throws-transformer/plugin.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
throws-transformer/transformer.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
src/room/data-track/depacketizer.test.ts (3)
src/room/data-track/depacketizer.ts (1)
DataTrackDepacketizer(90-268)src/room/data-track/handle.ts (1)
DataTrackHandle(44-66)src/room/data-track/utils.ts (1)
WrapAroundUnsignedInt(10-76)
src/room/data-track/utils.test.ts (1)
src/room/data-track/utils.ts (1)
WrapAroundUnsignedInt(10-76)
src/room/data-track/packet/serializable.ts (2)
src/utils/throws.ts (1)
Throws(16-16)src/room/data-track/packet/errors.ts (1)
DataTrackSerializeErrorAll(119-121)
src/room/data-track/packet/index.ts (8)
src/index.ts (3)
DataTrackPacketHeader(152-152)DataTrackExtensions(154-154)DataTrackPacket(152-152)src/room/data-track/handle.ts (2)
DataTrackHandle(44-66)DataTrackHandleError(10-42)src/room/data-track/utils.ts (4)
WrapAroundUnsignedInt(10-76)U16_MAX_SIZE(1-1)DataTrackTimestamp(78-116)coerceToDataView(185-199)src/room/data-track/packet/extensions.ts (1)
DataTrackExtensions(133-256)src/room/data-track/packet/constants.ts (15)
BASE_HEADER_LEN(9-9)EXT_WORDS_INDICATOR_SIZE(23-23)SUPPORTED_VERSION(8-8)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/rpc.ts (1)
byteLength(150-153)src/version.ts (1)
version(3-3)
src/room/data-track/handle.ts (2)
src/utils/throws.ts (1)
Throws(16-16)src/room/data-track/utils.ts (1)
U16_MAX_SIZE(1-1)
throws-transformer/cli.ts (1)
throws-transformer/engine.ts (1)
checkSourceFile(30-80)
src/room/data-track/packet/errors.ts (1)
src/room/data-track/handle.ts (1)
DataTrackHandleError(10-42)
🪛 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/handle.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/packetizer.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/depacketizer.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/utils.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
src/room/data-track/packet/index.test.ts
[error] 2-2: Unable to resolve path to module 'vitest'.
(import-x/no-unresolved)
🪛 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 (36)
.github/workflows/test.yaml (1)
29-31: LGTM!The new "Throws check" step is well-positioned in the workflow—after linting/formatting checks and before running tests. This ensures the throws-transformer validation is part of the CI pipeline.
src/logger.ts (1)
24-24: LGTM!The new
DataTrackslogger follows the existing naming convention and will be automatically included in the logger collection viaObject.values(LoggerNames).map(...).src/room/errors.ts (2)
7-17: LGTM with a note oncauseproperty.The
causeproperty addition follows the MDN pattern and provides explicit typing. Note that modern JavaScript (ES2022+)Erroralready has a nativecauseproperty—this implementation shadows it for explicit control and backwards compatibility.
21-29: Well-designed error abstraction.The
LivekitReasonedError<Reason>abstract class provides a clean pattern for typed error variants with mandatoryreasonandreasonNameproperties. This improves error handling consistency across the codebase..changeset/tiny-lamps-hang.md (1)
1-5: LGTM!Changeset correctly describes the addition of data track packetizer and depacketizer implementations with appropriate patch versioning.
.changeset/busy-ends-shake.md (1)
1-5: LGTM!Changeset correctly describes the addition of data track packet serialization/deserialization logic with appropriate patch versioning.
src/room/data-track/handle.test.ts (1)
5-12: LGTM!The test correctly validates all three cases of
DataTrackHandle.fromNumber(): valid input, reserved value (0), and values exceeding U16_MAX_SIZE. The assertions align with the implementation inhandle.ts.tsconfig.json (2)
12-14: LGTM!Adding
ES2022.Errorenables theError.causeproperty for better error chaining, which aligns well with the structured error handling introduced by the throws-transformer.
28-29: LGTM!The throws-transformer plugin configuration using a relative file path is intentional and valid for VS Code workspace TypeScript versions. Based on learnings, this configuration approach is by design for this repository.
src/room/data-track/utils.test.ts (1)
5-27: LGTM!The tests thoroughly cover
WrapAroundUnsignedIntinitialization, error handling for negative values, and both positive and negative wrap-around behavior at the u16 boundary (65535). The test cases correctly verify the clamping logic.src/room/data-track/frame.ts (1)
1-8: LGTM!Clean type definition for
DataTrackFramethat pairs payload bytes with packet extensions. The JSDoc properly links toDataTrackPacketizerfor discoverability.package.json (2)
52-52: LGTM!The
throws:checkscript correctly targets non-test TypeScript files for the throws-transformer validation. Usingpnpm dlx tsxavoids adding tsx as a permanent dependency.
102-102: LGTM!Adding
globas a devDependency supports the throws-transformer CLI's pattern matching functionality.src/room/data-track/depacketizer.test.ts (5)
11-27: LGTM!The single-packet depacketization test correctly validates that a
FrameMarker.Singlepacket returns immediately with the expected payload and empty extensions.
80-112: LGTM!The "interrupted" test correctly validates that pushing a Start packet for a new frame while a partial frame is being assembled throws the expected error when
errorOnPartialFramesis enabled.
114-144: LGTM!The "incomplete" test correctly validates the error when a Final packet arrives with a sequence number indicating missing intermediate packets.
146-162: LGTM!The "unknownFrame" test correctly validates that receiving an Inter packet without a preceding Start packet throws the expected error.
164-208: LGTM!The "bufferFull" test correctly validates buffer overflow behavior. The sequence numbers are properly set: Start=0, Inter packets use sequences 1 through
MAX_BUFFER_PACKETS-1, and the Final packet uses sequenceMAX_BUFFER_PACKETS(no overlap).src/room/data-track/packetizer.test.ts (2)
11-83: Nice coverage for multi-packet fragmentation.
121-130: Frame marker utility tests look solid.src/utils/TypedPromise.ts (1)
17-22: Typedresolveoverloads look good.throws-transformer/plugin.ts (1)
35-86: Plugin diagnostics wiring looks good.src/room/data-track/packet/index.test.ts (3)
15-290: Serialization test coverage looks comprehensive.
292-552: Deserialization cases look thorough and well structured.
554-613: Round‑trip test is clear and complete.src/utils/throws.ts (1)
1-42: LGTM — clear, lightweight Throws type utilities.
The branded/phantom design keeps runtime values unchanged while enabling static error propagation.src/room/data-track/packet/constants.ts (1)
1-27: LGTM — clear, centralized serialization constants.
The constants set is organized and consistent.src/room/data-track/packet/errors.ts (1)
1-121: Well-structured error hierarchy for data track serialization.The error design follows the established
DataTrackHandleErrorpattern nicely with:
- Discriminated union types via reason enums
- Factory methods for clean instantiation
- Consistent error code (19) across data track errors
- Proper cause chaining for
InvalidHandlesrc/room/data-track/utils.ts (2)
161-182: Previous issues have been addressed.The elapsed time calculation (line 162) and nanosecond conversion (line 179) are now correct:
timestamp.getTime() - this.epoch.getTime()correctly computes elapsed timedurationMilliseconds * 1e6correctly converts to nanoseconds
185-199: Clean input coercion utility.The
coerceToDataViewfunction properly handles all three input types and correctly preservesbyteOffsetandbyteLengthforUint8Arrayinputs, which is important for sliced views.src/room/data-track/packet/index.ts (3)
38-166: Solid header serialization with proper RTP extension length encoding.The header serialization correctly handles:
- Version and marker bit packing
- Extension flag and RTP-oriented length encoding (
extensionsLengthWords - 1)- Padding alignment to 4-byte words
- Length validation with panic on mismatch
168-275: Header deserialization looks correct.The deserialization properly:
- Validates minimum length
- Checks version support
- Converts wire format marker back to enum
- Reverses the RTP extension length encoding (
+ 1)- Wraps handle errors appropriately
Minor observation: Line 267 uses non-null assertion
trackHandle!which is safe given the try-catch structure, but TypeScript's control flow analysis doesn't track it.
301-363: DataTrackPacket implementation is clean.The packet class properly combines header and payload with:
- Correct length calculation
- Sequential serialization
- Payload extraction via
buffer.slice(creates copy, which is safe)src/room/data-track/packetizer.ts (2)
41-71: JSDoc and computeFrameMarker look correct.The JSDoc now correctly references
DataTrackFrame(previously flagged as incorrect), andcomputeFrameMarkerproperly handles all cases including single-packet frames.
78-127: Packetization logic is correct.The implementation properly:
- Guards against MTU too small with
>=check (previously flagged infinite loop risk)- Produces no packets for zero-length payloads (intentional per Rust implementation)
- Increments sequence per-packet and frameNumber per-frame
- Computes correct frame markers based on packet position
Based on learnings: zero-length payloads should produce no packets, matching the Rust implementation.
src/room/data-track/depacketizer.ts (1)
90-119: Clean state machine routing in push().The push method correctly routes packets based on marker type, with appropriate error handling for each path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
That will mean that the packetizer can feed it views into a core ArrayBuffer rather than having to make slices (ie, copies) of a potentially lengthy ArrayBuffer.
I needed to clone a value and didn't.
| header: DataTrackPacketHeader; | ||
|
|
||
| payload: ArrayBuffer; | ||
| payload: Uint8Array; | ||
|
|
There was a problem hiding this comment.
A notable change - originally I had DataTrackPacket's payload field stored as an ArrayBuffer because I thought it would be the source of truth of data. However as I got further into the packetizer / depacketizer work I realized that having a DataTrackPacket be a view (ie, with Uint8Array) into the array buffer being received from high level abstractions (ie, in the future a network packet) leads to much less data copying, so I switched it here.
There was a problem hiding this comment.
Makes a lot of sense. No reason to move it into a new buffer when the cursor can just be moved to isolate the payload.
| }); | ||
|
|
||
| it('should throw "bufferFull" when too many packets have been sent', () => { | ||
| const depacketizer = new DataTrackDepacketizer(); |
There was a problem hiding this comment.
FYI @ladvoc I added a few more tests in here than the corresponding rust version has. It might be worth porting the last few over - it just exercises all the error cases.
| push( | ||
| packet: DataTrackPacket, | ||
| options?: PushOptions, | ||
| ): Throws< | ||
| DataTrackFrame | null, | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.Interrupted> | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.BufferFull> | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.UnknownFrame> | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.Incomplete> | ||
| > { |
There was a problem hiding this comment.
An important deviation from how the rust implementation works - the rust implementation returns a DepacketizerPushResult here which contains frame and drop_error fields (both Options, so they are nullable). Note that either one can be set, or in one case (a packet being received from a different frame "interrupting" processing) both can be set.
Instead the web implementation opts to get rid of this DepacketizerPushResult layer and just return a DataTrackFrame | null. In all error cases the web implementation opts to throw instead, and for that one case where both can be set, the web implementation exposes an option passed to push that allows the caller to choose if they want to allow new packets to come in and overwrite an existing frame (errorOnPartialFrames). If this is true, an error is thrown, and if it is false, a warning is printed instead.
This option primarily exists for the tests right now but I could also see maybe being useful for like verifying idealized packetizing/depacketizing workflows where packet loss won't be a thing elsewhere too or something like that.
I think there could also be an argument for getting rid of the error all together and just making it a warning always if others think that is worthwhile to reduce complexity.
IMO the rust implementation should adopt this approach as well but I will leave that up to @ladvoc.
There was a problem hiding this comment.
Taking another look at this, I wanted to mention a third possibility. Push could return a complete frame, if one is available, but the drop error could be exposed through a property on the depacketizer (e.g., depacketizer.last_push_error()). If the consumer is interested, they can access this property after calling push.
There was a problem hiding this comment.
I think I'd be fine with your suggestion too, though I think my approach is a little more js-idiomatic. Do you see there being many more of these "a warning was raised during depacketizing" types of cases to worry about?
Another option too - at least for this one "interrupted" warning, I'm not really sure how important it is to really return it as an error in the majority of cases. Assuming there aren't more of these cases that a more advanced depacketizer would encounter / this is truly a special case, it could just be logged as a warning and that's it.
| export type DataTrackFrame = { | ||
| payload: ArrayBuffer; | ||
| extensions: DataTrackExtensions; | ||
| }; |
There was a problem hiding this comment.
FYI @ladvoc I have adopted the common data frame frame implementation as we discussed in a 1:1 rather than the packetize / depacketize specific implementations in rust. IMO it would be worth combining these in the rust implementation as well.
There was a problem hiding this comment.
Yes, that makes a lot of sense. However, it is worth calling out that this type should not be exposed in the public API, since the concept of extensions is an implementation detail. For example, E2EE metadata should not be accessible through the public frame type, and if we introduce more extensions in the future that control internal processing (e.g., a frame is marked as being compressed), these should also remain internal details.
There was a problem hiding this comment.
Good point - that being said, right now DataTrackFrame isn't exported by the package, only the frame.ts module. So I think this should be fine.
| static rtpRandom() { | ||
| // FIXME: does this need to be a higher quality PRNG? | ||
| const randomValue = Math.round(Math.random() * U32_MAX_SIZE); | ||
| return DataTrackTimestamp.fromRtpTicks(randomValue); |
There was a problem hiding this comment.
@ladvoc I am curious your thoughts on this - when initializing the rtp clock in the packetizer with a random timestamp, how high quality does the PRNG need to be? Does it need to be cryptography grade or is Math.random (more info about its pitfalls) good enough?
There was a problem hiding this comment.
I think the main reason RTP does this is to distinguish between streams rather than for security. Additionally, even if E2EE is not enabled for a track, track packets are still encrypted in transport between the SFU and clients, so I am not too concerned about making this cryptographic grade. However, I will do a bit more research on this before giving a definitive answer.
There was a problem hiding this comment.
I wonder if it is better with crypto.getRandomValues ? or it is more or less the same
There was a problem hiding this comment.
That was largely my question for jacob, yes, and if we needed this to be crypto-secure that's probably what I would use!
cc @ladvoc - I'm curious if you ended up digging into this any more. If you are unsure / want to be safe, I can migrate it over.
| @@ -11,7 +13,7 @@ | |||
| * | |||
| * For more info about how this is checked, see ./throws-transformer at the root of this repo. | |||
| */ | |||
| export type Throws<T, E extends Error> = T & { readonly __throws?: E }; | |||
| export type Throws<T, E extends Error> = (T & { readonly __throws?: E }) | Extract<T, Primitives>; | |||
There was a problem hiding this comment.
A small error handling logic update - with the above, Throws<{ key: "value" } | null, /* ... */> now works. Previously if you put a non object into the first parameter of Throws the branding would get all weird (and would sometimes get dropped all together or parts of the passed union which weren't objects would get dropped depending on the exact circumstance).
I don't think this is 100% required but the LLM recommended it and I think returning these values which can be mutated link this out of band is probably a future footgun.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/room/data-track/depacketizer.ts`:
- Line 151: The DataTrackFrame.payload type (declared in DataTrackFrame) is
ArrayBuffer but depacketizer returns a Uint8Array (packet.payload), causing a
type/API mismatch; either change DataTrackFrame.payload to Uint8Array or convert
the Uint8Array to a fresh ArrayBuffer before returning in the depacketizer
functions (the return at the line returning { payload: packet.payload, ... } and
the similar return around line 264). Locate the depacketizer functions in
src/room/data-track/depacketizer.ts and replace the direct packet.payload return
with a conversion that produces an ArrayBuffer representing exactly the view's
bytes (or update the DataTrackFrame type to Uint8Array and update consumers
accordingly).
- Around line 218-219: The code currently always increments
this.partial.payloadLenBytes when inserting into this.partial.payloads using the
key packet.header.sequence.value, which double-counts retransmitted/duplicate
packets; change the insertion logic to first check whether
this.partial.payloads.has(packet.header.sequence.value) — if the key is absent,
add packet.payload.length to this.partial.payloadLenBytes and set the map entry;
if the key is present, overwrite the map entry without changing payloadLenBytes
(or, if you prefer preserving original size, subtract the old payload length
then add the new one using this.partial.payloads.get(...) before set). Ensure
this logic is applied where packet.payloads are handled (the code touching
this.partial.payloadLenBytes and this.partial.payloads.set).
- Around line 221-223: The finalize call in depacketizer where it returns a
completed frame (if packet.header.marker === FrameMarker.Final) leaves
this.partial populated; change the code in that branch to capture the result of
this.finalize(this.partial, packet.header.sequence.value), then clear
this.partial (set to undefined/null) before returning the result so subsequent
packets aren't treated as interrupted; update the same pattern anywhere else
finalize is called to ensure partial is cleared on successful completion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/room/data-track/depacketizer.tssrc/room/data-track/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/room/data-track/utils.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
📚 Learning: 2026-01-28T02:36:18.432Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Applied to files:
src/room/data-track/depacketizer.ts
📚 Learning: 2026-01-28T02:22:50.735Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
Applied to files:
src/room/data-track/depacketizer.ts
📚 Learning: 2026-01-28T02:37:34.704Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Applied to files:
src/room/data-track/depacketizer.ts
🧬 Code graph analysis (1)
src/room/data-track/depacketizer.ts (4)
src/index.ts (5)
getLogger(120-120)LoggerNames(103-103)DataTrackExtensions(154-154)DataTrackDepacketizer(159-159)DataTrackPacket(152-152)src/logger.ts (1)
getLogger(50-54)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(10-76)U16_MAX_SIZE(1-1)src/room/data-track/frame.ts (1)
DataTrackFrame(5-8)
⏰ 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: package-size
🔇 Additional comments (1)
src/room/data-track/depacketizer.ts (1)
24-73: Well-structured drop error helpers.Typed reasons and static constructors keep drop handling concise and consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This allows one instance to be used for a bunch of frames in series
ladvoc
left a comment
ladvoc
left a comment
There was a problem hiding this comment.
I left some initial comments but am planning to make another pass.
src/index.ts
Outdated
| type DataTrackUserTimestampExtension, | ||
| type DataTrackE2eeExtension, | ||
| } from './room/data-track/packet/extensions'; | ||
| export { DataTrackPacketizer } from './room/data-track/packetizer'; // FIXME: should this be exported? Maybe not? |
There was a problem hiding this comment.
comment: If you mean exported publicly, then no, this should be an internal implementation detail.
There was a problem hiding this comment.
Makes sense. For now I will swap them out with imports (more info) so they will continue to get checked by static analysis tools. Once I'm importing these in code that is already exported I will get rid of these.
| push( | ||
| packet: DataTrackPacket, | ||
| options?: PushOptions, | ||
| ): Throws< | ||
| DataTrackFrame | null, | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.Interrupted> | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.BufferFull> | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.UnknownFrame> | ||
| | DataTrackDepacketizerDropError<DataTrackDepacketizerDropReason.Incomplete> | ||
| > { |
There was a problem hiding this comment.
Taking another look at this, I wanted to mention a third possibility. Push could return a complete frame, if one is available, but the drop error could be exposed through a property on the depacketizer (e.g., depacketizer.last_push_error()). If the consumer is interested, they can access this property after calling push.
For now, these need to be included in the bundle so they are static analyzed by tsc / eslint / etc.
1egoman
force-pushed
the
data-tracks-packetizer
branch
from
bce1cee to
e53136e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/room/data-track/depacketizer.ts`:
- Around line 278-282: When finalize() can throw
DataTrackDepacketizerDropError.incomplete after mutating partial state, reset
the depacketizer's mutated frame state before throwing to avoid poisoning later
frames: clear this.partial (set to undefined/null) and any related payload
containers (e.g., this.payloads or fields on the partial that were modified)
prior to calling DataTrackDepacketizerDropError.incomplete so subsequent frames
start from a clean state; you can implement this inline immediately before the
throw or extract a small resetPartialState() helper and call it there.
- Around line 211-223: The capacity check currently runs before duplicate
detection which causes retransmitted packets for an existing sequence to trigger
DataTrackDepacketizerDropError.bufferFull even though the unique payload count
wouldn’t change; update the logic in DataTrackDepacketizer so you first check if
this.partial.payloads.has(packet.header.sequence.value) and if so replace the
entry (log the duplicate) and return, otherwise then check if
this.partial.payloads.size >= DataTrackDepacketizer.MAX_BUFFER_PACKETS and throw
DataTrackDepacketizerDropError.bufferFull(this.partial.frameNumber) only for
truly new packets before calling this.partial.payloads.set(...).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/room/data-track/depacketizer.test.tssrc/room/data-track/depacketizer.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
📚 Learning: 2026-01-28T02:36:18.432Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Applied to files:
src/room/data-track/depacketizer.tssrc/room/data-track/depacketizer.test.ts
📚 Learning: 2026-01-28T02:22:50.735Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
Applied to files:
src/room/data-track/depacketizer.ts
📚 Learning: 2026-01-28T02:37:34.704Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Applied to files:
src/room/data-track/depacketizer.tssrc/room/data-track/depacketizer.test.ts
🔇 Additional comments (13)
src/room/data-track/depacketizer.ts (5)
22-71: Well-structured drop error helpers.Clear reasons and constructors make debugging and handling straightforward.
75-80: Drop-reason enum is clean and clear.
99-118: Marker dispatch is concise and readable.
124-150: Single-packet fast path looks solid.
152-185: Partial-frame initialization is correct and tidy.src/room/data-track/depacketizer.test.ts (8)
11-27: Single-packet case coverage looks good.
29-78: Multi-packet assembly test is thorough and parameterized well.
80-112: Interrupted-frame error path is asserted cleanly.
114-144: Incomplete-finalization scenario is well covered.
146-162: Unknown-frame error handling test is solid.
164-208: Buffer-full guard is exercised correctly.
210-282: Two-frame flow validates state continuity nicely.
284-334: Duplicate-packet overwrite behavior is verified clearly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
(github actions are down right now, that is what is causing all the ci failures) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/room/data-track/depacketizer.ts`:
- Around line 164-173: The partial-frame cleanup isn't performed before throwing
an Interrupted error in the same way as in frameFromSingle: ensure you call
this.reset() before throwing
DataTrackDepacketizerDropError.interrupted(this.partial.frameNumber) (and in the
non-throwing path preserve the existing log then reset), so both branches clear
this.partial; update the block in depacketizer methods that reference
this.partial and DataTrackDepacketizerDropError.interrupted to invoke
this.reset() prior to throwing to keep behavior consistent with frameFromSingle.
- Around line 138-147: The handler for interrupted partial frames currently
throws DataTrackDepacketizerDropError.interrupted(this.partial.frameNumber)
before calling this.reset(), leaving internal state dirty; modify the branch in
the depacketizer (the check using this.partial and
options?.errorOnPartialFrames) to call this.reset() first and then throw the
Interrupted error (or alternatively clear this.partial before throwing) so the
depacketizer state is cleared even when callers catch the exception.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/room/data-track/depacketizer.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
📚 Learning: 2026-01-28T02:36:18.432Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Applied to files:
src/room/data-track/depacketizer.ts
📚 Learning: 2026-01-28T02:22:50.735Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
Applied to files:
src/room/data-track/depacketizer.ts
📚 Learning: 2026-01-28T02:37:34.704Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Applied to files:
src/room/data-track/depacketizer.ts
🧬 Code graph analysis (1)
src/room/data-track/depacketizer.ts (5)
src/index.ts (4)
getLogger(124-124)LoggerNames(107-107)DataTrackExtensions(158-158)DataTrackPacket(156-156)src/logger.ts (1)
getLogger(50-54)src/room/data-track/utils.ts (2)
WrapAroundUnsignedInt(10-76)U16_MAX_SIZE(1-1)src/utils/throws.ts (1)
Throws(16-16)src/room/data-track/frame.ts (1)
DataTrackFrame(5-8)
🔇 Additional comments (6)
src/room/data-track/depacketizer.ts (6)
1-10: LGTM!Imports are minimal and appropriate. Logger setup follows the codebase pattern.
11-21: LGTM!The
PartialFrametype is well-documented with clear field purposes. Using aMap<number, Uint8Array>keyed by sequence value provides efficient O(1) payload lookup.
22-72: LGTM!The error class is well-structured with:
- Typed
Reasongeneric for type-safe error discrimination- Convenient static factory methods for common error cases
- Human-readable
reasonNamederived from the enum for debugging
74-86: LGTM!The enum has explicit numeric values for serialization stability, and
PushOptionsis minimal and well-documented.
88-122: LGTM!The
pushmethod cleanly routes packets based on their marker type, with comprehensiveThrowstype annotations documenting all possible error conditions. Theresetmethod is simple and appropriate.
212-226: Acknowledged tradeoff documented.The NOTE comment at lines 212-213 appropriately documents that the buffer-full check happens before duplicate detection, which could block reprocessing duplicates when the buffer is full. This is a reasonable tradeoff that's now explicitly documented.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This ensures that a depacketizer never gets into a broken state.
xianshijing-lk
left a comment
xianshijing-lk
left a comment
There was a problem hiding this comment.
some nits, look good in general
| let elapsedMs = timestamp.getTime() - this.epoch.getTime(); | ||
| let durationTicks = DataTrackClock.durationInMsToTicks(elapsedMs, this.rateInHz); | ||
|
|
||
| let result = this.base.clone(); |
There was a problem hiding this comment.
curiously, any way to avoid the clone ? Note, in this function, there are quite some clones, I wonder if we can avoid some of them ?
There was a problem hiding this comment.
I had the same concern when writing this, FWIW. I ended up not being too worried about this in the end because in this case a "clone" is super cheap, just copying the inner value and wrapping it in a new class instance.
I could maybe refactor this so that every DataTrackTimestamp value is immutable / always returns a copy of itself, but that gets annoying with methods like getThenIncrement and when I was writing this initially I liked the idea much more of making them all homogeneous rather than making some methods mutable and some immutable.
Definitely open to other ideas here, but at least as of now I think I've picked the "least bad" option out of what came to mind.
| static rtpRandom() { | ||
| // FIXME: does this need to be a higher quality PRNG? | ||
| const randomValue = Math.round(Math.random() * U32_MAX_SIZE); | ||
| return DataTrackTimestamp.fromRtpTicks(randomValue); |
There was a problem hiding this comment.
I wonder if it is better with crypto.getRandomValues ? or it is more or less the same
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/room/data-track/utils.ts`:
- Around line 77-83: The isBefore method in WrapAroundUnsignedInt incorrectly
uses this.maxSize as the modulus and threshold; change the logic to compute
modulus = this.maxSize + 1 (the full range), compute diff = (b - a) >>> 0 mod
modulus, and then return diff !== 0 && diff <= modulus / 2 (i.e. less than or
equal to the half-range) so ordering respects wraparound; update the isBefore
implementation (referencing function WrapAroundUnsignedInt.isBefore and
variables a, b, diff, this.maxSize) accordingly.
- Around line 118-121: DataTrackTimestamp.isBefore currently delegates to
WrapAroundUnsignedInt.isBefore which makes it wraparound-aware; change it to a
plain numeric comparison to preserve intended semantics: inside
DataTrackTimestamp.isBefore compare the underlying numeric timestamp values on
this.timestamp and other.timestamp (referencing DataTrackTimestamp, isBefore and
timestamp) and return the boolean result instead of calling
WrapAroundUnsignedInt.isBefore.
🧹 Nitpick comments (1)
src/room/data-track/utils.ts (1)
95-99: Resolve or track theFIXMEinrtpRandom.
If this is shippable as-is, please replace the FIXME with a short rationale or link it to an issue. Happy to help draft a follow-up task.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/room/data-track/utils.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
📚 Learning: 2026-01-28T02:37:28.111Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:28.111Z
Learning: In src/room/data-track/utils.ts, the isBefore() method of DataTrackTimestamp intentionally uses simple numeric comparison and is not wraparound-resilient, matching the approach of the Rust implementation being ported. When reviewing related changes, treat this as an intentional design choice rather than a bugfix; ensure tests and downstream code reflect that wraparound handling is not expected here, and avoid introducing wraparound-aware logic for this timestamp type unless the design is explicitly changed. This guideline applies specifically to this file (not a broad project-wide change).
Applied to files:
src/room/data-track/utils.ts
📚 Learning: 2026-01-28T02:22:50.735Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
Applied to files:
src/room/data-track/utils.ts
📚 Learning: 2026-01-28T02:36:18.432Z
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Applied to files:
src/room/data-track/utils.ts
🔇 Additional comments (5)
src/room/data-track/utils.ts (5)
2-2: U32 helpers are a good addition.Also applies to: 19-21
50-75: Utility mutators/cloners look good.
101-116: Timestamp helpers are clear and consistent.
124-180: Clock construction and monotonicity handling look solid.
182-187: Tick conversion math looks correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /** Returns true if {@link this} is before the passed other {@link WrapAroundUnsignedInt}. */ | ||
| isBefore(other: WrapAroundUnsignedInt<MaxSize>) { | ||
| const a = this.value >>> 0; | ||
| const b = other.value >>> 0; | ||
| const diff = (b - a) >>> 0; | ||
| return diff !== 0 && diff < this.maxSize; | ||
| } |
There was a problem hiding this comment.
Fix wraparound ordering in WrapAroundUnsignedInt.isBefore.
Current logic compares diff to maxSize, which makes almost any non-equal pair return true and also computes the wrong modulus for u16. Use (maxSize + 1) as the modulus and compare against the half-range.
🛠️ Proposed fix
isBefore(other: WrapAroundUnsignedInt<MaxSize>) {
- const a = this.value >>> 0;
- const b = other.value >>> 0;
- const diff = (b - a) >>> 0;
- return diff !== 0 && diff < this.maxSize;
+ const modulus = this.maxSize + 1;
+ const diff = (other.value - this.value + modulus) % modulus;
+ return diff !== 0 && diff < modulus / 2;
}🤖 Prompt for AI Agents
In `@src/room/data-track/utils.ts` around lines 77 - 83, The isBefore method in
WrapAroundUnsignedInt incorrectly uses this.maxSize as the modulus and
threshold; change the logic to compute modulus = this.maxSize + 1 (the full
range), compute diff = (b - a) >>> 0 mod modulus, and then return diff !== 0 &&
diff <= modulus / 2 (i.e. less than or equal to the half-range) so ordering
respects wraparound; update the isBefore implementation (referencing function
WrapAroundUnsignedInt.isBefore and variables a, b, diff, this.maxSize)
accordingly.
| /** Returns true if {@link this} is before the passed other {@link DataTrackTimestamp}. */ | ||
| isBefore(other: DataTrackTimestamp<RateInHz>) { | ||
| return this.timestamp.isBefore(other.timestamp); | ||
| } |
There was a problem hiding this comment.
Keep DataTrackTimestamp.isBefore as a simple numeric compare.
This should not be wraparound-aware in this file; delegating to WrapAroundUnsignedInt.isBefore changes the intended semantics.
🛠️ Proposed fix
isBefore(other: DataTrackTimestamp<RateInHz>) {
- return this.timestamp.isBefore(other.timestamp);
+ return this.timestamp.value < other.timestamp.value;
}Based on learnings: In src/room/data-track/utils.ts, DataTrackTimestamp.isBefore() is intentionally not wraparound-resilient and should use simple numeric comparison.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Returns true if {@link this} is before the passed other {@link DataTrackTimestamp}. */ | |
| isBefore(other: DataTrackTimestamp<RateInHz>) { | |
| return this.timestamp.isBefore(other.timestamp); | |
| } | |
| /** Returns true if {`@link` this} is before the passed other {`@link` DataTrackTimestamp}. */ | |
| isBefore(other: DataTrackTimestamp<RateInHz>) { | |
| return this.timestamp.value < other.timestamp.value; | |
| } |
🤖 Prompt for AI Agents
In `@src/room/data-track/utils.ts` around lines 118 - 121,
DataTrackTimestamp.isBefore currently delegates to
WrapAroundUnsignedInt.isBefore which makes it wraparound-aware; change it to a
plain numeric comparison to preserve intended semantics: inside
DataTrackTimestamp.isBefore compare the underlying numeric timestamp values on
this.timestamp and other.timestamp (referencing DataTrackTimestamp, isBefore and
timestamp) and return the boolean result instead of calling
WrapAroundUnsignedInt.isBefore.
There was a problem hiding this comment.
No this is correct, shijing's comment resulted in the behavior of this changing, it is wraparound aware now.
There was a problem hiding this comment.
@1egoman, thank you for the clarification! You're absolutely right—I was working from outdated information. The current implementation correctly delegates to WrapAroundUnsignedInt.isBefore() to make it wraparound-aware, which aligns with the commit message "feat: make isBefore handle wraparound." I've updated my knowledge base to reflect this change.
✏️ Learnings added
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:118-121
Timestamp: 2026-02-03T17:59:38.117Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is now wraparound-aware and correctly delegates to `WrapAroundUnsignedInt.isBefore()` to handle sequence number wraparound in comparisons. This behavior was updated following shijing's comment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
🧠 Learnings used
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/utils.ts:110-112
Timestamp: 2026-01-28T02:37:34.704Z
Learning: In the client-sdk-js codebase at `src/room/data-track/utils.ts`, the `isBefore()` method in `DataTrackTimestamp` is intentionally NOT resilient to wraparounds and uses simple numeric comparison. This matches the Rust implementation being adapted.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packet/extensions.ts:216-242
Timestamp: 2026-01-28T02:22:50.735Z
Learning: In the client-sdk-js codebase at `src/room/data-track/packet/extensions.ts`, RTP length fields represent the actual length in bytes minus 1, following the RTP extension encoding convention.
Learnt from: 1egoman
Repo: livekit/client-sdk-js PR: 1798
File: src/room/data-track/packetizer.ts:85-126
Timestamp: 2026-01-28T02:36:18.432Z
Learning: In the client-sdk-js codebase's data track packetizer (`src/room/data-track/packetizer.ts`), zero-length payloads should produce no packets rather than one packet with an empty payload, matching the behavior of the Rust implementation.
Adds the Packetizer and Depacketizer for Data Tracks.
The packetizer takes a
DataTrackFrame(new concept, basically right now a pair of aUint8Arraypayload and any packet extensions) and converts it into a series ofDataTrackPackets.The depacketizer does the inverse - it takes a series of packets and aggregates them to recreate the original
DataTrackFrame.Summary by CodeRabbit