Skip to content

Commit f14e680

Browse files
1egomanclaude
andauthored
Ensure roomOptions in Room.connect(...) are using the locally defined variable, NOT implicitly this.roomOptions (#1169)
While working on #1166, I discovered this bug incidentally, so here is an isolated fix. `connect`'s first line is `var roomOptions = this.roomOptions;`. However, a parameter of the same name (`roomOptions`) was in scope. After some unexpected behavior, I realized that dart permits a local to shadow a parameter (silently, with no warning and the local winning) so every `RoomOptions` a caller passed to `connect` was discarded and the Room's own options were used instead. Nothing surfaced the mismatch. The `roomOptions` parameter on `connect` seems to be deprecated in favor of the parameter on the `Room` constructor, but deprecated is not the same as inert: while it is still accepted it has to take effect. It now does, falling back to the Room's options when absent. `Engine.connect` already adopted whatever it was handed, so the value propagates from there without further changes. The local is renamed to `effectiveRoomOptions`, since restoring the parameter's visibility is the whole point and leaving two things called `roomOptions` in one scope is what caused this. I also addeds a regression test, and threaded `connectOptions`/`roomOptions` through the E2E container so it can be exercised. I verified the test failed against the old shadowing behavior. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 7485922 commit f14e680

4 files changed

Lines changed: 94 additions & 15 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
patch type="fixed" "Room.connect no longer ignores the roomOptions argument passed to it"

lib/src/core/room.dart

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -274,21 +274,26 @@ class Room extends DisposableChangeNotifier with EventsEmittable<RoomEvent> {
274274
@Deprecated('deprecated, please use roomOptions in Room constructor') RoomOptions? roomOptions,
275275
FastConnectOptions? fastConnectOptions,
276276
}) async {
277-
var roomOptions = this.roomOptions;
278-
if (lkPlatformIs(PlatformType.web) && (roomOptions.networkOptions.certificatePinning?.isEnabled ?? false)) {
277+
var effectiveRoomOptions = roomOptions ?? this.roomOptions;
278+
if (lkPlatformIs(PlatformType.web) &&
279+
(effectiveRoomOptions.networkOptions.certificatePinning?.isEnabled ?? false)) {
279280
throw UnsupportedError('Certificate pinning is not supported on Flutter web, '
280281
'remove certificatePinning from NetworkOptions when targeting web');
281282
}
282283
connectOptions ??= ConnectOptions();
283284
_pendingTrackQueue.updateTtl(connectOptions.timeouts.subscribe);
284285
// ignore: deprecated_member_use_from_same_package
285-
if ((roomOptions.encryption != null || roomOptions.e2eeOptions != null) && engine.e2eeManager == null) {
286+
if ((effectiveRoomOptions.encryption != null || effectiveRoomOptions.e2eeOptions != null) &&
287+
engine.e2eeManager == null) {
286288
if (!lkPlatformSupportsE2EE()) {
287289
throw LiveKitE2EEException('E2EE is not supported on this platform');
288290
}
289291
// ignore: deprecated_member_use_from_same_package
290-
final e2eeOptions = roomOptions.encryption ?? roomOptions.e2eeOptions;
291-
_e2eeManager = E2EEManager(e2eeOptions!.keyProvider, dcEncryptionEnabled: roomOptions.encryption != null);
292+
final e2eeOptions = effectiveRoomOptions.encryption ?? effectiveRoomOptions.e2eeOptions;
293+
_e2eeManager = E2EEManager(
294+
e2eeOptions!.keyProvider,
295+
dcEncryptionEnabled: effectiveRoomOptions.encryption != null,
296+
);
292297
await _e2eeManager!.setup(this);
293298
engine.setE2eeManager(_e2eeManager);
294299
} else {
@@ -297,8 +302,8 @@ class Room extends DisposableChangeNotifier with EventsEmittable<RoomEvent> {
297302

298303
if (_e2eeManager != null) {
299304
// Disable backup codec when e2ee is enabled
300-
roomOptions = roomOptions.copyWith(
301-
defaultVideoPublishOptions: roomOptions.defaultVideoPublishOptions.copyWith(
305+
effectiveRoomOptions = effectiveRoomOptions.copyWith(
306+
defaultVideoPublishOptions: effectiveRoomOptions.defaultVideoPublishOptions.copyWith(
302307
backupVideoCodec: const BackupVideoCodec(enabled: false),
303308
),
304309
);
@@ -310,7 +315,11 @@ class Room extends DisposableChangeNotifier with EventsEmittable<RoomEvent> {
310315
}
311316
if (isCloudUrl(Uri.parse(url))) {
312317
if (_regionUrlProvider == null) {
313-
_regionUrlProvider = RegionUrlProvider(url: url, token: token, networkOptions: roomOptions.networkOptions);
318+
_regionUrlProvider = RegionUrlProvider(
319+
url: url,
320+
token: token,
321+
networkOptions: effectiveRoomOptions.networkOptions,
322+
);
314323
} else {
315324
_regionUrlProvider?.updateToken(token);
316325
}
@@ -328,7 +337,7 @@ class Room extends DisposableChangeNotifier with EventsEmittable<RoomEvent> {
328337
// AudioManager once, on the first connect. Skipping it on a later manual
329338
// connect of the same Room keeps a runtime speaker change from being
330339
// reverted. New code should call setSpeakerOutputPreferred directly.
331-
final legacySpeakerOn = roomOptions.defaultAudioOutputOptions.speakerOn;
340+
final legacySpeakerOn = effectiveRoomOptions.defaultAudioOutputOptions.speakerOn;
332341
if (legacySpeakerOn != null && !_legacySpeakerBridged && lkPlatformIsMobile()) {
333342
_legacySpeakerBridged = true;
334343
await AudioManager.instance.setSpeakerOutputPreferred(legacySpeakerOn);
@@ -343,7 +352,7 @@ class Room extends DisposableChangeNotifier with EventsEmittable<RoomEvent> {
343352
_regionUrl ?? url,
344353
token,
345354
connectOptions: connectOptions,
346-
roomOptions: roomOptions,
355+
roomOptions: effectiveRoomOptions,
347356
fastConnectOptions: fastConnectOptions,
348357
regionUrlProvider: _regionUrlProvider,
349358
);
@@ -366,7 +375,7 @@ class Room extends DisposableChangeNotifier with EventsEmittable<RoomEvent> {
366375
nextUrl,
367376
token,
368377
connectOptions: connectOptions,
369-
roomOptions: roomOptions,
378+
roomOptions: effectiveRoomOptions,
370379
fastConnectOptions: fastConnectOptions,
371380
regionUrlProvider: _regionUrlProvider,
372381
);
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// Copyright 2026 LiveKit, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
@Timeout(Duration(seconds: 10))
16+
library;
17+
18+
import 'package:flutter_test/flutter_test.dart';
19+
20+
import 'package:livekit_client/livekit_client.dart';
21+
import '../mock/e2e_container.dart';
22+
import '../mock/peerconnection_mock.dart';
23+
24+
void main() {
25+
setUp(resetMockDataChannels);
26+
27+
group('Room.connect options', () {
28+
// Regression: the deprecated `roomOptions` parameter was shadowed by a local of the same name
29+
// in the first line of `connect`, which Dart permits silently. Everything passed here was
30+
// discarded, so callers saw the Room's own options with no indication anything was wrong.
31+
test('honors the roomOptions passed to connect', () async {
32+
final container = E2EContainer(
33+
roomOptions: const RoomOptions(dynacast: false, adaptiveStream: false),
34+
);
35+
addTearDown(container.dispose);
36+
37+
await container.connectRoom(
38+
// ignore: deprecated_member_use_from_same_package
39+
roomOptions: const RoomOptions(dynacast: true, adaptiveStream: true),
40+
);
41+
42+
expect(container.room.roomOptions.dynacast, isTrue);
43+
expect(container.room.roomOptions.adaptiveStream, isTrue);
44+
});
45+
46+
test('falls back to the Room\'s options when connect is given none', () async {
47+
final container = E2EContainer(
48+
roomOptions: const RoomOptions(dynacast: true, adaptiveStream: true),
49+
);
50+
addTearDown(container.dispose);
51+
52+
await container.connectRoom();
53+
54+
expect(container.room.roomOptions.dynacast, isTrue);
55+
expect(container.room.roomOptions.adaptiveStream, isTrue);
56+
});
57+
});
58+
}

test/mock/e2e_container.dart

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,12 @@ class E2EContainer {
3737
/// since [connectRoom] returned. Populated only when [captureOutbound] is true.
3838
final List<lk_models.DataPacket> capturedDataPackets = [];
3939

40-
E2EContainer() {
40+
E2EContainer({RoomOptions roomOptions = const RoomOptions()}) {
4141
wsConnector = MockWebSocketConnector();
4242
client = SignalClient(wsConnector.connect);
4343
engine = Engine(
4444
connectOptions: const ConnectOptions(),
45-
roomOptions: const RoomOptions(),
45+
roomOptions: roomOptions,
4646
signalClient: client,
4747
peerConnectionCreate: MockPeerConnection.create,
4848
);
@@ -58,8 +58,19 @@ class E2EContainer {
5858
/// that value (used to exercise v1 vs v2 caller paths in self-loop tests).
5959
/// When [captureOutbound] is true, all DataPackets sent over the reliable
6060
/// data channel are recorded in [capturedDataPackets].
61-
Future<void> connectRoom({int? localClientProtocol, bool captureOutbound = false}) async {
62-
final connectFuture = room.connect(exampleUri, token);
61+
Future<void> connectRoom({
62+
int? localClientProtocol,
63+
bool captureOutbound = false,
64+
ConnectOptions? connectOptions,
65+
@Deprecated('mirrors the deprecated Room.connect parameter') RoomOptions? roomOptions,
66+
}) async {
67+
final connectFuture = room.connect(
68+
exampleUri,
69+
token,
70+
connectOptions: connectOptions,
71+
// ignore: deprecated_member_use_from_same_package
72+
roomOptions: roomOptions,
73+
);
6374
Future.delayed(const Duration(milliseconds: 1), () {
6475
final resp = _buildJoinResponse(localClientProtocol);
6576
wsConnector.onData(resp.writeToBuffer());

0 commit comments

Comments
 (0)