fix(llc): update watcher count from realtime events and ignore events after dispose - #2898
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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: Pro Plus 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 |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v9 #2898 +/- ##
=====================================
Coverage ? 67.39%
=====================================
Files ? 431
Lines ? 27417
Branches ? 0
=====================================
Hits ? 18477
Misses ? 8940
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Submit a pull request
Linear: FLU-619, FLU-658
Github Issue: #
CLA
Description of the pull request
Backports two
llcfixes frommastertov9. They are independent; each is a separate commit.1. Watcher count / watchers from realtime events — FLU-619
Backport of master commit
84c6a24(originally #2837).Why
ChannelClientState.watcherCountandChannelClientState.watchersnever reflected realtime events during a session:watcher_countonuser.watching.start/user.watching.stop/message.newwas dropped intoEvent.extraData, so the count stayed frozen at the last fullchannel.watch()/queryChannelssnapshot.watchersonuser.watching.stop— the additive keyed merge inupdateChannelState(needed for watcher pagination) can only upsert, so the departing watcher lingered until the next full query.iOS and Android already trust the server-provided
watcher_countper event, so Flutter was the outlier.Changes
Event.watcherCount, mapping the serverwatcher_countfield (promoted to a first-class field instead ofextraData).user.watching.start,user.watching.stop, andmessage.new.ChannelClientState.watchersonuser.watching.stop(assign_channelStatedirectly, bypassing the additive merge).notification.message_new/notification.thread_message_neware intentionally excluded: the backend fetches the watcher count only after the notification fan-out, so these events structurally carrywatcher_count: 0.Port notes
Manual port, not a clean cherry-pick:
v9uses the 80-col default, so every touched hunk needed rewrapping.Event.deletedForMe— a master-only field adjacent to the ported hunks inevent.dart; not brought over._listenMessageNew— master extracted this handler's body intoaddNewMessage, which does not exist onv9. The extracted body was verified byte-for-byte identical tov9's inline body, so the watcher-count block was appended at the equivalent point (aftersubmitForDelivery).event.g.dart— regenerated withjson_serializablepinned to6.9.5. The currently-resolving6.14.1emits Dart 3.10 null-aware map elements ('key': ?instance.x), whichv9'ssdk: ^3.6.2rejects. Output matchesv9's existingif (instance.x case final value?)style. Note separately: this meansmelos run generate:dartis currently broken onv9with an up-to-date toolchain, independent of this PR.Performance note
The
message.newhandler's extraupdateChannelStatecall is O(1), not a message-list walk:copyWith(watcherCount:)preserves themessageslist reference, andmergeshort-circuits onidentical(other, this). It does add one extrachannelStateStreamemission, in a path that already emits multiple times per message (updateMessage, then theunreadCountsetter). Same shape and cost as onmaster.2. Ignore events dispatched after the client is disposed — FLU-658
Backport of master commit
4ba19d7(originally #2855).Why
StreamChatClient.handleEventcould throwStateError: Cannot add new events after calling closewhen the client was disposed while a reconnect recovery was still in flight._onConnectionStatusChangedisasync. On an offline→online transition it awaits a recovery pipeline (sync/queryChannelsOnline) and only then emits the trailingconnectionRecoveredevent, driven by a fire-and-forget subscription. Ifdispose()closes_eventControllerwhile that continuation is suspended on anawait, the trailinghandleEventadds to a closed controller — surfacing as an unhandled async error.Changes
Guard
handleEventagainst a closed controller (early return onisClosed, plussafeAdd).Safety analysis on
v9Verified directly against the
v9sources rather than inherited from the master PR:_eventController.close()has a single call site, insidedispose().disconnectUser()closes the WS and resets state/credentials/persistence but leaves the event controller open, so the logout→login reuse path is unaffected — the guard cannot swallow events on a live or reused client._eventControlleris a non-latefinalinitialized inline and never reassigned, soisClosedis terminal for the client's lifetime.close()the broadcast stream is already complete, so no subscriber could have received these events; the guard removes the throw, not a delivery.Semantic equivalence with master, despite a controller-type divergence: master's
_eventControlleris a master-onlyEventController<Event>,v9's is aPublishSubject<Event>— but both are rxdartSubjectsubclasses backed by a broadcastStreamController, soisClosedis the same inherited implementation (Subject.isClosed => _controller.isClosed) andsafeAddresolves to the sameStreamControllerXextension on both. The controllers differ only in where the poll-vote→poll-answer normalization happens (master overridesEventController.add;v9maps it in theeventStreamgetter), which is orthogonal to the guard.Behaviour change worth naming (identical placement on master): because the guard precedes the health-check branch, a
health.checkarriving after dispose no longer runs_handleHealthCheckEvent. That is correct —disconnectUser()has by then reset the connection-id manager and closed the persistence connection, so those writes would target torn-down objects.Port notes
The
client.darthunk applies cleanly (v9'shandleEventis byte-identical to master's pre-fix version). Two adaptations:v9'sclient.dartlackedimport 'package:stream_chat/src/core/util/extension.dart'(master has it), sosafeAdddid not resolve until it was added. This is the only line beyond master's diff.main(). Master inserts it beforegroup('WS events'), which does not exist inv9'sclient_test.dart.import 'dart:async'was also added forCompleter.Tests
FLU-619 — new
Watching Eventsgroup inchannel_test.dart: start/stop update the count, stop removes the watcher, a null count preserves the existing value,message.newupdates the count,message.newwithout a count preserves it, andnotification.message_newdoes not overwrite it.Beyond the master commit,
event_test.dart's existing parse / serialize /copyWithtests andtest/fixtures/event.jsonwere extended to coverwatcher_count, following this repo's convention of extending existing serialization tests when adding model fields.FLU-658 — new
dispose during reconnect recoverygroup inclient_test.dart, disposing the client while a recovery is suspended on a held-openqueryChannels, then resuming it. Verified as a genuine regression test: with the guard reverted it fails with exactlyBad state: Cannot add new events after calling close; with the guard it passes.Verification
melos run format— clean (0 changed).melos run analyze—stream_chat: No issues found.stream_chatsuite: 1309 pass, 2 skipped, 0 fail.Screenshots / Videos
No UI changes.