chore: extract data channel handling into manager class by lukasIO · Pull Request #2014 · livekit/client-sdk-js

lukasIO · 2026-07-16T12:48:22Z

No description provided.

…congestion control

changeset-bot · 2026-07-16T12:48:26Z

🦋 Changeset detected

Latest commit: 2c273ce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
livekit-client	Patch

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

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Resolve conflicts in favour of the refactored data-channel module: the base branch's Option 2 replay fix, the throws-check handling, and the byterate/stat changes are already ported onto the module structure (FlowControlledDataChannel / ReliableDataChannel / LossyDataChannel). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Resolve the RTCEngine conflicts in favour of the refactored data-channel module (the base branch's changes were inline renames of code this branch moved into FlowControlledDataChannel), then apply the equivalent naming to the module so the intent actually lands here: - waitForHeadroom / waitForHeadroomLocked -> waitForHeadroomWithLock / waitForHeadroomWithoutLock - the per-kind AbortController "epoch" -> waiterAbortController (epochSignal -> abortSignal, onEpochAbort -> onAbort) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…dling

main squash-merged the data-channel watermark work (#2013), so its inline RTCEngine changes conflicted with this branch's refactored data-channel module. Resolved RTCEngine.ts / RTCEngine.test.ts in favour of the module version (which already carries all of that work), and took main's other additions cleanly — notably the data-track SID reassignment fix (#2000) this branch didn't yet have. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

github-actions · 2026-07-17T15:16:14Z

size-limit report 📦

Path	Size
dist/livekit-client.esm.mjs	102.33 KB (0%)
dist/livekit-client.umd.js	111.24 KB (0%)

1egoman · 2026-07-20T13:49:34Z

+
+  readonly lowWaterMark: number;
+
+  readonly highWaterMark: number;


thought: I don't see any interface overlap between this class and LossyDataChannel which is a bit surprising - I would expect this to at least have a send method of some sort matching the lossy one? I bring it up because it might be nice to have an abstract class / interface all three are children of to help ensure future api homogeneity.

it's a fair consideration. both lossy and reliable extend this one, so things like refreshBufferStatus/attach/detach are shared. send isn't because the signature for the reliable one is slightly different (we also pass a sequence number on the reliable channel) and it would arguably become a bit awkward if we had a shared base signature that looks like send(bytes, sequence?). There's also no call site that acts on FlowControlledDataChannel as a base, so we always know in code which kind we're dealing with.

1egoman · 2026-07-20T13:51:07Z

+    if (!dc) {
+      return;
+    }
+    this.messageBuffer.popToSequence(lastMessageSeq);


nitpick: resurfacing this comment: #2013 (comment)

1egoman · 2026-07-20T13:52:45Z

+ * {@link replay} delivers them (plus any unacked packets) after a resume. Only an engine close
+ * rejects, because no replay is coming after that.
+ */
+export class ReliableDataChannel extends FlowControlledDataChannel {


Very nice that you were able to make this superset of FlowControlledDataChannel!

1egoman · 2026-07-20T20:50:25Z

+
+    manager.createPublisherChannels(pcManager);
+
+    expect(created.map((dc) => dc.label)).toEqual(['_lossy', '_reliable', '_data_track']);


nitpick: should this .sort() before asserting? Should the test be asserting the order or just that all three were made?

converted it into a record which should also address this

1egoman · 2026-07-20T21:06:00Z

+    ...overrides,
+  };
+  const manager = new DataChannelManager(opts);
+  const created: FakeDataChannel[] = [];


suggestion: You might consider making created some sort of map / object with dc.label as the key. Then code like created.find((dc) => dc.label === '_reliable')!.bufferedAmount = ... could become created['_reliable'].bufferedAmount = ... or also the earlier nitpick I made regarding .sort() could maybe be done more elegantly by checking Object.keys(...).

1egoman · 2026-07-20T21:07:45Z

+const tick = () => new Promise<void>((resolve) => setTimeout(resolve, 0));
+
+function makeChannel(opts?: { dc?: FakeDataChannel | undefined; engineClosed?: boolean }) {
+  const dc = 'dc' in (opts ?? {}) ? opts!.dc : new FakeDataChannel();


nitpick: This is a bit of a weird way do do this, couldn't this just be:

Suggested change

const dc = 'dc' in (opts ?? {}) ? opts!.dc : new FakeDataChannel();

const dc = opts?.dc ?? new FakeDataChannel();

yeah, that's a lot nicer!

there's a slight difference though, where opts: { dc: undefined } would become identical to no opts being passed. solved it by using null as the explicit pass option.

1egoman · 2026-07-20T21:08:38Z

+  if (dc) {
+    channel.attach(dc as unknown as RTCDataChannel);
+  }
+  return { channel, dc: dc as FakeDataChannel, state, onBufferStatusChanged };


nitpick: Also not exactly sure why you need this cast here since dc is already FakeDataChannel, maybe I'm missing something though:

Suggested change

return { channel, dc: dc as FakeDataChannel, state, onBufferStatusChanged };

return { channel, dc, state, onBufferStatusChanged };

the cast is only for type checking in the tests to work as expected. the dc could be passed as null for checking the no-handle cases. The return is cast to simply not-be-null

1egoman · 2026-07-20T21:15:36Z

+    it('notifies only on a change and in both directions', () => {
+      const { channel, dc, onBufferStatusChanged } = makeChannel();
+
+      // Starts "low" (empty buffer); a refresh that stays low emits nothing.
+      channel.refreshBufferStatus();
+      expect(onBufferStatusChanged).not.toHaveBeenCalled();
+
+      // Cross above the low mark → one not-low notification.
+      dc.bufferedAmount = 512;
+      channel.refreshBufferStatus();
+      channel.refreshBufferStatus(); // debounced: no second fire
+      expect(onBufferStatusChanged.mock.calls).toEqual([[false]]);
+
+      // Drain back below → one low notification.
+      dc.bufferedAmount = 0;
+      channel.refreshBufferStatus();
+      expect(onBufferStatusChanged.mock.calls).toEqual([[false], [true]]);
+    });
+
+    it('is a no-op without a handle (no throw, no notification)', () => {
+      const { channel, onBufferStatusChanged } = makeChannel({ dc: undefined });
+      expect(() => channel.refreshBufferStatus()).not.toThrow();
+      expect(onBufferStatusChanged).not.toHaveBeenCalled();
+    });
+  });
+
+  it('reports watermark status against the given channel', () => {
+    const { channel, dc } = makeChannel();
+    const handle = dc as unknown as RTCDataChannel;
+    dc.bufferedAmount = 0;
+    expect(channel.isBelowHighWaterMark(handle)).toBe(true);
+    expect(channel.isBelowLowWaterMark(handle)).toBe(true);
+
+    dc.bufferedAmount = 512; // between low (64) and high (1024)
+    expect(channel.isBelowHighWaterMark(handle)).toBe(true);
+    expect(channel.isBelowLowWaterMark(handle)).toBe(false);
+
+    dc.bufferedAmount = 2048;
+    expect(channel.isBelowHighWaterMark(handle)).toBe(false);
+  });
+
+  it('waiting for headroom without a handle rejects with a connection error', async () => {


These tests are really easy to understand, amazing job!

1egoman · 2026-07-20T21:18:27Z

+    dc.bufferedAmount = 2048;
+    const resolved = vi.fn();
+    const send = channel.send(new Uint8Array(10)).then(resolved);
+    await vi.advanceTimersByTimeAsync(0);


question: What's the difference between this and the await tick() you were using in other tests in practice (I realize one is mocking timers and one isn't, but does that matter here? I'm assuming vitest is calling these "next tick" deferred handlers on the current tick to implement this?), and does it make sense to standardize on one or the other?

good question, dove a bit deeper into the why:

the main consideration is that advanceTimersByTimeAsync is only used in files where we use fake timers. Here we need those for the dynamic threshold scheduling.

the tick method uses real timers and it would simply hang if we were using it in a file with fake timers.
the other way around, using advanceTimersByTimeAsync in the other files would mean we have to do manual timer book keeping with fake timers.

…t-sdk-js into lukas/refactor-dc-handling

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

1egoman and others added 23 commits July 15, 2026 10:44

feat: add new data channel high / low water mark approach for packet …

3b101cd

…congestion control

fix: address typescript errors

c4aefaf

fix: add missing changeset

35215c7

fix: add waitUntilBelowHighWaterMark mock to tests

2b509cd

tighten lossy control sending, reject on dc close

f7d349d

consistency

2031267

rename

ccc1669

fix dynamic lossy behaviour

cbd6a13

naming

22ffc35

cleanup

756e8e3

comments

23da009

naming

e6fa0d7

guard against silent package loss on resume

95dd9fa

address resume edge cases

0461bc2

catch throwing buffer check

ed2ac47

phase 1

b74ea7c

fix test

808bc5a

phase 2

a78fd36

phase 4

3ec286a

phase 4

43f539b

data decode helper

8a8bd82

rename

fbb5a63

extract buffer status handling

f85ed94

lukasIO requested review from 1egoman and xianshijing-lk as code owners July 16, 2026 12:48

devin-ai-integration Bot reviewed Jul 16, 2026

View reviewed changes

lukasIO added 3 commits July 16, 2026 16:14

fix throws-check

d6faabc

address devin comment

cc79fd7

sync with data-channel-buffer-range

09969c3

This comment was marked as resolved.

Sign in to view

lukasIO and others added 3 commits July 17, 2026 16:35

make naming clearer

3f63fbb

clarify epoch/abort naming

facfe90

Base automatically changed from data-channel-buffer-range to main July 17, 2026 15:08

lukasIO and others added 2 commits July 17, 2026 17:13

Merge remote-tracking branch 'origin/main' into lukas/refactor-dc-han…

9d3853e

…dling

1egoman approved these changes Jul 20, 2026

View reviewed changes

lukasIO added 4 commits July 21, 2026 09:50

address comments

b98e0f7

fix tests

e7a17b7

Merge branch 'lukas/refactor-dc-handling' of github.com:livekit/clien…

cfb1898

…t-sdk-js into lukas/refactor-dc-handling

address devin comment

c0d2724

This comment was marked as resolved.

Sign in to view

lukasIO and others added 2 commits July 21, 2026 10:35

Update src/room/data-channel/LossyDataChannel.ts

2643dea

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Create brave-crews-flash.md

2c273ce

lukasIO merged commit b365435 into main Jul 21, 2026
4 of 5 checks passed

lukasIO deleted the lukas/refactor-dc-handling branch July 21, 2026 09:31

This was referenced Jul 21, 2026

Version Packages #2016

Merged

Version Packages apnerve/client-sdk-js#123

Open


		readonly lowWaterMark: number;

		readonly highWaterMark: number;


		manager.createPublisherChannels(pcManager);

		expect(created.map((dc) => dc.label)).toEqual(['_lossy', '_reliable', '_data_track']);

	const dc = 'dc' in (opts ?? {}) ? opts!.dc : new FakeDataChannel();
	const dc = opts?.dc ?? new FakeDataChannel();

	return { channel, dc: dc as FakeDataChannel, state, onBufferStatusChanged };
	return { channel, dc, state, onBufferStatusChanged };

Uh oh!

Conversation

lukasIO commented Jul 16, 2026

Uh oh!

changeset-bot Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

This comment was marked as resolved.

Uh oh!

github-actions Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasIO Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1egoman Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Jul 16, 2026 •

edited

Loading

github-actions Bot commented Jul 17, 2026 •

edited

Loading

lukasIO Jul 21, 2026 •

edited

Loading

1egoman Jul 20, 2026 •

edited

Loading