feat: new reachability for multibackend - WPB-20050 #3543
Conversation
This file contains hidden or 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
Test Results3 498 tests 3 471 ✅ 3m 50s ⏱️ Results for commit ed38504. ♻️ This comment has been updated with latest results. |
johnxnguyen
requested review from
David-Henner,
KaterinaWire,
netbe and
samwyndham
johnxnguyen
commented
Sep 5, 2025
WireNetwork/Sources/WireNetwork/Network/NetworkStack/NetworkReachability.swift
Outdated
Show resolved
Hide resolved
samwyndham
approved these changes
Sep 5, 2025
netbe
reviewed
Sep 5, 2025
There was a problem hiding this comment.
@johnxnguyen I believe there might be other cases besides the internet banner:
- Looking at
reachability.mayBeReachablein the code, we also used reachability to reopen the websocket. i believe we don't at the moment -> but that could go under -> https://wearezeta.atlassian.net/browse/WPB-19904 - ZMTransportRequestScheduler seems to change of status if we're offline
Comment on lines
+496
to
+499
| self.isNetworkReachableCancellable = networkReachability.isReachablePublisher | ||
| .sink { [weak self] isReachable in | ||
| isReachable ? self?.didReceiveData() : self?.didGoOffline() | ||
| } |
There was a problem hiding this comment.
question: do we need to shutdown ReachabilityWrapper if multibackend is on?
|
I'm not sure actually this PR is necessary for the initial release: the old ZMReachability should still work for individual accounts because there are separate instances configured with the selected backend. I'll leave this PR open to finish later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Issue
This PR introduces a new
NetworkReachabilitythat makes use ofNWPathMonitorto report whether the app has an internet connection. This is in contrast to the oldZMReachabilitywhich is written in Objective C, uses a lower level C style network API, and checks reachability of specific hosts.A key difference between the two is that although
NetworkReachabilityis much simpler, it only tells us if there is a connection available and not whether a connection to the Wire backends is available. However, I observed that we only use the existing reachability to hide and show the "no internet" banner, so I think this is fine. If we want to check host reachability, we could extend the code to also send pings to specific hosts.Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: