feat: use network and WS connection observer services from LLC - #3281
Open
MartinCupela wants to merge 8 commits into
Open
feat: use network and WS connection observer services from LLC#3281MartinCupela wants to merge 8 commits into
MartinCupela wants to merge 8 commits into
Conversation
…ng i18n init The persistent "Waiting for network…" notification was removed moments after being published, whenever the socket dropped before `Streami18n.init()` resolved — an offline app launch, a captive portal, an expired token. That is precisely when the banner is wanted, and there was none. Dismissal was part of the subscription effect's cleanup, so any change to that effect's dependencies destroyed the notification, and `t` changes identity when init completes (the constructed placeholder gives way to i18next's real one). Re-subscribing does not republish, so the banner was gone for good. Dismissal is now scoped to the mount, which is what it was always for. `t` stays in the subscription's dependencies, where the lint rule wants it; re-subscribing on a change is idempotent and leaves the notification alone. The existing test passed only because `waitFor` polls and caught the transient window between the notification being published and being removed. The new test drops the socket without awaiting anything first, so it lands inside the init window, and fails if dismissal is moved back into the subscription's cleanup.
`connection.changed` and `connection.recovered` carry `connection: 'network' | 'ws'` in stream-chat v10, and both variants reach every existing handler. A handler that ignores the field therefore reacts to the device's network as well as to our WebSocket, which would report a lost network every time the socket drops on a working one. Every subscription keeps its event name and its `online` field; each now narrows to `'ws'`, preserving today's behaviour exactly: - `useReportLostConnectionSystemNotification` publishes its banner for the socket only. The notification type says `network`, but the fact behind it has always been the socket. - `Channel` reloads its loaded message window on the socket's recovery only. The compiler cannot help here — both variants have the same payload shape, so an un-guarded handler keeps compiling and keeps behaving as before. Two tests fail if either guard is removed. `Channel` also drops its `connection.changed` subscription entirely. The branch that read it went with the dead `online` ref, and `handleEvent` has no catch-all, so the event reached the component and did nothing. The mock builders take an optional `ConnectionType`, defaulting to `'ws'` so existing call sites keep the meaning they had when this event could only be about the socket.
`ConnectionRecoveryManager` reloads every active channel and then dispatches `connection.recovered`. `Channel` handled that event by calling `channel.reload()` again, and since it marks its channel active while mounted, every open channel was reloaded twice per reconnect — two full `watch()` requests each. `Channel.reload()`'s `_reloading` flag is a re-entrancy guard and has already reset by the time the event is dispatched, so it did not collapse the pair. Measured at two reloads per reconnect before this change and one after. The handler goes, and with it the `connection.recovered` subscription: nothing else in `handleEvent` reacted to that event. The reconciliation React's reload existed for — a hard delete that happened offline arrives via no event, so only a re-query surfaces it — still happens, because the client reloads active channels itself. The old test dispatched `connection.recovered` directly, which is exactly why it never saw the duplicate; the replacement drives a whole reconnect from the socket coming back. `keeps rendering when the reload fails` moves with the behaviour it covered: the client reloads with `Promise.allSettled`, so a socket flapping mid-reload cannot throw into the component at all. Note the `stream-chat` pin is deliberately left at 10.0.0-rc.7. It has to move to the release that carries the connection work — rc.9 exports none of `ConnectionType`, `NetworkConnectionState` or `client.networkConnection`, which this branch already imports — so bumping it is a release-time step, not part of this change.
The persistent banner read "Waiting for network…" whenever `connection.changed` reported `online: false`. That event is the WebSocket, so the message told users their network was down when the server had closed the socket, the token had expired, or a health check had timed out on working Wi-Fi. It now reads both facts and picks the wording: - the device reports no network → "Waiting for network…" - the network is up, or unknown, and the socket is down → "Reconnecting…" One banner throughout, replaced rather than stacked when the reason changes. Choosing that grouping is a copy decision rather than a fact about connectivity, which is why the client publishes no combined status and why the decision is made here, in the component that renders the copy. The existing translation key keeps the network case, where its wording was always true, so its translations stay valid and only one new string was needed. It also reads the current state at mount. It reacted only to transitions before, so a client that was already offline showed nothing until something changed. Two things kept deliberately: - One notification type for both messages. Consumers filter banners on `system:network:connection:lost` — the SDK's own cookbook recipe does — so splitting it would silently stop those filters seeing the socket case. The `network` in the name is historical; the message is what was wrong. - The socket half stays on `connection.changed` rather than `client.wsConnection.state`. The event is held `WS_OFFLINE_ANNOUNCE_DELAY_MS` (5s) on the way down and dropped entirely if the socket returns inside that window, which is what stops a brief flap strobing the banner; the store publishes the raw edge. The socket's last announced value therefore lives in a ref seeded from the store only once — re-reading the store on every effect re-run discarded what the event had said and dismissed the banner. `ConnectionType` is imported from the client rather than redeclared, so a connection type added there breaks `yarn build` here until someone decides what the banner should say about it. Both signals are subscribed imperatively rather than through `useNetworkConnectionState` / `useWSConnectionState`: `Chat` calls this hook, and re-rendering the whole tree on every network flap is what those hooks exist to let consumers avoid.
Driving the two connection facts apart by hand is awkward, and the obvious tool is the wrong one: DevTools' "Offline" checkbox takes down `navigator.onLine` *and* the socket, which is the one combination that always worked. What needs exercising is a dead socket on a live network — a server close, an expired token, a health-check timeout — because that is the case the offline banner used to describe as "Waiting for network…". Two toggles, showing both facts and flipping each independently. Hidden behind a sidebar button that mirrors the theme and RTL toggles, backed by a `devTools.connectionPanel` setting that defaults to off. Both toggles simulate rather than sever, and the socket one writes the store *and* dispatches `connection.changed`, because that is what the real socket does: the store carries the raw state, the event is the announcement the banner listens to. Closing the real socket is no use for a toggle — it reconnects on its own within a second or two, so it would flip back by itself — and the one thing that does hold, `client.closeConnection()`, deliberately dispatches no event, so no banner appears. The component's doc gives the console one-liner for the genuine path, including the five-second announce delay the toggles cannot show. The German and Italian dictionaries here are complete by type assertion, so the new `chat.reportLostConnection.reconnecting.text` key needed translating in both — the assertion fails the build naming any key left out. Earned its keep immediately: it surfaced a regression where every watched request went out without a `connection_id`.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Depends on GetStream/stream-chat-js#1859
🎯 Goal
The offline banner said "you're offline" when the user's internet was fine.
It rendered off WebSocket health, and a socket dies for reasons that have nothing to do with the
user's network — the server closes it, an auth token expires, a keep-alive times out. On good
Wi-Fi, the app still blamed the network.
The SDK had no way to know better: it couldn't tell whether the device had a network, because
nothing told it. GetStream/stream-chat-js#1859
adds that missing signal — the device's network as a fact separate from the socket. This is the
React half: use it, and fix the things that were wrong once the two facts are told apart.
🛠 Implementation details
The banner now picks its wording from both facts. No network → "Waiting for network…". Network
up (or unknown) and socket down → "Reconnecting…". One banner throughout, replaced rather than
stacked when the reason changes. Deciding which of those to show is a copy decision, not a fact
about connectivity, which is why the client publishes no combined status and the choice is made
here, in the component that renders the copy.
It also reads the current state on mount. It only reacted to transitions before, so a client that
was already offline showed nothing until something changed.
Two hooks, one per fact, thin wrappers over
useStateStore, exported from the package:Aliased deliberately: both stores expose
isOnlineand they mean different things. There is nothird hook combining them, for the reason above.
useNetworkConnectionStateSelectoris there forcomponents that only want one field and shouldn't re-render on the others.
Note
networkOnlineisboolean | undefined—undefinedmeans no platform listener hasreported yet, which is the normal state on React Native until one is installed. Test it with
=== false;!networkOnlinealso fires when the answer is unknown, which would show a permanentoffline banner.
Every connection-event handler now narrows on the new
connectionfield. Both events reachevery handler, so one that ignores the field reacts to the device's network as well as to our
socket. The compiler can't catch a missing guard — both variants have the same payload shape — so
there are tests that fail if either guard is removed.
Fixed: open channels were reloaded twice per reconnect.
ConnectionRecoveryManagerreloadsevery active channel and then dispatches
connection.recovered;Channelhandled that event byreloading again, and it marks its channel active while mounted. Two full
watch()requests peropen channel, measured. The reload now happens once, in the client —
Channel's own handler andits subscription are gone.
Fixed: the banner vanished on a cold offline start.
Streami18n.init()is async andtchanges identity when it resolves. Dismissal was part of the subscription effect's cleanup, so a
socket dropping inside that window published the notification and then had it removed — no banner
on an offline app launch or behind a captive portal. Dismissal is now scoped to the mount, where
it belongs.
Removed a dead
onlineref inChannel. Written on everyconnection.changed, read bynothing since the v15 state migration.
Added a dev panel to the vite example, behind a sidebar toggle next to theme and RTL. Two
toggles that drive the network and the socket independently, because DevTools' "Offline" checkbox
takes down both at once — which is the one combination that always worked. It found a real
regression in the LLC during review.
🎨 UI Changes
Screenshots needed — I can't produce them. What to capture:
socket toggle, or
client.wsConnection.connection.ws.close()in the console). Previously thissaid "Waiting for network…".
Offline.
Behaviour changes for integrators
No API is removed, so nothing here is a
BREAKING CHANGE:, but three things are worth knowing.chat.reportLostConnection.reconnecting.text(default "Reconnecting…"). Theexisting
waitingNetworkkey keeps the network case, so its translations stay valid. A language you have not translated it into falls back to that English text, so nothing renders a raw chat.reportLostConnection… string. Add it to your dictionaries when you want it localized. The existing waitingNetwork key is unchanged and still used for the no-network case, so its translations stay valid.message, or on the
system:network:connection:lostnotification meaning "no network", should bere-checked. The notification type is unchanged so existing filters keep working; it has always
tracked the socket despite
networkin its name.Channelno longer reloads onconnection.recovered. If you relied on that component torefresh a channel after a reconnect, the client does it now.