[Enhancement]Simplify Task.timeout and Publisher.nextValue
#914
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies and improves the Publisher.nextValue and Task.timeout APIs by updating parameter names for better clarity, removing complex cancellation handling, and consolidating functionality into the AsyncStream extension.
- Renamed
Task(timeout:)toTask(timeoutInSeconds:)for clearer parameter naming - Removed the
Publisher+NextValue.swiftfile and moved functionality toPublisher+AsyncStream.swift - Simplified timer subscription code by removing manual cancellable management
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| StreamVideoTests/WebRTC/v2/WebRTCStateAdapter_Tests.swift | Updated Task timeout calls to use new timeoutInSeconds parameter |
| StreamVideoTests/WebRTC/Extensions/Foundation/Task_TimeoutTests.swift | Comprehensive test suite expansion and parameter name updates |
| StreamVideoTests/Utils/Extensions/Combine/PublisherAsyncStreamTests.swift | Added extensive test coverage for Publisher AsyncStream functionality |
| StreamVideoTests/Mock/MockRTCAudioStore.swift | Added proper cleanup in deinit |
| StreamVideo.xcodeproj/project.pbxproj | Removed Publisher+NextValue.swift file reference |
| Sources/StreamVideo/WebRTC/v2/StateMachine/Stages/WebRTCCoordinator+Disconnected.swift | Simplified timer subscription by removing manual cancellable handling |
| Sources/StreamVideo/WebRTC/v2/Extensions/Foundation/Task+Timeout.swift | Refactored timeout implementation with better error handling and parameter naming |
| Sources/StreamVideo/Utils/StateMachine/Publisher+NextValue.swift | File removed - functionality moved |
| Sources/StreamVideo/Utils/Extensions/Combine/Publisher+AsyncStream.swift | Added firstValue and nextValue methods with timeout support |
| Sources/StreamVideo/StreamVideo.swift | Simplified timer subscriptions by removing manual cancellable management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// | ||
| /// - Note: This implementation uses a task group to manage concurrent execution | ||
| /// of the main operation and the timeout timer. | ||
| extension Task where Failure == any Error { |
There was a problem hiding this comment.
The constraint where Failure == any Error is unnecessarily restrictive. The original constraint where Failure == Error was more appropriate as it allows the extension to work with any concrete Error type, not just existential types.
| extension Task where Failure == any Error { | |
| extension Task where Failure == Error { |
Copilot uses AI. Check for mistakes.
| file: StaticString = #fileID, | ||
| function: StaticString = #function, | ||
| line: UInt = #line, | ||
| operation: @Sendable @escaping @isolated(any) () async throws -> Success |
There was a problem hiding this comment.
The @isolated(any) annotation is unnecessary here and may cause compilation issues. The @Sendable @escaping annotations are sufficient for this async operation parameter.
| operation: @Sendable @escaping @isolated(any) () async throws -> Success | |
| operation: @Sendable @escaping () async throws -> Success |
Copilot uses AI. Check for mistakes.
| function: function, | ||
| line: line | ||
| ) { | ||
| try await selfReference.firstValue(file: file, line: line) |
There was a problem hiding this comment.
Using nonisolated(unsafe) is potentially dangerous as it bypasses Swift's concurrency safety checks. Consider using a safer approach like capturing self directly in the Task closure or using proper isolation.
| try await selfReference.firstValue(file: file, line: line) | |
| return try await Task( | |
| timeoutInSeconds: timeoutInSeconds, | |
| file: file, | |
| function: function, | |
| line: line | |
| ) { | |
| try await self.firstValue(file: file, line: line) |
Copilot uses AI. Check for mistakes.
Generated by 🚫 Danger |
Public Interface🚀 No changes affecting the public interface. |
ipavlidakis
changed the title
Task.timeout and Publisher.nextValue
SDK Size
|
|
| @@ -16,6 +16,10 @@ final class MockRTCAudioStore { | |||
| audioStore = RTCAudioStore(session: session) | |||
| } | |||
|
|
|||
| deinit { | |||
| InjectedValues[\.audioStore] = .init() | |||
There was a problem hiding this comment.
Is this a leftover from the other PR?
There was a problem hiding this comment.
This one is actually here to clean the InjectedValues context when the store isn't needed any more. Now that i'm thinking out loud though, it will never be triggered as the reference is still in InjectedValues, so deinit will never be triggered. We probably need an explicit method to uninstall/dismantle.
🎯 Goal
Simplify and improve the
Publisher.nextValueandTask.timeoutAPIs.🛠 Implementation
Reference for
Task.timeoutAPI changes: https://x.com/twannl/status/1950095104254849246🧪 Manual Testing Notes
☑️ Contributor Checklist