Do not reset blocked state when channel specific WS event without blocked state is handled #3794
Conversation
laevandus
added
🐞 Bug
WalkthroughAdds a demo app channel-list filter option for matching blocked/hidden channels, changes ChannelDTO saving to avoid overwriting isBlocked when payload.isBlocked is nil, updates test fixtures to allow optional blocked/hidden, adds a test ensuring blocked channels remain listed when events omit blocked/hidden, and updates the changelog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DemoAppVC as DemoApp VC
participant ChannelListCtrl as ChannelListController
participant Backend
participant DB as Database (ChannelDTO.saveChannel)
User->>DemoAppVC: Select "Blocked/Unblocked with Matching Hidden" filter
DemoAppVC->>ChannelListCtrl: replaceQuery(blocked/hidden composite)
ChannelListCtrl->>Backend: Query channels
Backend-->>ChannelListCtrl: Channel list payloads (with blocked/hidden)
ChannelListCtrl->>DB: saveChannel(payload, via ChannelListQuery)
note over DB: If payload.isBlocked == nil\nDO NOT overwrite existing dto.isBlocked
DB-->>ChannelListCtrl: Persisted channels
User->>DemoAppVC: Open a blocked channel
Backend-->>ChannelListCtrl: Event (e.g., mark read) without blocked/hidden
ChannelListCtrl->>DB: saveChannel(event payload, non-list context)
note over DB: Missing flags leave dto.isBlocked/dto.isHidden unchanged
DB-->>ChannelListCtrl: Channel remains blocked/hidden
ChannelListCtrl-->>User: Channel stays in list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (2)
🔇 Additional comments (2)
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…cked state is handled
laevandus
force-pushed
the
fix/blocked-hidden-channel-list-query
branch
from
b9a417f to
577bdba
Compare
Public Interface🚀 No changes affecting the public interface. |
SDK Size
|
![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 (7)
CHANGELOG.md (1)
9-10: Clarify the fix scope and trigger in the changelog entryCurrent text is a bit vague. Consider making the trigger and impact explicit (WS event omitting
blocked/hiddencausing a channel to disappear from Channel List).Proposed copy tweak:
- - Fix channel getting removed from channel list which includes blocked channels [#3794](https://github.com/GetStream/stream-chat-swift/pull/3794) + - Fix blocked 1:1 channels being removed from Channel List when a WebSocket event (e.g. `notification.mark_read`) omits `blocked`/`hidden` flags and the list includes blocked channels [#3794](https://github.com/GetStream/stream-chat-swift/pull/3794)Sources/StreamChat/Database/DTOs/ChannelDTO.swift (1)
284-291: Add insertion-only default forisBlockedWe confirmed that every “event‐save” path calls
session.saveChannel(payload: …) // uses the overload with query: nil // or session.saveChannel(payload: …, query: nil, cache: nil)so
queryis alwaysnilfor WS events and the new branch will never run there. The suggested patch safely limits defaulting to channel‐list imports only:// Backend only returns a boolean // for blocked 1:1 channels on channel list query if let isBlocked = payload.isBlocked { dto.isBlocked = isBlocked - } else if query != nil { - dto.isBlocked = false + } else if query != nil, dto.isInserted { + // Only default for newly created rows during channel-list imports. + dto.isBlocked = false }No other call sites pass a non-nil
query, so existing channels won’t be accidentally unblocked.TestTools/StreamChatTestTools/TestData/DummyData/ChannelDetailPayload.swift (1)
25-25: OptionalisBlockedparameter: consider defaulting tonilfor clarityMaking
isBlockedoptional is correct to model “field absent.” Defaulting it tofalsemay hide intent at call sites. Defaulting tonilnudges tests to be explicit when the field should be present vs. omitted.- isBlocked: Bool? = false, + isBlocked: Bool? = nil,If changing the default is risky for existing tests, keep it as-is but consider documenting that “nil means omitted by backend/event.”
DemoApp/StreamChat/Components/DemoChatChannelListVC.swift (3)
52-60: Query looks good; minor readability nit: extract the paired conditionThe filter is correct and mirrors the intended “blocked ⇔ hidden” pairing. For readability and reuse in this file, consider extracting the inner paired conditions into a small helper.
Example:
+ private var blockedHiddenParityFilter: Filter<ChannelListFilterScope> { + .or([ + .and([.equal(.blocked, to: false), .equal(.hidden, to: false)]), + .and([.equal(.blocked, to: true), .equal(.hidden, to: true)]) + ]) + } - lazy var blockedUnblockedWithHiddenChannelsQuery: ChannelListQuery = .init( - filter: .and([ - .containMembers(userIds: [currentUserId]), - .or([ - .and([.equal(.blocked, to: false), .equal(.hidden, to: false)]), - .and([.equal(.blocked, to: true), .equal(.hidden, to: true)]) - ]) - ]) - ) + lazy var blockedUnblockedWithHiddenChannelsQuery: ChannelListQuery = .init( + filter: .and([ .containMembers(userIds: [currentUserId]), blockedHiddenParityFilter ]) + )
175-182: Rename action variable; title mentions “excluding deleted” which isn’t reflected in the filterThe variable name suggests “excluding deleted” and “UnBlocked” (mixed casing), but the filter neither references
deletedAtnor adds special handling. The fetch request already excludes deleted channels by default.- let blockedUnBlockedExcludingDeletedChannelsAction = UIAlertAction( - title: "Blocked Unblocked with Matching Hidden Channels", + let blockedUnblockedWithHiddenChannelsAction = UIAlertAction( + title: "Blocked/Unblocked with Matching Hidden Channels", style: .default, handler: { [weak self] _ in - self?.title = "Blocked Unblocked with Matching Hidden Channels" + self?.title = "Blocked/Unblocked with Matching Hidden Channels" self?.setBlockedUnblockedWithHiddenChannelsQuery() } )And update its usage in the actions list (Line 252):
- blockedUnBlockedExcludingDeletedChannelsAction, + blockedUnblockedWithHiddenChannelsAction,
268-274: Method naming is consistent; consider grouping “query setters” together and documenting themMinor: You already follow a pattern with
setXxxQuery(). Consider adding a brief doc comment explaining each query’s purpose for demo users scanning the code.Tests/StreamChatTests/StateLayer/ChannelList_Tests.swift (1)
346-404: Solid regression test; add a state-layer assertion for the specific channel’s flagsThe test validates DB and overall count. Add explicit assertions that the state-layer view of the same channel preserves
isBlocked/isHidden, ensuring end-to-end correctness (DB → FRC → state).@@ let secondChannelDataAfterEvent = try env.client.databaseContainer.readSynchronously { session in try XCTUnwrap(session.channel(cid: secondChannel.cid)).asModel() } XCTAssertEqual(true, secondChannelDataAfterEvent.isBlocked, "State did not change") XCTAssertEqual(true, secondChannelDataAfterEvent.isHidden, "State did not change") + // Also assert the state-layer reflects the same flags for the specific channel. + let stateChannel = await channelList.state.channels.first(where: { $0.cid == secondChannel.cid }) + XCTAssertEqual(true, stateChannel?.isBlocked) + XCTAssertEqual(true, stateChannel?.isHidden) await XCTAssertEqual(5, channelList.state.channels.count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)DemoApp/StreamChat/Components/DemoChatChannelListVC.swift(4 hunks)Sources/StreamChat/Database/DTOs/ChannelDTO.swift(1 hunks)TestTools/StreamChatTestTools/TestData/DummyData/ChannelDetailPayload.swift(1 hunks)TestTools/StreamChatTestTools/TestData/DummyData/XCTestCase+Dummy.swift(4 hunks)Tests/StreamChatTests/StateLayer/ChannelList_Tests.swift(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/StreamChatTests/StateLayer/ChannelList_Tests.swift (4)
Sources/StreamChat/Query/ChannelListQuery.swift (1)
containMembers(84-86)TestTools/StreamChatTestTools/SpyPattern/Spy/APIClient_Spy.swift (1)
test_mockResponseResult(118-120)TestTools/StreamChatTestTools/Mocks/StreamChat/Database/DatabaseSession_Mock.swift (2)
channel(400-402)user(101-103)TestTools/StreamChatTestTools/SpyPattern/Spy/DatabaseContainer_Spy.swift (1)
readSynchronously(162-172)
⏰ 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: Automated Code Review
- GitHub Check: Test LLC (Debug)
- GitHub Check: Metrics
🔇 Additional comments (4)
Tests/StreamChatTests/StateLayer/ChannelList_Tests.swift (1)
549-565: Nice extensibility with per-channelblocked/hiddenclosuresThis makes tests expressive and focused. No changes needed.
TestTools/StreamChatTestTools/TestData/DummyData/XCTestCase+Dummy.swift (3)
194-195: Pass-through of optional blocked/hidden into ChannelPayload looks correctPropagating
blocked/hiddenas optionals enables simulating both explicit values and field absence (nil). This aligns with the PR goal to prevent nil WS fields from overwriting existing state in tests.
414-416: Canonicalize Poll dictionaries to concrete types with empty defaults — LGTMSwitching to
[String: RawJSON],[String: [PollVotePayload]], and[String: Int]with[:]defaults improves type clarity and reduces optional handling in tests.
506-506: No functional changeLooks like a harmless normalization; nothing to do.
SDK Performance
|
| @@ -48,6 +48,16 @@ final class DemoChatChannelListVC: ChatChannelListVC { | |||
| .equal(.blocked, to: true) | |||
| ]) | |||
| ])) | |||
|
|
|||
| lazy var blockedUnblockedWithHiddenChannelsQuery: ChannelListQuery = .init( | |||
There was a problem hiding this comment.
why this one? We already have:
lazy var allBlockedChannelsQuery: ChannelListQuery = .init(filter: .and([
.containMembers(userIds: [currentUserId]),
.or([
.equal(.blocked, to: false),
.equal(.blocked, to: true)
])
]))
This is the one customers would use.
There was a problem hiding this comment.
What I learned was that they are going to use the one with blocked and hidden fields.
| @@ -285,7 +285,7 @@ extension NSManagedObjectContext { | |||
| // for blocked 1:1 channels on channel list query | |||
| if let isBlocked = payload.isBlocked { | |||
| dto.isBlocked = isBlocked | |||
| } else { | |||
| } else if query != nil { | |||
There was a problem hiding this comment.
is this condition enough? I think it's better that this is only solved on the backend.
There was a problem hiding this comment.
To be honest I am not sure why the else part was added anyway. But there is a comment just above which seems to indicate that this is required. Ideally we wouldn't change the local state if payload does not have it. Currently I only change it in a way that it matches to the comment above. Maybe I should try to remove the else part completely. 🤔
There was a problem hiding this comment.
that could be dangerous too - if the backend just removes the field if you unblock someone.
There was a problem hiding this comment.
Looking at the git history I found this comment, which still doesn't clarify the need for this fallback. I believe the existing isBlocked state should remain unchanged if a websocket event payload doesn’t include it, wdyt?
There was a problem hiding this comment.
I removed the else part and tested our manual regression testing test cases, all was good! Just in case added default value to the Core Data model as well since it did not have it before (looks mistake, but it was fine since ChannelDTO always updated this property).
| @@ -49,7 +49,7 @@ | |||
| <attribute name="hasUnreadSorting" attributeType="Integer 16" defaultValueString="0" usesScalarValueType="YES"/> | |||
| <attribute name="id" optional="YES" attributeType="String"/> | |||
| <attribute name="imageURL" optional="YES" attributeType="URI"/> | |||
| <attribute name="isBlocked" optional="YES" attributeType="Boolean" usesScalarValueType="YES"/> | |||
| <attribute name="isBlocked" optional="YES" attributeType="Boolean" defaultValueString="NO" usesScalarValueType="YES"/> | |||
There was a problem hiding this comment.
Safe if default value is set, but it is optional (since now ChannelDTO might not update the property anymore).
| } else { | ||
| dto.isBlocked = false | ||
| } |
There was a problem hiding this comment.
Went through manual regression testing steps for blocked, and all was good.
SDK Size
|
|
@laevandus check the LLC test failures before merging |
|
🔗 Issue Links
Resolves: IOS-1094
🎯 Goal
Channel list removes blocked channel on it is presented
📝 Summary
🛠 Implementation
If there is a need for showing both blocked and unblocked channels (blocked are automatically hidden) then one needs to have a query looking like this:
This works fine until blocked channel is opened which triggers notification.mark_read WS event which has channel data, but blocked field is nil. This triggered SDK to incorrectly setting isBlocked of the channel to false which in turn triggered removing it from the channel list.
🎨 Showcase
🧪 Manual Testing Notes
Result: that channel is still there.
☑️ Contributor Checklist
docs-contentrepoSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Documentation