feat: restore accepted incoming call from signaling handshake (WT-1167)#1063
feat: restore accepted incoming call from signaling handshake (WT-1167)#1063SERDUN wants to merge 6 commits into
Conversation
7223536 to
2f37cbd
Compare
When Android recreates the Activity (e.g. permission change) during an active call, the new CallBloc starts fresh. The signaling handshake contains the active call in call_logs (IncomingCallEvent + AcceptedEvent) but _handleHandshakeReceived had no path to restore it — the call was silently lost. Changes: - Add CallProcessingStatus.incomingRestoringMedia for the restoration state - Add _RestoreAcceptedIncomingCall internal event carrying the original IncomingCallEvent from the handshake - In _handleHandshakeReceived: detect the pattern (AcceptedEvent latest + IncomingCallEvent earliest, connection == null, call not in state) and dispatch _RestoreAcceptedIncomingCall; hoist connection variable scope - Add _onRestoreAcceptedIncomingCall handler that: 1. Emits ActiveCall with incomingRestoringMedia status 2. Re-registers with Callkeep via reportNewIncomingCall + answerCall 3. Re-negotiates WebRTC using the original offer SDP 4. Sends AcceptRequest to signaling to re-establish media 5. Transitions to connected
- Add `acceptedTime: DateTime` field to `_RestoreAcceptedIncomingCall` event so the original `AcceptedEvent` timestamp is propagated through the flow - Use `event.acceptedTime` instead of `clock.now()` when transitioning to `connected`, so the restored call shows the real accepted time - Fix resource leak in catch block: declare `localStream` and `peerConnection` outside the try, null them out after ownership transfer, and dispose any non-null locals in catch to avoid leaks when an error occurs mid-setup - Add `incomingRestoringMedia` case to l10n extension (was missing, causing a non-exhaustive switch warning) - Tighten `_RestoreAcceptedIncomingCall.line` to non-nullable `int` (null is guarded at the detection site in `_handleHandshakeReceived`)
…ection callLogs is ordered newest-first, so: - firstOrNull = AcceptedEvent (latest) - lastOrNull = IncomingCallEvent (earliest / original offer) The previous code assigned them to variables named `earliest` and `latest` in the wrong order, making the `isRestorationCandidate` type checks always evaluate to false and the restoration path never triggered.
…T-1167) Extract the per-line and local-connection decision logic from CallBloc into a dedicated HandshakeProcessor class that returns a sealed HandshakeAction list. CallBloc._handleHandshakeReceived now only executes those actions (signaling calls, callkeep calls, BLoC event dispatch), keeping all decision logic testable without platform dependencies. New files: - handshake_action.dart — sealed action hierarchy (Hangup, Decline, Restore, HandleIncoming, EndLocalCall) - handshake_processor.dart — stateless processor; only depends on CallkeepConnections (mockable) - test/…/handshake_processor_test.dart — 16 unit tests covering every branch, including the callLogs newest-first ordering that caused the WT-1167 swap-bug
2f37cbd to
f308dd0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final allLines = [...lines, guestLine].whereType<Line>().toList(); | ||
| final localConnections = await callkeepConnections.getConnections(); | ||
|
|
||
| for (final activeLine in allLines) { | ||
| // Get the newest call event from the call logs, if any. | ||
| final callEvent = activeLine.callLogs.whereType<CallEventLog>().map((log) => log.callEvent).firstOrNull; | ||
|
|
||
| CallkeepConnection? connection; | ||
| if (callEvent != null) { | ||
| connection = await callkeepConnections.getConnection(callEvent.callId); | ||
|
|
||
| if (connection?.state == CallkeepConnectionState.stateDisconnected) { | ||
| if (callEvent is AcceptedEvent || callEvent is ProceedingEvent) { | ||
| // Early exit: only this action, consistent with the original `return` in the BLoC. | ||
| return [HangupSignalingAction(line: callEvent.line, callId: callEvent.callId)]; | ||
| } else if (callEvent is IncomingCallEvent) { | ||
| // Early exit: only this action. | ||
| return [DeclineSignalingAction(line: callEvent.line, callId: callEvent.callId)]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // WT-1167 Subtask 2: restore an accepted incoming call after Activity recreation. | ||
| // | ||
| // callLogs is newest-first: | ||
| // firstOrNull → AcceptedEvent (latest) | ||
| // lastOrNull → IncomingCallEvent (earliest / original offer) | ||
| final callEventLogEntries = activeLine.callLogs.whereType<CallEventLog>().toList(); | ||
| final latestCallLog = callEventLogEntries.firstOrNull; | ||
| final earliestCallLog = callEventLogEntries.lastOrNull; |
There was a problem hiding this comment.
HandshakeProcessor uses firstOrNull/lastOrNull, but this file doesn’t import package:collection/collection.dart (or another source for those extensions). As written, this will fail to compile unless an implicit export exists. Add the proper import or avoid those extensions (e.g., by indexing with checks).
| /// 3. Re-negotiate WebRTC using the original offer SDP from [IncomingCallEvent]. | ||
| /// 4. Send an [AcceptRequest] to signaling with the new local answer — the server | ||
| /// re-establishes the media session. | ||
| /// 5. Transition to [CallProcessingStatus.connected]. | ||
| Future<void> _onRestoreAcceptedIncomingCall(_RestoreAcceptedIncomingCall event, Emitter<CallState> emit) async { | ||
| _logger.info('_onRestoreAcceptedIncomingCall: restoring callId=${event.callId}'); | ||
|
|
||
| final incoming = event.incomingCallEvent; | ||
| final jsep = JsepValue.fromOptional(incoming.jsep); |
There was a problem hiding this comment.
The state guard that prevents duplicate restoration (if (state.activeCalls.any(...)) return;) runs after awaiting contactNameResolver.resolveWithNumber(...). If multiple _RestoreAcceptedIncomingCall events are queued concurrently for the same callId, both handlers can pass the guard before either emits, leading to duplicate Callkeep/WebRTC operations. Move the guard to the top (before awaits) and/or mark the call as restoring in state immediately to make the handler idempotent under concurrency.
|
|
||
| final remoteDescription = jsep.toDescription(); | ||
| sdpSanitizer?.apply(remoteDescription); | ||
| await peerConnection.setRemoteDescription(remoteDescription); | ||
|
|
||
| final localDescription = await peerConnection.createAnswer({}); | ||
| sdpMunger?.apply(localDescription); | ||
|
|
||
| await peerConnection.setLocalDescription(localDescription).catchError((e) => throw SDPConfigurationError(e)); | ||
|
|
||
| await _signalingModule.execute( | ||
| AcceptRequest( | ||
| transaction: WebtritSignalingClient.generateTransactionId(), | ||
| line: event.line, | ||
| callId: event.callId, | ||
| jsep: localDescription.toMap(), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
_signalingModule.execute(...) is nullable (returns null when not connected). Here the AcceptRequest send is awaited without checking for null, so if signaling is disconnected the await becomes a no-op and the code will still complete() the peer connection and transition the call to connected, leaving app state inconsistent with server signaling. Treat a null execute as an error (or reconnect and retry) and only transition to connected after confirming the request was actually dispatched / acknowledged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await _signalingModule.execute( | ||
| AcceptRequest( | ||
| transaction: WebtritSignalingClient.generateTransactionId(), | ||
| line: event.line, | ||
| callId: event.callId, | ||
| jsep: localDescription.toMap(), | ||
| ), | ||
| ); | ||
|
|
||
| _peerConnectionManager.complete(event.callId, peerConnection); | ||
| peerConnection = null; // ownership transferred to manager | ||
|
|
||
| emit( | ||
| state.copyWithMappedActiveCall( | ||
| event.callId, | ||
| (c) => c.copyWith(processingStatus: CallProcessingStatus.connected, acceptedTime: event.acceptedTime), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
In _onRestoreAcceptedIncomingCall, _signalingModule.execute(...) returns Future<void>? (null when disconnected). If it returns null here, the code will still proceed to _peerConnectionManager.complete(...) and emit CallProcessingStatus.connected, even though the AcceptRequest was never sent. Consider treating a null return as a hard failure (e.g., throw/abort restoration + reset state) so we don't end up with a locally “connected” call that the server never accepted.
…ll signalingState guard (WT-1167)
Summary
StateHandshake(Android Activity recreation scenario) and re-negotiates WebRTC media from the original offer SDPfirstOrNull/lastOrNullswap in restoration detection —callLogsis newest-first, so the previous check always evaluated tofalseHandshakeProcessorfrom_handleHandshakeReceived: decision logic lives in a separate class, BLoC only executes the returned actionsHandshakeProcessorcovering all branches including the ordering bugTest plan
flutter test test/features/call/bloc/handshake_processor_test.dart— 16/16 pass