Fix Channel List / Channels showing no data after reconnection #1435
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ConnectionRecoveryUpdater does nothing useful. it’s broken because it calls /sync with the wrong date (we don’t get missing events), furthermore it duplicates channel query calls (with wrong sorting if it’s different from default)
Removing channels completely leads to bugs where they're not unlinked from query. We should reset channel data (which would be recovered when channel is queried) and unlink channel
b-onc
force-pushed
the
hotfix/channel-list-after-connection-recovery
branch
from
10d847b
to
d179cd7
Compare
tbarbugli
requested changes
Sep 9, 2021
| @@ -229,16 +229,7 @@ public class ChatClient { | |||
| ] | |||
|
|
|||
| // All production event workers | |||
| eventWorkerBuilders = [ | |||
There was a problem hiding this comment.
@b-onc better if you comment this instead deleting + add a /// patch comment
| @@ -129,7 +129,7 @@ public class ChatChannelListController: DataController, DelegateCallable, DataSt | |||
| callback: { [unowned self] in | |||
| switch $0.webSocketConnectionState { | |||
| case .connected: | |||
| self.updateChannelList(trumpExistingChannels: self.channels.count > self.requestedChannelsLimit) | |||
| self.updateChannelList(trumpExistingChannels: true) | |||
There was a problem hiding this comment.
AFAICT this is not a needed change, I would leave it alone
There was a problem hiding this comment.
Hm, I would have it 🤔
When we reconnect, we should invalidate history of local channels because there can be changes we are not aware of.
Use case:
- load 1st page (25 channels)
- go offline
- user becomes a member of a new channel that goes to 1st page (forcing locally existed channel X out from 1st page)
- smth changes in channel X outside the 1st page of messages
- go back online and do not trump local data
The history of channel X will be out of sync and user won't see the updates
There was a problem hiding this comment.
I would trump on synchronise/reconnect no matter the storage setup. With no /sync and enabled local storage, queries will go out of sync very soon
| @@ -162,6 +162,8 @@ protocol ChannelDatabaseSession { | |||
| query: ChannelListQuery? | |||
| ) throws -> ChannelDTO | |||
|
|
|||
| @discardableResult func saveQuery(query: ChannelListQuery) -> ChannelListQueryDTO | |||
| @@ -27,7 +27,8 @@ class ChannelListUpdater: Worker { | |||
| self?.database.write { session in | |||
|
|
|||
| if trumpExistingChannels { | |||
| try session.deleteChannels(query: channelListQuery) | |||
| let queryDTO = session.saveQuery(query: channelListQuery) | |||
| @@ -72,7 +72,7 @@ class DatabaseCleanupUpdater: Worker { | |||
| } | |||
| } | |||
|
|
|||
| private extension ChannelDTO { | |||
| extension ChannelDTO { | |||
There was a problem hiding this comment.
Extension contains ChannelDTO .resetLocalData used in channel list updater to trump a query -> we need this change 🙂
| @@ -165,6 +165,10 @@ extension DatabaseSessionMock { | |||
| underlyingSession.loadChannelReads(for: userId) | |||
| } | |||
|
|
|||
| func saveQuery(query: ChannelListQuery) -> ChannelListQueryDTO { | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discussion in slack