Fix marking channel read when scrolling to bottom without unread count #3881
Conversation
WalkthroughThis pull request fixes a bug where channels were being marked as read on every scroll to the bottom, regardless of unread message count. The solution adds a guard condition requiring at least one unread message before allowing channels to be marked as read, along with comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant VC as ChatChannelVC
participant Channel as Channel Model
User->>VC: Scrolls to bottom
VC->>VC: shouldMarkChannelRead() called
rect rgb(200, 220, 240)
Note over VC: Check existing conditions<br/>(isLastMessageVisible,<br/>hasSeenFirstUnreadMessage, etc.)
end
rect rgb(240, 200, 200)
Note over VC: NEW: Verify unreadCount > 0
VC->>Channel: Get unreadCount.messages
Channel-->>VC: unreadCount
alt Has unread messages
rect rgb(200, 240, 200)
VC->>VC: Mark channel as read ✓
end
else No unread messages
rect rgb(240, 200, 200)
VC->>VC: Skip marking as read ✗
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/StreamChatUITests/SnapshotTests/ChatChannel/ChatChannelVC_Tests.swift (1)
907-939: NewshouldMarkChannelReadtests cover the unread-count guard but could be made more explicitThe two new tests correctly validate that:
- With jump-to-unread enabled and
unreadCount.messages == 1,shouldMarkChannelReadreturnstrueunder the expected preconditions.- With the same preconditions but
unreadCount.messages == 0,shouldMarkChannelReadreturnsfalse.This gives good coverage of the new guard. However, the tests currently depend on subtle internal behavior (
hasSeenFirstUnreadMessagebeing flipped viawillDisplayMessageAtwhen bothmessage?.idandfirstUnreadMessageIdarenil, withvc.messagesstill empty).To make these tests more robust against future refactors, consider:
- Initializing
vc.messageswith at least one concreteChatMessage.- Setting
channelControllerMock.mockFirstUnreadMessageIdto that message’s id and invokingvc.channelController(channelControllerMock, didUpdateMessages: [])sofirstUnreadMessageIdis populated through the normal code path.- Then calling
willDisplayMessageAt/scrollViewDidScrollsohasSeenFirstUnreadMessageandhasSeenLastMessageare set in a way that mirrors production behavior.Functionally the tests are correct as-is; this would just reduce their reliance on optional-equality quirks and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)Sources/StreamChatUI/ChatChannel/ChatChannelVC.swift(1 hunks)Tests/StreamChatUITests/SnapshotTests/ChatChannel/ChatChannelVC_Tests.swift(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/StreamChatUITests/SnapshotTests/ChatChannel/ChatChannelVC_Tests.swift (3)
Tests/StreamChatUITests/Helpers/UIView+Helpers.swift (1)
mockIsViewVisible(23-27)TestTools/StreamChatTestTools/Mocks/Models + Extensions/ChatChannel_Mock.swift (3)
mock(10-52)mock(57-73)mock(78-151)Sources/StreamChatUI/ChatChannel/ChatChannelVC.swift (9)
chatMessageListVC(362-364)chatMessageListVC(366-378)chatMessageListVC(380-396)chatMessageListVC(406-416)chatMessageListVC(418-448)chatMessageListVC(458-470)chatMessageListVC(472-478)chatMessageListVC(480-501)chatMessageListVC(503-509)
🔇 Additional comments (2)
CHANGELOG.md (1)
6-8: Changelog entry accurately documents the bug fixThe new StreamChatUI “Fixed” bullet is consistent with existing style and clearly describes the behavioral change tied to this PR.
Sources/StreamChatUI/ChatChannel/ChatChannelVC.swift (1)
92-97: Unread-count guard inshouldMarkChannelReadlooks correctAdding
unreadMessageCount > 0to the jump-to-unread path is a minimal, targeted way to avoid issuing read events when the channel has no unread messages, while preserving existing behavior and throttling in other cases.One thing to double-check: for
isJumpToUnreadEnabled == falsewe still mark the channel read whenever the last message is fully visible and the first page is loaded, even if the unread count is zero. If the intent is to suppress no-op read events across all configurations (not just when jump-to-unread is enabled), you might want to reuse the same unread-count guard in the non-jump-to-unread branch as well; otherwise, this looks like an intentional scope limitation of the fix.
SDK Size
|
SDK Performance
|
StreamChatUI XCSize
|
Public Interface🚀 No changes affecting the public interface. |
|
nuno-vieira
deleted the
fix/marking-channel-read-when-no-unread-count
branch
🔗 Issue Links
https://linear.app/stream/issue/IOS-1217
🎯 Goal
Fix marking channel read when scrolling to bottom without unread count.
We were not checking whether there were unreads or not before marking the channel read. The SwiftUI SDK is already doing this, so there is no issue there.
🧪 Manual Testing Notes
(Requires using Proxyman to verify the requests)
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit