fix(mobile): give Riverpod state classes value equality - #7318
Open
vguerci wants to merge 4 commits into
Open
Conversation
RelaySessionNotifier._connect re-emits SessionState(reconnecting, attempt: n) with the attempt it already holds, so a reconnect publishes a fresh object carrying values identical to the one _scheduleReconnect just set. SessionState declared no ==, so Riverpod compared by identity and notified every watcher. 25 sites watch relaySessionProvider. Each redundant emission disposed and rebuilt them all, and a rebuild that re-subscribes against the relay perturbs the session that triggered it. Anything needing several round-trips could not finish. _scheduleReconnect genuinely increments the attempt, so real reconnects still notify; only the no-op re-emission is filtered. The equality test builds its operands through a non-const path on purpose: identical const instances are canonicalized to a single object, so a const pair compares equal on identity even with no operator== at all. Signed-off-by: Vincent Guerci <vincent@guerci.com> (cherry picked from commit 302692df299e1cd612c3142aaa40a4252bd71edb)
ReadStateNotifier.build() disposes and recreates its manager, then calls
ref.watch(activeCommunityProvider). Community declared no ==, so Riverpod
compared by identity and every community save tore the manager down and built a
new one.
With push notifications enabled that becomes self-sustaining:
reservePushLeaseGeneration saves the community on every publish attempt, the
save rebuilds the read-state manager, the rebuild re-subscribes against the
relay, and the resulting churn fails the publish that started it. Device logs
show the cycle repeating about twice a second, each pass replaying the full
read-state history, racing its own NIP-42 auth ("auth-required: not
authenticated") and ending in "Push lease bootstrap failed: Connection lost".
The comment directly above that watch documents this exact hazard for
SessionState. The same trap applied to the line below it.
Equality is field-by-field so a new field cannot silently join it, and delegates
the nested push state to buzzPushSubscriptionStateFingerprint, which the
codebase already treats as that type's canonical comparison.
Signed-off-by: Vincent Guerci <vincent@guerci.com>
(cherry picked from commit 98e2eadc55764aa5fe04bacefbdc5deaaec3d7c8)
Community equality alone does not stop the read-state rebuild loop. activeCommunityProvider is a FutureProvider, so every recompute passes through AsyncLoading before AsyncData. ReadStateNotifier watched the whole AsyncValue and so rebuilt on that transition however equal the resulting Community was — measured at four rebuilds per re-emission with == already in place. Selecting the value puts the comparison on Community instead of the AsyncValue. Both halves are load-bearing: removing either returns the count to four, which the added test pins in both directions. The test drives the production notifier against the existing inert fake relay and forces a fresh Community carrying identical values, which is exactly what reservePushLeaseGeneration does on every publish attempt. Signed-off-by: Vincent Guerci <vincent@guerci.com> (cherry picked from commit 29bb4ae0d1ac67f17ab770fde4b847242945aa47)
RelaySessionNotifier.build() watches relayConfigProvider, registers ref.onDispose(_dispose) and schedules _connect(), so a rebuild destroys the socket and opens a new one. RelayConfigNotifier.build() returns a fresh RelayConfig on every rebuild, and RelayConfig declared no ==, so Riverpod compared by identity and treated an unchanged config as new. That closes a self-sustaining loop once push is on: reservePushLeaseGeneration saves the community on every publish attempt, the save re-emits the active community, the config rebuilds, the session is disposed and reconnected, and the publish fails over the socket it just tore down. Device logs show the cycle about twice a second, with "Relay session is disposed", "auth-required: not authenticated" from requests racing the new socket's NIP-42, and "Push lease bootstrap failed: Connection lost". The doc comment on baseUrl already recorded the hazard — Riverpod's default updateShouldNotify falls back to identity, and "a fresh instance per rebuild would resubscribe every listener". It held only for the const fallback; the community-derived config is built fresh each time. Equality compares the stored origin rather than the canonical one, because storedOrigin keys identity-scoped preferences and two differently-stored origins that canonicalize alike are not interchangeable. The community watch is also selected, so the FutureProvider's AsyncLoading transition alone no longer rebuilds the config. Signed-off-by: Vincent Guerci <vincent@guerci.com> (cherry picked from commit 4642c63be3e03a5f6d7774c0a08b3d042508ef1a)
🔐 Codex Security Review
|
Chessing234
approved these changes
Sep 5, 2026
Chessing234
left a comment
Chessing234
left a comment
Contributor
There was a problem hiding this comment.
adding ==/hashCode on the community + relay state objects matches the riverpod rebuild symptom. tests pin the publish-path identity churn.
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
2 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.
Summary
An unchanged relay config was tearing down and reconnecting the mobile relay
socket, roughly twice a second, once push notifications are on.
Riverpod's default
updateShouldNotifyisprevious != next, so a state classwith no
==is compared by identity and a rebuild producing anidentical-but-new object counts as a change.
RelayConfigNotifierreturns afresh
RelayConfigevery rebuild andRelaySessionNotifierdisposes its socketon rebuild, so a publish tore down the session it was publishing over.
This gives value equality to
RelayConfig,CommunityandSessionState, andstops two providers watching
activeCommunityProvider'sAsyncValuewrapperwhen they only need the value inside it — a
FutureProviderpasses throughAsyncLoadingon every recompute, which rebuilt them on its own.It only reproduces with push enabled, which is off by default and landed in
PR #6269 on 2026-08-28 — hence no issue for it. Hand-written
==withObject.hashmatches the existing convention in this package; no codegendependency is added.
Related issue
None found. Searched issues and PRs, and scanned every open PR's diff for added
operator ==,Object.hashor.select(undermobile/. All three classesstill lack
==onb1f6b7ef7.Testing
dart formatandflutter analyze libclean.b1f6b7ef7;the 11 added tests are the delta.
community does not replace the socket while a genuine relay-URL change still
reconnects.
No UI change.