fix(mobile): preserve channel group edits and sync sorting

[tellaho]

Category: fix

User Impact: Mobile channel-group changes now survive reconnects, and channel sorting stays consistent between mobile and desktop.

Problem: Mobile could discard an unpublished channel-group edit when relay status changed during the publish delay, replacing the newer local state with an older relay copy. Mobile also lacked the per-group sorting preferences already available and synced on desktop.

Solution: Preserve a durable record of unpublished mobile edits, reconcile them after reconnect, and only seed the relay after confirming no remote state exists. Add desktop-compatible encrypted sort preference sync and mobile controls for sorting Starred, custom groups, Channels, and DMs by recent activity or A–Z.

File changes

mobile/lib/features/channels/channel_sections/channel_sections_manager.dart
Tracks unpublished section edits across manager rebuilds, protects newer local state from stale relay data, and republishes safely after reconnect.

mobile/lib/features/channels/channel_sections/channel_sections_storage.dart
Persists whether channel-section changes are still waiting to reach the relay so they survive reconnects and app restarts.

mobile/lib/features/channels/channel_sort/channel_sort_manager.dart
Adds encrypted relay synchronization for mobile channel-sort preferences using the same contract as desktop, including reconnect protection.

mobile/lib/features/channels/channel_sort/channel_sort_provider.dart
Exposes synchronized sort preferences to the mobile channel list and scopes them to the active account and community lifecycle.

mobile/lib/features/channels/channel_sort/channel_sort_storage.dart
Defines the shared sort payload, local cache, group-key cleanup, and A–Z/recent channel ordering behavior.

mobile/lib/features/channels/channels_page.dart
Connects the channel page to the new sort preference feature.

mobile/lib/features/channels/channels_page/body.dart
Applies each group's selected order and routes sort changes from section controls into synchronized preferences.

mobile/lib/features/channels/channels_page/sections.dart
Adds checked Recent and A–Z options to built-in and custom channel-group menus using existing mobile UI components.

mobile/test/features/channels/channel_sections/channel_sections_manager_test.dart
Covers the edit-loss regression, reconnect publishing, failed-fetch safety, local dirty-state persistence, and clean remote adoption.

mobile/test/features/channels/channel_sort/channel_sort_manager_test.dart
Verifies desktop-compatible sort reads and writes, reconnect protection, and cleanup of deleted custom-group preferences.

mobile/test/features/channels/channel_sort/channel_sort_storage_test.dart
Covers payload validation, local persistence, orphan cleanup, and both channel ordering modes.

Reproduction steps

1. Run the mobile app while connected to a relay and create or modify a custom channel group.
2. Interrupt or cycle the relay connection before the delayed publish completes.
3. Reconnect and confirm the local group change remains visible and is published rather than replaced by older relay state.
4. Open the menu for Starred, a custom group, Channels, or DMs.
5. Select Sort: Recent and confirm active channels move first; select Sort: A–Z and confirm alphabetical ordering returns.
6. Open desktop with the same account and confirm the selected sort preference is shared across clients.

Screenshots / demos

No screenshot is included. The visual change uses the existing mobile popup-menu components inside section headers, and the repository does not currently provide a Flutter screenshot or golden-test harness for this flow.

[wpfleger96]

Reviewed at head 83271da8d. Combined and deduplicated feedback from three independent reviewers (Paul, Thufir, Duncan) — each formed findings separately before seeing the others, and I reconciled them.

One CRITICAL, flagged independently by all three of us: the dirty-state protection in _mergeEvent stops mobile from losing edits by making mobile unconditionally win — which converts the bug into cross-device data loss in the other direction. A legitimate newer desktop edit received while mobile is dirty is dropped and marked as seen, then mobile's publish deliberately timestamps itself above it and erases it on every client. Because channel-sections is a whole-blob record, one lost event means every group the other client created. Details inline at channel_sections_manager.dart:311.

That finding is why this is REQUEST_CHANGES rather than comments. 3-of-3 independent convergence on a silent data-loss path is the strongest signal a review can produce.

Five IMPORTANT findings inline: a cross-instance dirty-flag race, a permanent publish wedge from a future-dated remote timestamp, two desktop-compatibility divergences (storage scoping and A–Z collation) that undercut the PR's stated cross-client goal, and a seed-publish branch that goes unreachable on retries. Plus the duplication issue below.

Structural note that shapes the fix order. channel_sort_manager.dart is a near-verbatim copy of channel_sections_manager.dart — I measured it: of 279 non-comment lines, 224 are byte-identical after renaming the type. _syncWithRelay, _scheduleStartupRetry, markDirty, _clearDirty, _publish, _mergeEvent, _mergeEvents, _handleIncomingEvent, dispose, _fetchAndMerge, _startLiveSubscription, and both Crypto classes are clones; only the d-tag, store type, and setSortModeFor differ. The consequence is concrete rather than aesthetic: every finding below exists in two files and has to be fixed twice, and a future fix to one will silently miss the other. We'd suggest extracting the shared machinery into one generic encrypted-NIP-78-blob sync manager parameterized on d-tag, store codec, equality, and conflict policy, then fixing the CRITICAL once. ChannelSectionsCrypto and ChannelSortCrypto are line-for-line identical and collapse into one type outright.

What's genuinely well done, and we checked these rather than assuming:

• The Future<bool?> tri-state cleanly separates "fetch failed" (null) from "relay confirmed no blob" (false), so seed-publish can't fire after an ambiguous failure. All three of us went looking for a hole here; there isn't one. A rate-limited REQ produces a relay CLOSED (connection.rs:655-679 routes every admission rejection through request_rejection_message, emitting CLOSED whenever a sub_id is present — and a REQ always has one), which surfaces as a thrown error, not an empty result. That's the right design and the comment explaining it is accurate.
• The sort path is applied consistently to Starred, custom sections, Channels, and DMs, and DMs correctly keep display-label ordering unless Recent is chosen — the label-vs-name distinction is a subtle thing to get right.
• stripOrphanedSectionModes bounding the stored map is the right instinct, and it mirrors desktop's implementation closely.

CI is green (all 25 checks). None of the findings below are CI-detectable, which is worth noting given the PR is still in draft.

[wpfleger96]

CRITICAL — dirty-state protection silently and permanently discards legitimate remote edits, then overwrites them. Flagged independently by all three reviewers.

The ordering is the defect. When isNewer is true, this advances _lastRemoteCreatedAt and _lastRemoteEventId to the incoming event, and then line 319 bails with if (_dirtySince > 0) return; — dropping incoming entirely. Walk a real sequence:

1. Mobile is dirty — one unpublished edit, say a renamed group.
2. On desktop the user creates a new group. Desktop publishes at T+10.
3. Mobile's _publish calls _fetchAndMerge (line 363), pulls the desktop event, sets _lastRemoteCreatedAt = T+10 and _lastRemoteEventId = <desktop id>, then returns here without storing it.
4. Mobile publishes with createdAt = max(currentUnixSeconds(), _lastRemoteCreatedAt + 1) (line 378) — deliberately sorting above desktop's event — carrying a blob that never contained desktop's new group.
5. Desktop adopts it by whole-blob LWW. The desktop-created group is now gone on both clients.

It is also unrecoverable, not merely deferred. Because _lastRemoteEventId was advanced in step 3, a later redelivery of that same desktop event fails the isNewer test forever — even after the dirty flag clears.

The comment at 313-318 says "the pending publish then reconciles the relay." It doesn't reconcile; it overwrites. That's the crux: a local dirty veto on the read combined with an unconditional win on the write is guaranteed clobber, not conflict resolution. The PR's goal is preventing edit loss on mobile, and this achieves it by relocating the identical bug onto desktop. Whole-blob LWW plus a one-sided veto can't be made correct.

Severity is amplified by the blob shape: channel-sections is one whole-blob record, so "one dropped event" means every section and assignment the other client created in it.

Fix (preferred) — bail before touching the cursor, and stash the dropped state for reconciliation:

if (isNewer) {
  if (_dirtySince > 0) {
    _deferredRemote = incoming;   // reconcile after the pending publish lands
    return;                       // cursor NOT advanced
  }
  _lastRemoteCreatedAt = event.createdAt;
  _lastRemoteEventId = event.id;
  _store = incoming;
  _persist();
}

Then field-merge _deferredRemote into _store before publishing, so both edits survive.

Fix (minimum, if a real merge is out of scope for this PR) — move the bail above the two assignments so the event stays re-adoptable once clean, and don't let the publish createdAt leapfrog an unmerged remote event. That leaves a window where the two clients disagree, but nothing is destroyed.

Note this applies identically at channel_sort_manager.dart:286-290, which is the same code — see the duplication note in the review summary.

[wpfleger96]

IMPORTANT — the dirty flag is persisted per-pubkey but _editGeneration is in-memory per-instance, so one manager can clear another's dirty state and strand an unpublished edit.

_clearDirty guards with _editGeneration != generationAtStart, which correctly catches an edit landing during an in-flight publish within one instance. It cannot see edits made on a different instance — and instances are routinely concurrent here, because channel_sections_provider.dart:73-79 registers ref.onDispose(() => manager.dispose()) with the default flushPending: true, which fires unawaited(_publish(allowDisposed: true)) (line 142) on a manager that is being torn down while its replacement is already being constructed.

The race:

1. Manager A is dirty with a publish in flight (generationAtStart captured, _editGeneration == 3).
2. Provider rebuild — relay status flip — tears A down. A's flush keeps running.
3. Manager B is constructed, reads _dirtySince > 0 from prefs, and accepts a new local edit. B's own _editGeneration starts at 0 and is unrelated to A's.
4. A's publish succeeds and calls _clearDirty(3). A's counter still equals 3, so the guard passes and A writes dirty = 0 for the pubkey.
5. B's edit is now unpublished but no longer marked dirty. It won't survive a restart, and — worse — the next remote blob will overwrite it, since the dirty veto at line 319 is what was protecting it.

The persisted flag is shared state; the guard protecting it is not. Fix: make the persisted value the thing you compare against — store a monotonic edit token alongside dirty-since and have _clearDirty re-read it, clearing only when it still equals the value observed at publish start (compare-and-clear). A cheaper variant: have _clearDirty re-read _storage.readDirtySince(pubkey) and skip clearing when it differs from the _dirtySince this instance captured before publishing.

Also note the flush is only attempted when hadPending is true (line 137) — a manager that is dirty with no live debounce timer is torn down with no flush at all, relying entirely on the persisted flag that this race can clear.

Same code at channel_sort_manager.dart:361-366.

[wpfleger96]

IMPORTANT — one future-dated remote event permanently wedges publishing for this d-tag.

(Anchored here; the line in question is final createdAt = max(currentUnixSeconds(), _lastRemoteCreatedAt + 1); just below, which this PR does not itself modify but now makes durable-state-dependent.)

createdAt = max(currentUnixSeconds(), _lastRemoteCreatedAt + 1) inherits whatever _lastRemoteCreatedAt was set to from a remote event, and _mergeEvent applies no plausibility check before advancing it. The relay rejects any event more than ±900s from server time (crates/buzz-relay/src/handlers/ingest.rs:1506-1513).

So a single blob published with created_at = now + 20min — a desktop with a skewed clock is sufficient, no malice required — puts _lastRemoteCreatedAt 20 minutes ahead. Every subsequent publish then computes a createdAt the relay refuses, throws, leaves the dirty flag set (line 397), and gets retried on the next initialize(). The state is permanent: nothing lowers _lastRemoteCreatedAt, so section changes stop syncing from this device indefinitely, and the only visible symptom is a debugPrint.

This codebase already solved this. read_state_manager.dart:526-527 has:

bool _isPlausibleCreatedAt(int createdAt) =>
    createdAt <= currentUnixSeconds() + readStateMaxClockDriftSeconds;

guarding its own cursor advance at :236 and :307, with readStateMaxClockDriftSeconds = 300 in read_state_time.dart:25 — comfortably inside the relay's 900s. These two new managers just don't call it.

Fix: reuse the existing helper — refuse to advance _lastRemoteCreatedAt for an event failing _isPlausibleCreatedAt, and clamp the publish createdAt to currentUnixSeconds() + readStateMaxClockDriftSeconds. read_state_manager.dart also has _isPermanentReadStateRemoteError (:529) to stop retrying rejections that will never succeed; worth mirroring, since neither of these managers distinguishes a transient failure from a permanent one.

Same code at channel_sort_manager.dart:338.

[wpfleger96]

IMPORTANT — mobile keys these stores by pubkey alone; desktop deliberately scopes them by relay. Same identity in two communities reads and can publish the wrong community's data.

Mobile: 'buzz.channel-sort.v1:$pubkey' (line 7) and 'buzz.channel-sort.dirty-since.v1:$pubkey' (line 9). Desktop does the opposite on purpose — desktop/src/features/sidebar/lib/channelSortPreference.ts:42-47 appends the normalized relay URL, with the comment "so preferences don't bleed across communities/relays."

channel_sort_provider.dart:38 watches activeCommunityProvider so the manager is rebuilt on a community switch, but the rebuild reads back through the same unscoped key — so the new community's manager loads the previous community's sort prefs as its local state. Combined with the seed-publish path in _syncWithRelay (channel_sort_manager.dart:119), if the new relay has no blob, those carried-over preferences get published to it as though the user had chosen them there.

The persisted dirty flag makes it sharper: it too is unscoped, so a dirty flag set in community A causes a publish in community B.

This also applies to channel_sections_storage.dart:5 and the new :7 dirty key — pre-existing for the store itself, newly extended to the dirty marker by this PR.

Fix: thread the active relay URL into the managers and include it in the mobile keys, matching desktop's normalization so the two clients agree on scope. Existing pubkey-only entries need a one-time read-through migration so users don't silently lose their current grouping on upgrade.

[wpfleger96]

IMPORTANT — A–Z ordering doesn't match desktop, so a preference whose entire purpose is cross-client consistency renders differently per client.

Mobile: left.name.toLowerCase().compareTo(right.name.toLowerCase()) — a UTF-16 code-unit comparison. Desktop: left.name.localeCompare(right.name) (desktop/src/features/sidebar/lib/channelSortPreference.ts:132).

These disagree on any non-ASCII name. localeCompare places Éclair next to Eclair; compareTo sorts it after Zulu, because É (U+00C9) is above z (U+007A) in code-unit order. Diacritics are common in channel names, and the divergence also hits case-mixed ASCII where toLowerCase() and locale collation disagree on tie ordering.

The PR sells this feature as "the selected sort preference is shared across clients." The preference syncs correctly; the rendering doesn't, which is the part a user can see — same account, same setting, two different lists.

Fix: use a locale-aware comparator on mobile so the two clients agree. Dart doesn't have localeCompare in the SDK, so either bring in an ICU-backed collator or normalize both sides to a shared, explicitly-documented rule — the important thing is that desktop and mobile share one rule. If matching localeCompare exactly isn't practical, changing desktop to the same normalized comparison is equally acceptable; consistency matters more than which rule wins.

[wpfleger96]

IMPORTANT — forcing foundRemote = true on retries makes the retry unable to see a blob that lands mid-stall, and renders the seed-publish branch unreachable.

Line 111 reads final foundRemote = _startupFetchSucceeded ? true : await _fetchAndMerge();, and line 112 sets _startupFetchSucceeded whenever foundRemote != null — including when the fetch succeeded and returned false. Two consequences:

1. The retry never re-fetches. Take the case this retry exists for: the history REQ gets a permit but the live REQ is rate-limited. _startupFetchSucceeded is true, a retry is scheduled (line 123). On that retry line 111 short-circuits, so no fetch is issued — and the live subscription being established now only delivers events from now forward. A remote blob published between pass 1 and the retry is therefore never seen, and stays invisible until the next provider rebuild. On mobile that can be a long time.
2. Seed-publish is dead on retries. foundRemote == false at line 119 can never be true once the sentinel is applied.

On (2) the current code is incidentally fine — pass 1 already evaluated the branch, and a sort change made between passes routes through markDirty() and is caught by the _dirtySince > 0 arm. But it's correct by accident, and it's load-bearing for a feature this PR introduces. The underlying problem in both is that _startupFetchSucceeded conflates "a fetch completed once" with "the fetch result is still current."

Fix: track fetch-success and subscription-success as genuinely independent flags, skip only the arm that already succeeded, and re-fetch once the subscription finally lands so nothing published during the stall is missed. Keep the seed decision as a value captured on the pass where the fetch actually returned a definite false, rather than recomputing it from a variable that's been overwritten with a sentinel.

[wpfleger96]

IMPORTANT — this class is a near-verbatim copy of ChannelSectionsManager, which means every finding in this review has to be fixed twice.

Measured at this head: of 279 non-comment, non-blank lines in this file, 224 are byte-identical to channel_sections_manager.dart after substituting the type name. _syncWithRelay, _scheduleStartupRetry, dispose, markDirty, _clearDirty, _publish, _fetchAndMerge, _startLiveSubscription, _mergeEvent, _mergeEvents, _handleIncomingEvent, and _isIdenticalToLastPublished are clones. ChannelSortCrypto (lines 14-29) is line-for-line ChannelSectionsCrypto. Only the d-tag string, the store type, and setSortModeFor genuinely differ.

The cost is not tidiness. The CRITICAL at channel_sections_manager.dart:311 exists here too at line 286-290; the dirty-flag race exists here at 361; the clock-drift wedge exists here at 338. Each needs two fixes, and any future correction to one file will silently skip the other — which is exactly how these two managers came to share three bugs in the first place.

Fix: extract the shared machinery into one generic encrypted-NIP-78-blob sync manager parameterized on d-tag, store codec, equality function, and conflict policy, and let both sections and sort subclass it. Fix the CRITICAL once, in the shared implementation. ChannelSectionsCrypto / ChannelSortCrypto collapse into a single SelfEncryptedBlobCrypto with no parameterization needed at all.

Desktop reached the same conclusion structurally — channelSortSync.ts's class doc says it "follow[s] the same pattern as channel sections." Mobile has the opportunity to share the implementation rather than the pattern.

[wpfleger96]

MINOR — clearing the dirty flag on the no-op-suppression path is safe today only by accident.

_isIdenticalToLastPublished() returns false when _lastPublishedStore == null (line 336), so neither a fresh manager nor one rebuilt from the persisted dirty flag can reach this line — the dirty flag can't be cleared without a publish having happened in this instance. That's the saving grace, and it holds.

But it's incidental rather than stated. If _lastPublishedStore ever gets seeded from persistence — a natural follow-up, since everything else here is now durable — this line starts clearing the persisted dirty flag for a store the relay never received.

One reviewer initially rated this CRITICAL on the grounds that _lastPublishedStore can drift from relay truth. It can, but only as a consequence of the CRITICAL at line 311 (a remote blob dropped while dirty), so it resolves when that does rather than needing an independent fix.

Fix: either add a comment making the _lastPublishedStore == null dependency explicit, or don't clear dirty in the no-op branch at all and let the next real publish clear it — a redundant publish of an unchanged blob is much cheaper than a lost edit.

Same code at channel_sort_manager.dart:331.

[wpfleger96]

MINOR — _isIdenticalToLastPublished can report equality for two different assignment maps.

(Anchored at the call site; the defect is in the method body at lines 348-351, unchanged by this PR but newly load-bearing now that this branch clears the persisted dirty flag.)

The loop iterates only _store.assignments.keys, so a key present in last but absent from _store is never examined — the lookup for a key not in _store is simply never performed. Equal lengths are checked at line 338, so the only hole is an equal-size key swap: one channel unassigned and a different one assigned within the same debounce window. assignChannel + unassignChannel on different channels between publishes does exactly that.

The consequence is a suppressed publish, so the relay keeps the old assignment map and the local change is silently not synced — and with the dirty flag cleared on that path (line 371), it isn't retried either.

Low likelihood, real consequence. Fix: check containment in both directions.

if (last.assignments.length != _store.assignments.length) return false;
for (final entry in _store.assignments.entries) {
  if (!last.assignments.containsKey(entry.key)) return false;
  if (last.assignments[entry.key] != entry.value) return false;
}

The positional comparison of sections above is fine by contrast — a reordered list compares unequal, which only costs a redundant publish.

[tellaho]

Closing as superseded after review exposed a silent cross-device data-loss path in the proposed dirty-state sync policy: a pending mobile edit can ignore a newer desktop blob and later overwrite it globally.

We are intentionally not trading the existing, recoverable edge case—mobile group edits can be lost if teardown lands inside the debounce window—for destructive concurrent-edit behavior. The user-visible cold-start issue that motivated this investigation is addressed independently by #3004, which retries sections sync after relay rate-limiting.

Future work, if prioritized, should define and test a safe cross-device conflict policy before revisiting mobile write persistence or synced sorting. Thank you to everyone who reproduced, reviewed, and pressure-tested this approach.