-
Notifications
You must be signed in to change notification settings - Fork 224
Add support for user deleted messages event #3792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…st in the local DB
WalkthroughAdds support for a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WS as WebSocket
participant MW as UserChannelBanEventsMiddleware
participant DB as Database
participant EC as EventsController
participant LSC as LivestreamChannelController
WS->>MW: user.messages.deleted payload (userId, hardDelete, createdAt)
MW->>DB: Resolve UserDTO and messages for userId
alt hardDelete = true
MW->>DB: Mark messages isHardDeleted = true
else hardDelete = false
MW->>DB: Set messages.deletedAt = createdAt
end
MW-->>EC: Forward UserMessagesDeletedEventDTO / domain event
EC-->>LSC: didReceiveEvent(UserMessagesDeletedEvent)
alt hardDelete = true
LSC->>LSC: Remove loaded messages authored by userId
else hardDelete = false
LSC->>LSC: Set deletedAt = createdAt for loaded messages authored by userId
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (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). (4)
✨ 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 (
|
Public Interface+ public struct UserMessagesDeletedEvent: Event
+
+ public let user: ChatUser
+ public let hardDelete: Bool
+ public let createdAt: Date
|
SDK Size
|
SDK Performance
|
SDK Size
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (11)
Sources/StreamChat/WebSocketClient/Events/UserEvents.swift (2)
275-285
: ClarifyhardDelete
semantics in docs (maps to soft vs hard delete).The payload in the linked issue mentions
soft_delete: true
. Our domain event exposeshardDelete: Bool
. Please add a short note in the comment to avoid ambiguity: "hardDelete == true
means messages are permanently removed; otherwise they are soft-deleted (deletedAt set)."Apply this doc tweak:
/// Triggered when the messages of a banned user should be deleted. public struct UserMessagesDeletedEvent: Event { /// The banned user. public let user: ChatUser - /// If the messages should be hard deleted or not. + /// Whether messages should be hard deleted (`true`) or soft deleted (`false`). + /// Note: This is derived from the payload (soft_delete vs hard_delete). public let hardDelete: Bool
298-314
: Be consistent about DTO-to-domain fallback behavior.Unlike other user events here, this DTO falls back to the payload user when the DB user is missing. That’s likely the right call for a global sweep event; however, the difference is subtle. Either:
- add a brief comment explaining why fallback is intentional here, or
- adopt the same fallback pattern for other user events (presence/updated), if appropriate.
Proposed inline comment:
- func toDomainEvent(session: DatabaseSession) -> Event? { + func toDomainEvent(session: DatabaseSession) -> Event? { + // Intentionally fall back to payload user so global delete events are not dropped + // when the user is not present in the local DB. if let userDTO = session.user(id: user.id), let userModel = try? userDTO.asModel() { return UserMessagesDeletedEvent( user: userModel, hardDelete: payload.hardDelete, createdAt: createdAt ) } return UserMessagesDeletedEvent( user: user.asModel(), hardDelete: payload.hardDelete, createdAt: createdAt ) }Sources/StreamChat/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware.swift (2)
33-44
: Apply both flags for hard delete and avoid redundant writes.Two improvements:
- For hard deletes, set both
isHardDeleted = true
anddeletedAt = createdAt
to keep invariants aligned with other delete paths.- Skip updates when the message is already in the desired state to reduce churn.
Apply this diff:
case let userMessagesDeletedEvent as UserMessagesDeletedEventDTO: let userId = userMessagesDeletedEvent.user.id if let userDTO = session.user(id: userId) { - userDTO.messages?.forEach { message in - if userMessagesDeletedEvent.payload.hardDelete { - message.isHardDeleted = true - } else { - message.deletedAt = userMessagesDeletedEvent.createdAt.bridgeDate - } - } + userDTO.messages?.forEach { message in + if userMessagesDeletedEvent.payload.hardDelete { + // No-op if already hard-deleted + if message.isHardDeleted == false { + message.isHardDeleted = true + // Keep parity with other deletion paths + if message.deletedAt == nil { + message.deletedAt = userMessagesDeletedEvent.createdAt.bridgeDate + } + } + } else { + // Soft delete only when needed + if message.deletedAt == nil { + message.deletedAt = userMessagesDeletedEvent.createdAt.bridgeDate + } + } + } }
33-44
: Leverage existing delete cascade in MessageRepository; no manual reply handling neededThe repository already cascades hard-deletes to replies when you set
message.isHardDeleted = true
. Rather than adding manual loops here, rely on that centralized logic and, if needed, extend it for other related entities (quotes, pinned state, counters):• In
Sources/StreamChat/Repositories/MessageRepository.swift
(around line 287), you’ll see:if messageDTO.isHardDeleted { session.delete(message: deletedMessage) messageDTO.replies.forEach { session.delete(message: $0) } }This automatically deletes the parent and all its replies.
• If you also need to clear quote references, pinned flags, counters, etc., add those rules to the same repository—either in the
delete(message:)
path or via DB-level cascades—so all delete logic remains in one place.No additional helper methods are required in the middleware.
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift (2)
2302-2390
: Good coverage for soft-delete semantics ofUserMessagesDeletedEvent
.
- Verifies both targeted and non-targeted users, and asserts deletedAt equals event.createdAt.
- Consider using a deterministic timestamp helper (e.g.,
.unique
) for consistency with the rest of the suite, instead ofDate()
. This can reduce flakiness if tests ever rely on time comparisons elsewhere.
2391-2479
: Hard-delete path assertions look solid.
- Correctly validates removal of only the banned user’s messages and preserves other messages without mutating their deletedAt.
- Optional: Add a quick assert that message ordering of remaining items is preserved (if that’s important to consumers), though not strictly required here.
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift (2)
1106-1111
: Also remove pinned messages from the channel on hard delete (optional, keeps in-memory state consistent).When hard-deleting a user’s messages, any pinned messages authored by that user will remain in
channel?.pinnedMessages
. If the expected UX is that hard-deleted messages disappear everywhere, we should trim them from pinned messages too.Apply this diff to update
hardDeleteMessages(from:)
:- private func hardDeleteMessages(from userId: UserId) { - messages.removeAll { message in - message.author.id == userId - } - } + private func hardDeleteMessages(from userId: UserId) { + // Collect the ids we are about to remove to keep channel pinnedMessages in sync. + let removedIds = Set(messages.filter { $0.author.id == userId }.map(\.id)) + guard !removedIds.isEmpty else { return } + + // Remove from the in-memory timeline. + messages.removeAll { removedIds.contains($0.id) } + + // Keep pinned messages consistent in-memory as well. + if var pinned = channel?.pinnedMessages { + pinned.removeAll { removedIds.contains($0.id) } + channel = channel?.changing(pinnedMessages: pinned) + } + }
1094-1104
: Minor micro-optimization (optional): update in place to avoid reallocating the array.
map
is perfectly fine here; if you ever profile hotspots, updating by index can avoid a full-copy. Not a blocker.Example (no behavior change):
for i in messages.indices where messages[i].author.id == userId { messages[i] = messages[i].changing(deletedAt: deletedAt) }Tests/StreamChatTests/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware_Tests.swift (3)
199-244
: Soft delete middleware test is thorough; silence an unused variable to avoid warnings.The local
userDTO
binding is only used to assert existence viaXCTUnwrap
; it’s not read afterwards. Replace with_ =
to avoid an “unused variable” warning.Apply this diff:
- let userDTO = try XCTUnwrap(database.viewContext.user(id: userId)) + _ = try XCTUnwrap(database.viewContext.user(id: userId))
246-291
: Hard delete middleware test looks good; same minor_ =
nit.Mirror the
_ = try XCTUnwrap(...)
change here as well.Apply this diff:
- let userDTO = try XCTUnwrap(database.viewContext.user(id: userId)) + _ = try XCTUnwrap(database.viewContext.user(id: userId))
323-350
: DTO → domain event falls back to payload user when DB user is missing — correct.Nice coverage for the fallback path; aligns with resilient event decoding.
Do you also want a middleware test that asserts graceful handling when the user doesn’t exist in DB (i.e., no crash, event forwarded unchanged, and no writes)? I can draft it.
📜 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 (8)
CHANGELOG.md
(1 hunks)Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift
(2 hunks)Sources/StreamChat/Models/ChatMessage.swift
(1 hunks)Sources/StreamChat/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware.swift
(1 hunks)Sources/StreamChat/WebSocketClient/Events/EventType.swift
(2 hunks)Sources/StreamChat/WebSocketClient/Events/UserEvents.swift
(1 hunks)Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift
(1 hunks)Tests/StreamChatTests/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware_Tests.swift
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
Sources/StreamChat/Models/ChatMessage.swift (1)
Sources/StreamChat/Models/Channel.swift (1)
lhs
(379-405)
Tests/StreamChatTests/Controllers/ChannelController/LivestreamChannelController_Tests.swift (1)
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift (1)
eventsController
(792-807)
Sources/StreamChat/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware.swift (3)
Sources/StreamChat/Database/DTOs/UserDTO.swift (2)
user
(34-40)user
(138-140)TestTools/StreamChatTestTools/Mocks/StreamChat/Database/DatabaseSession_Mock.swift (2)
user
(101-103)message
(288-290)Sources/StreamChat/Database/DTOs/MessageDTO.swift (2)
message
(454-460)message
(1342-1342)
Sources/StreamChat/WebSocketClient/Events/UserEvents.swift (2)
Sources/StreamChat/Database/DTOs/UserDTO.swift (3)
user
(34-40)user
(138-140)asModel
(194-194)Sources/StreamChat/WebSocketClient/Events/EventPayload.swift (2)
value
(234-240)value
(243-253)
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift (1)
Sources/StreamChat/Models/ChatMessage.swift (1)
changing
(277-334)
Tests/StreamChatTests/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware_Tests.swift (5)
Sources/StreamChat/Database/DTOs/UserDTO.swift (2)
user
(34-40)user
(138-140)TestTools/StreamChatTestTools/SpyPattern/Spy/DatabaseContainer_Spy.swift (4)
createCurrentUser
(192-209)createChannel
(212-248)createMessage
(283-368)createMessage
(370-394)Sources/StreamChat/Database/DTOs/MessageDTO.swift (2)
message
(454-460)message
(1342-1342)Sources/StreamChat/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware.swift (1)
handle
(9-53)Sources/StreamChat/WebSocketClient/Events/UserEvents.swift (8)
toDomainEvent
(27-34)toDomainEvent
(57-64)toDomainEvent
(104-114)toDomainEvent
(139-146)toDomainEvent
(194-206)toDomainEvent
(229-236)toDomainEvent
(264-272)toDomainEvent
(298-313)
🔇 Additional comments (4)
Sources/StreamChat/WebSocketClient/Events/UserEvents.swift (1)
301-305
: Verified:hardDelete
is correctly parsed from the payloadI checked the
EventPayload
definition inSources/StreamChat/WebSocketClient/Events/EventPayload.swift
and found:
- Coding key “hard_delete” is mapped to
hardDelete
in theCodingKeys
enum (case hardDelete = "hard_delete"
)- The
hardDelete
property is declared aslet hardDelete: Bool
with a default value offalse
in the initializer (hardDelete: Bool = false
)- During decoding, it uses
decodeIfPresent(Bool.self, forKey: .hardDelete) ?? false
, ensuring a safe default offalse
when the key is absentSince
payload.hardDelete
is already backed by this mapping and defaulting logic, no changes are necessary here.Sources/StreamChat/Models/ChatMessage.swift (1)
557-557
: Good call: includedeletedAt
in equality to surface soft-delete changes.This will ensure lists refresh when a message transitions to or from soft-deleted state.
Sources/StreamChat/Controllers/ChannelController/LivestreamChannelController.swift (1)
793-806
: Handle globalUserMessagesDeletedEvent
separately from channel-scoped events — nice.
- Restricting channel-scoped handling to
channelEvent.cid == cid
prevents cross-channel leakage.- Processing the global user-messages-deleted event irrespective of
cid
is correct and aligns with the event’s semantics.If a pinned message authored by the banned user is hard-deleted, should it also be removed from
channel.pinnedMessages
in-memory to avoid stale UI? If yes, see the follow-up change below onhardDeleteMessages(from:)
.Tests/StreamChatTests/WebSocketClient/EventMiddlewares/UserChannelBanEventsMiddleware_Tests.swift (1)
293-321
: DTO → domain event uses DB user when present — correct.This validates the preference for the DB user model, preserving richer/consistent local data. No action needed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Verified both controllers and with soft and hard delete.
🔗 Issue Links
https://linear.app/stream/issue/IOS-1088
🎯 Goal
Add support for user deleted messages event.
📝 Summary
Add support for user deleted messages event for both ChannelController and LivestreamController. When this event is triggered, it should soft delete or hard delete (depending on the event) all the user's messages of the banned user.
🛠 Implementation
For the ChannelController, the
UserChannelBanEventsMiddleware
is responsible for handling this event.For the LivestreamController, it also goes through the
UserChannelBanEventsMiddleware
but it is the responsibility of the controller to apply the changes. For the this case, the middleware will only be used to forward the event to the livestream controller. Using theManualEventHandler
would not be possible at the moment, because the events need have acid
and this is a global event.🧪 Manual Testing Notes
How to trigger this event:
Make sure to test for both
ChannelController
andLivestreamController
. When the event is triggered, the messages from the user should be updated to soft deleted or hard deleted depending on what was selected.☑️ Contributor Checklist
docs-content
repoSummary by CodeRabbit
New Features
Documentation
Tests