Add LivestreamChannelController
#3750
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DemoLivestreamChatChannelVC
participant Controller as LivestreamChannelController
participant API as APIClient
participant Event as EventsController
UI->>Controller: synchronize()
Controller->>API: fetch channel/messages
API-->>Controller: ChannelPayload/MessagePayload
Controller->>Controller: Convert payloads to models
Controller->>UI: delegate.didUpdateChannel / didUpdateMessages
Event-->>Controller: Event (message/new, message/updated, etc.)
Controller->>Controller: Update in-memory state
Controller->>UI: delegate.didUpdateMessages
UI->>Controller: createNewMessage(...)
Controller->>API: send message
API-->>Controller: MessagePayload
Controller->>Controller: Update messages, notify delegate
UI->>Controller: loadPreviousMessages/loadNextMessages
Controller->>API: fetch messages
API-->>Controller: MessagePayload[]
Controller->>Controller: Merge into messages, notify delegate
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nuno-vieira
changed the title
LivestreamChannelController
SDK Size
|
There was a problem hiding this comment.
Looks good so far! From the testing, I think we should be more aggressive with fetching the next page (do it sooner), there's a bit of delay while loading more messages. Also, we should think about the initial blank state when opening the chat - since there's no local data available now - do we expose some loading state?
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
Outdated
Show resolved
Hide resolved
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
Outdated
Show resolved
Hide resolved
nuno-vieira
added
🌐 SDK: StreamChat (LLC)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift (3)
118-121: Useself.controllerfor consistencyFor consistency with the rest of the test file, consider using
self.controllerinstead of justcontroller.-controller = LivestreamChannelController( +self.controller = LivestreamChannelController( channelQuery: channelQuery, client: client )
807-807: Consider consistent cleanup pattern for mock objectsThe
mockUpdater.cleanUp()is called in these tests but not in other tests that use mocks. Consider either:
- Adding cleanup for all mock objects consistently across all tests
- Relying on the tearDown method for cleanup
- Using
defer { mockUpdater.cleanUp() }right after creating the mockAlso applies to: 859-859
1765-1769: Potential test flakiness due to time-based assertionsThe hardcoded time range assertions (18-22 seconds) could cause flaky tests in CI environments where timing might vary. Consider using a more flexible tolerance or mocking the current date.
// Then // Should be approximately 20 seconds (30 - 10) -XCTAssertGreaterThan(cooldownTime, 18) -XCTAssertLessThan(cooldownTime, 22) +let expectedCooldown = 20 +let tolerance = 5 // Allow 5 seconds tolerance +XCTAssertEqual(cooldownTime, expectedCooldown, accuracy: tolerance)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift(1 hunks)StreamChat.xcodeproj/project.pbxproj(25 hunks)TestTools/StreamChatTestTools/Mocks/StreamChat/Workers/EventNotificationCenter_Mock.swift(1 hunks)Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift(1 hunks)Tests/StreamChatTests/Workers/ChannelUpdater_Tests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- StreamChat.xcodeproj/project.pbxproj
- Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
TestTools/StreamChatTestTools/Mocks/StreamChat/Workers/EventNotificationCenter_Mock.swift (1)
Sources/StreamChat/Workers/EventNotificationCenter.swift (2)
registerManualEventHandling(35-37)unregisterManualEventHandling(40-42)
⏰ 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). (3)
- GitHub Check: Build Test App and Frameworks
- GitHub Check: Build LLC + UI (Xcode 15)
- GitHub Check: Metrics
🔇 Additional comments (4)
Tests/StreamChatTests/Workers/ChannelUpdater_Tests.swift (1)
1773-1807: LGTM! Well-structured test suite for disableSlowMode functionality.The new test suite properly covers all aspects of the
disableSlowModemethod:
- API Call Verification: Correctly verifies that disabling slow mode calls the enable slow mode endpoint with
cooldownDuration: 0- Success Response Handling: Properly tests that successful API responses are propagated to the completion handler
- Error Response Handling: Appropriately tests that API errors are propagated to the completion handler
The implementation follows the established testing patterns in the file and provides comprehensive coverage for the new functionality.
TestTools/StreamChatTestTools/Mocks/StreamChat/Workers/EventNotificationCenter_Mock.swift (2)
19-24: LGTM! Consistent with existing mock patterns.The tracking variables follow the established naming conventions and patterns used throughout this mock class. Using optional
ChannelId?is appropriate for tracking the last called parameter.
25-33: LGTM! Proper mock implementation.The override methods correctly implement mock behavior by tracking call counts and parameters without invoking the parent implementation. This follows the established pattern for other mock methods in this class and aligns with the actual methods being mocked as shown in the relevant code snippets.
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift (1)
29-43: Memory leak prevention looks goodThe tearDown method properly cleans up resources and uses
AssertAsyncto verify memory deallocation, which is a best practice for preventing retain cycles in test suites.
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController+Combine_Tests.swift (2)
64-78: Ensure weak reference is valid before use.The test sets
livestreamChannelController = niland then immediately uses the weak referencecontroller. Consider adding a nil check to make the test more robust.// Keep only the weak reference to the controller. The existing publisher should keep it alive. weak var controller: LivestreamChannelController? = livestreamChannelController livestreamChannelController = nil // Simulate channel update event -controller?.eventsController( - EventsController(notificationCenter: client.eventNotificationCenter), - didReceiveEvent: event -) +guard let controller = controller else { + XCTFail("Controller should be kept alive by publisher") + return +} +controller.eventsController( + EventsController(notificationCenter: client.eventNotificationCenter), + didReceiveEvent: event +)
168-193: Consider testing resume functionality as well.The isPaused publisher test covers the pause scenario well. Consider adding a test for the resume functionality to ensure complete coverage of the pause/resume cycle.
Add this test after the pause verification:
// Test pausing controller?.pause() // Use AssertAsync to wait for the async update AssertAsync { Assert.willBeEqual(recording.output, [false, true]) Assert.willBeEqual(controller?.isPaused, true) } +// Test resuming +controller?.resume() + +AssertAsync { + Assert.willBeEqual(recording.output, [false, true, false]) + Assert.willBeEqual(controller?.isPaused, false) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
StreamChat.xcodeproj/project.pbxproj(26 hunks)Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController+Combine_Tests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- StreamChat.xcodeproj/project.pbxproj
⏰ 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). (4)
- GitHub Check: Test LLC (Debug)
- GitHub Check: Build Test App and Frameworks
- GitHub Check: Build LLC + UI (Xcode 15)
- GitHub Check: Metrics
🔇 Additional comments (3)
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController+Combine_Tests.swift (3)
11-38: Well-structured test setup with proper memory management.The test class setup follows excellent testing practices with comprehensive resource cleanup and memory leak detection using
AssertAsync.canBeReleased.
98-164: Comprehensive message publisher testing.The message changes publisher tests properly verify both functionality and memory management. The test correctly simulates multiple message events and validates the expected output count.
212-257: Excellent test of skipped message business logic.The test properly validates the key business rule that skipped messages are only counted when the controller is paused and the messages are from other users. The feature flag configuration and event simulation are correctly implemented.
nuno-vieira
force-pushed
the
add/livestream-channel-controller
branch
from
52efeee to
5153c46
Compare
|
testableapple
added
🟢 QAed
🔗 Issue Links
https://linear.app/stream/issue/IOS-1025/create-a-livestream-controller
🎯 Goal
Create a new
LivestreamChannelControllerwhich is a controller suitable for Livestreams due to better performance. It does not rely on DB persistence. But has fewer features than a regular channel controller.📝 Summary
LivestreamChannelControllerhandles events manually instead of relying on Event Middlewares and DB updatesDemoLivestreamChatChannelVCin the Demo App to test drive the controllerLivestreamChannelController.maxMessageLimitOptionspause()that will stop collecting messages in memory onmessage.neweventresume()that will load the first page and start collecting messages again onmessage.newcreateNewMessage()is called, it resumes automatically if it was paused.Performance Benefits
The
LivestreamChannelControlleris optimised for livestream scenarios:Comparison with Regular Chat
Current Issues/Limitations
ownCapabilities.🧪 Manual Testing Notes
How to test:
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores