Skip to content

perf(mobile): restore cached channels before community sync - #8580

Merged
wesbillman merged 6 commits into
mainfrom
star-lord/mobile-loading-20261024
Oct 26, 2026
Merged

wesbillman merged 6 commits into
mainfrom
star-lord/mobile-loading-20261024

Conversation

@wesbillman

@wesbillman wesbillman commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Returning to a previously visited community shows its saved channel list immediately, including offline, while membership and unread state refresh in the background. Initial revalidation failures keep the list visible and retry every 60 seconds without overlapping background attempts. After successful history revalidation, ordinary reconnects use metadata-only refreshes. A reconnect arriving during a background request queues one follow-up attempt; timer ticks never queue overlapping work. History deadlines retain the full-revalidation retry until history succeeds.

Snapshots are disposable, versioned and scoped by canonical relay URL and signing identity, capped at 2 MB / 2,000 member channels. They include hidden-DM IDs and prefer fresher in-memory state. Community removal and identity replacement retire outstanding writes; re-adding unchanged credentials preserves the current cache and writer. Startup sweeps orphan snapshots without waiting for disk acknowledgement; community mutations also sweep orphan snapshots, including data restored without secure community records. Cache-delete failures do not block sign-out or strand push revocation; deletion is attempted before journaling and retried by orphan cleanup. Successfully persisted unchanged snapshots skip disk writes; failed writes remain retryable.

The cold path reuses fetched membership rosters, overlaps profile/latest-message requests, shares verified DM profiles with avatars, and fetches the own profile once per authenticated connection. Community connection work overlaps the switching animation; avatar hydration no longer gates landing. Full channel refreshes also revalidate cached DM participant profiles so relay-side name changes reach labels and avatars; metadata-only reconnects and background refreshes reuse admitted profiles.

Me-chat and invite starter recovery use an explicit fresh membership fetch that propagates errors while leaving the cached list visible. Cached layout/read preferences permit immediate landing; newer cross-device preferences may update ordering or badges after reveal.

Integrated with main’s #8490 idle/resume changes: hidden chats detach live subscriptions, background membership polling stays quiet, and each resume reconciles once. Chat history follows main’s live-admission-first ordering to avoid missing messages committed during relay admission; cached community landing remains immediate.

Validation

  • just mobile-install mobile-check mobile-test: clean analysis; 3,778 tests passed, 19 skipped; four checks for builds without push configuration passed. File-size and diff checks passed.
  • Provider/storage journeys cover offline startup, failed initial revalidation recovering live delivery, bounded retries, lightweight reconnects, scope isolation, default-port URLs, padded/mismatched stored credentials, credential replacement, backup-restored orphan data, removal races, failed platform deletes and failed writes.
  • Follow-up regressions cover interrupted native Me-chat lookup, invite failure propagation, retry after an initial list failure, history deadlines, queued reconnects, escaped-path keys, unchanged credential re-adds and non-blocking startup cleanup. Reverting each of the eight behavior/caller/cleanup-order fixes fails its corresponding regression.
  • DM-name regression changes only the relay profile, verifies a full refresh updates the channel label and shared profile cache, and confirms ordinary reconnects avoid another profile request. The original cache-only implementation fails this regression.
  • Original signed-in iPhone 18 Pro debug simulator measurements: Buzz's 413 channels went from 6.26–6.30 s to approximately 1.5 s on returning; all three warmed communities switched in 1.47–1.54 s. Real socket pause restored the cached list offline and retained it on reconnect. These are debug simulator observations, not release-device benchmarks.
  • Integration with current main also passes the real-session resume/background/AUTH and live-admission/detach journeys.
  • Follow-up failure paths were exercised with deterministic provider/storage tests and widget journeys; the running simulator was not modified during this follow-up.

Human acceptance remains pending; this PR stays draft. Try switching among previously visited communities, opening a channel, restarting and repeating. First visits and oversized snapshots require network loading. Cached metadata can be stale until refresh; it lives in ordinary preferences without additional application-level encryption. The snapshot contains no message bodies or credentials. Persistent disk-deletion failures can leave metadata until a later cleanup succeeds.

Persist bounded channel snapshots per relay and identity, render them offline,
and refresh without blocking community landing. Reuse membership rosters and
verified DM profiles, overlap independent reads, and fetch the own profile once
per authenticated connection. Retire cache writes on community removal.

Co-authored-by: Rocket <0c41329ade659e01801116549bc617351689892a382bc864a1186dc9191293a7@buzz.block.builderlab.xyz>
Co-authored-by: Groot <f4c08302591916fb4b80926bf7e18d22eef8aa0c877e4e841acc46405a45700b@buzz.block.builderlab.xyz>
Co-authored-by: Mantis <530af7635ef7d0e1a5189d234338db88cb4ef1ede7119b4ebb4fb05c3eafe3b1@buzz.block.builderlab.xyz>
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
@wesbillman
wesbillman marked this pull request as ready for review October 25, 2026 15:30
@wesbillman
wesbillman requested a review from a team as a code owner October 25, 2026 15:30
@wesbillman
wesbillman deployed to codex-review October 25, 2026 15:30 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 25, 2026
@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: d090ac24bb7c78eb8a9db68f422ffaf7f8c54bfc...6f5cbd2dee2836f1808f4cd8db3e7f261118579f
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability regressions were identified in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source, caller, test, and history inspection. Builds and tests were not executed.

Generated by Codex Security Review |
Requested by: @wesbillman |
Workflow run

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Reviewed head 8cdbc5390e0d7c74fb70c5fdf827f9b29f671039 against main 9e8c9fc04daf20259cd259dcaec89109587cc7f5, with independent lanes covering persistence/privacy, state/concurrency and profile cache/tests.

Verdict: two blockers before ready. They're posted as comments because this is the author's own account.

  1. A restored list gets no live sync or retry if its first background refresh fails. Confirmed with a production-provider probe.
  2. Removal and sign-out can leave a snapshot (private channel names, DM participants) on disk, which contradicts the PR's deletion contract.

There's also one should-fix: a cache delete failure blocks sign-out and can strand a push revocation. Plus two non-blocking perf notes. Details are in the inline threads.

Holds up: removal-token retirement beats an in-flight save, since there's no await between the epoch check and the setString cache update. read() discards corrupt, oversized or wrong-version data. Hidden DMs are stored as IDs only. The DM resolver goes through the existing parser/merge with generation checks. The new request-path tests bind production seams. Channel.toJson round-trips every fromJson field.

Validation gaps: mostly a source review. The only runs were one focused provider probe (finding 1) and a Dart Uri probe (finding 2). The runtime and device behaviour in the PR description was not re-run.

Exit criteria: fix 1 and 2, then add a step to the existing snapshot journey for each: a failed first refresh still recovers live sync, and removal of a wss://…:443 community (or one whose credentials were swapped) leaves no buzz.channel-list.v1:* key behind.

Future<void>.microtask(() async {
if (waitingForInitialConnection) await connected.future;
if (!ref.mounted || buildGeneration != _buildGeneration) return;
await refresh();

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Blocking: a failed first refresh leaves a restored list with no live sync and no retry.

The snapshot path schedules exactly one refresh(). Live subscriptions (_subscribeLive, :487-491) and the 60 s backstop timer (channels_provider_lifecycle.dart:168-172) only start after a successful _fetchChannels. If that first refresh fails while the socket stays connected, the catch at :982-995 keeps the snapshot and nothing else ever runs. The reconnect listener only fires on a status transition. New messages, unread state and membership changes then stay stale until a manual pull-to-refresh, a foreground event or a reconnect. Before this PR, the same failure surfaced as an error with a retry affordance.

Probe at this head: seed a snapshot, connected relay fails the membership query once, then the relay recovers and fake time advances 2 minutes. Result: 1 membership request, 0 live listeners. A manual refresh() then succeeds and installs live sync.

Fix: start bounded background revalidation independently of subscription success. For example, arm the backstop timer (or a backoff retry) when the snapshot is installed, not only from _subscribeLive. Then add that failure step to the existing snapshot journey.

if (snapshots == null) return;
final pubkey = community.pubkey ?? _pubkeyFromNsec(community.nsec);
if (pubkey == null) return;
await snapshots.remove(pubkey, community.relayUrl);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Blocking: removal can miss the snapshot, leaving private channel names on disk after sign-out.

The writer keys on RelayConfig.baseUrl, which goes through Uri.replace(scheme:) and drops default ports. Removal keys on the raw community.relayUrl through a regex scheme swap in ChannelListSnapshotStorage.key. Dart probe:

wss://relay.example.com:443 -> writer key https://relay.example.com
                              removal key https://relay.example.com:443   (no match)

remove then deletes nothing and reports success. The identity side also derives differently: removal prefers community.pubkey, and the new _pubkeyFromNsec here duplicates pubkeyFromNsec in relay_provider.dart without its trim.

The same orphaning happens without any key mismatch:

  • addCommunity with an existing relay URL swaps pubkey/nsec in place (:268-276). The previous identity's snapshot is never deleted, and later removal only targets the new identity.
  • FlutterSharedPreferences.xml is included in Android cloud backup and device transfer; data_extraction_rules.xml only excludes secure storage. A restored device gets every snapshot but none of the communities that could remove them.

Fix: derive both keys through one canonicalizer (RelayConfig(baseUrl: relayUrl).baseUrl and the existing pubkeyFromNsec). Add a sweep of buzz.channel-list.v1:* keys that match no stored community at startup and after removal. The sweep also covers the credential-swap and restore cases. Optionally exclude snapshots from backup.

// Delete cached channel names before credentials, so a failure here
// leaves the community removable rather than orphaning the cache.
// Removal also retires every write token captured before it.
await _removeChannelListSnapshot(current[removedIndex]);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Should fix: a disposable cache now gates sign-out, and a failure here strands a push revocation.

remove() throws when prefs.remove returns false, which aborts _removeCommunity. Two consequences:

  1. Credentials stay on the device and sign-out fails, because a cache couldn't be deleted.
  2. This runs after communityPushLeaseRevocationEnqueuer has persisted a revocation record. The community survives, but the next outbox start()/trigger() still revokes its push lease, so push silently stops for a community the user kept.

Fix: make the snapshot delete best-effort (retire the token, log the failure, and let the orphan sweep from the other thread retry it) and run it before the revocation enqueue. Every prefix of the removal then stays consistent.

}
}
}
saveSnapshot?.call(channels, _hiddenDmIds);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Non-blocking (perf): this runs on every fetch, including the 60 s backstop, whether or not anything changed. Each write runs jsonEncode plus two utf8.encode passes on the UI isolate over up to 2 MB. Legacy SharedPreferences.setString has no unchanged-value short-circuit, so the platform store is rewritten every minute. Cheap fix: skip the write when the encoded string equals _prefs.getString(key).

Related: every scope's raw snapshot string is also loaded into memory by SharedPreferences.getInstance() in main.dart before the first frame. It isn't parsed, but the number of scopes is unbounded. That's fine at today's sizes; per-scope files would be worth considering if the cap stays at 2 MB.

// Ordinary refreshes must not retry an unchanged terminal rejection.
_terminallyClosedLiveChunks.clear();
unawaited(_backstopRefresh());
unawaited(refresh());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Non-blocking (question): reconnect used to call _backstopRefresh() (metadata only, keeping known lastMessageAt). It now calls refresh(), which also batches latest-message reads for every member channel (up to 20 events each). That's more relay load on every mobile resume or reconnect, and the PR description doesn't mention it. Is it intended? If it's only needed to start live sync for a restored snapshot, gating it on that case would keep reconnects light.

Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 25, 2026
@wesbillman
wesbillman deployed to codex-review October 25, 2026 15:51 — with GitHub Actions Active
@wesbillman
wesbillman marked this pull request as draft October 25, 2026 15:51
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 25, 2026

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Re-review at c323dd2594cb0f848b54f09ddc1e98c1405f8ae5 (base main 9e8c9fc0). This pass covered the prior findings and anything the fix commit introduced, again with independent lanes for state and storage.

Verdict: no blockers remain.

  • Failed first refresh: resolved. The original probe now passes: recovery runs automatically and installs live listeners. Focused provider probes also cover old/new build overlap on the in-flight flag, retry once per minute on repeated failure, live delivery resuming, metadata-only reconnects after revalidation, and zero timers or listeners after dispose.
  • Snapshot left on disk: resolved. A Dart probe of 11 relay URL shapes (:443, trailing slashes, case, .., ws:// with a port, and more) gave identical writer and sweeper keys. Deriving the pubkey from the nsec first now matches myPubkeyProvider. The sweep only touches keys absent from every stored community, and it can't race a writer.
  • Sign-out gating and push stranding: resolved. Snapshot deletion is now best-effort and runs before the revocation journal entry.
  • Unchanged writes: resolved. They now skip the platform store, though jsonEncode and utf8.encode still run every 60 s.
  • Reconnect cost: resolved. Reconnects are metadata-only again once revalidated.

There are three optional, cache-loss-only nits inline.

Validation gaps: focused provider tests and Dart probes only. No device or live-relay run, and the full mobile suite wasn't re-run here; CI covers it. Human acceptance is still pending per the PR description.

/// like `RelayConfig.baseUrl`, so both onboarding paths share a snapshot.
static String key(String pubkey, String relayUrl) {
final relay = RelayConfig(
baseUrl: normalizeCommunityRelayUrl(relayUrl),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Optional (cache loss only): key derivation isn't idempotent for percent-escaped wss paths. normalizeCommunityRelayUrl lowercases the escape, then Uri uppercases it again, and the writer's input has already been through RelayConfig.baseUrl once. Probe: wss://h/a%2Fb gives writer key https://h/a%2fb but sweeper key https://h/a%2Fb. Every sweep then deletes the active community's snapshot and retires its writer. That's rare, but lowercasing once more after canonicalizing makes key() idempotent.

pubkey: community.pubkey,
nsec: community.nsec,
);
await _removeChannelListSnapshot(existing);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Optional (cache loss only): this deletes the snapshot on every re-add, including when the nsec hasn't changed. For the active community the relay config doesn't change, so nothing rebuilds, and the retired writer token means no snapshot is saved for the rest of the session. Consider removing only when _channelSnapshotPubkey changes; the orphan sweep already covers an identity change.

Future<List<Community>> build() async {
final storage = ref.read(communityStorageProvider);
final communities = await storage.loadAll();
await _removeOrphanChannelListSnapshots(communities);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.

Optional: build waits for each orphan delete in turn before returning the community list. That's the startup path after a backup restore, and it retries on every rebuild if deletes keep failing. Running the sweep with unawaited(...) keeps startup independent of cache cleanup.

@wesbillman
wesbillman marked this pull request as ready for review October 25, 2026 16:15

@loganj loganj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Larry and Rbot: an independent adversarial review of c323dd2. Source review only: we read every changed production file and the relevant tests, but did not run the suite or the simulator.

Approving. The snapshot scoping, token retirement on remove, orphan sweep and the initial-revalidation retry loop all look deliberate and well covered. One should-fix and three smaller notes are inline:

  1. Error (should fix): DM participant names no longer refresh while you stay in a community, and the stale name is then persisted into the snapshot.
  2. Advice: a reconnect that lands while a background refresh is in flight is dropped, not queued.
  3. Question: a restored snapshot reveals before sections/stars/sort/mutes/read state finish relay sync.
  4. Preference (simplicity): the lastMessageAt carry-forward now exists twice, and one disconnected-build branch is effectively dead.

final profiles = await _fenced(
fence,
session.fetchHistory(NostrFilters.profilesBatch(dmParticipants.toList())),
ref.read(userCacheProvider.notifier).resolve(dmParticipants.toList()),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Error (should fix). resolve only requests pubkeys that are not already in userCacheProvider (user_cache_provider.dart:62-64), and that cache has no expiry within a relay scope. Before this PR, _resolveDmDisplayNames fetched the profiles on every list fetch, so a manual refresh, resume, reconnect or the 60 s backstop picked up a renamed DM participant. Now the first name sticks for the rest of the session unless something else happens to call cacheProfileEvent (only the channel detail page and huddle sheet do). Because _fetchChannels writes the snapshot on every fetch, the stale label is also persisted and comes back after a restart.

Keeping the shared cache and the parallel reads makes sense. One option: on the full revalidation path (fetchLastMessage == true), request every DM participant and let _verifyAndMerge replace older entries, and keep the cache-only lookup for the metadata-only backstop. The request-path test at channels_provider_request_path_cases.dart:42-63 calls cacheProfileEvent itself, so it does not cover this. Changing the fake relay's profile after the first load and refreshing without that call would.

}

Future<void> _refreshInBackground() async {
if (_backgroundRefreshInFlight ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Advice. The reconnect listener clears _terminallyClosedLiveChunks so that the next refresh retries those chunks under the new connection's admission policy. With this guard, if a timer-started refresh is still in flight when the reconnect fires, the reconnect refresh returns immediately. The retry then waits for the next 60 s tick. Before this PR the reconnect always started _backstopRefresh, and the fence let the newer one win. A small _backgroundRefreshQueued flag that reruns once in the finally would keep the no-overlap property without dropping the trigger.

!profile.isLoading &&
(!hasIdentity || (sections && stars && sort && mutes && readState)),
(!hasIdentity ||
cached ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Question. With cached, landing no longer waits for sections, stars, sort, mutes or read state. Those managers show local prefs first, but isReady waits for their relay sync. If a section was changed or channels were read on another device since the last visit, the list can reorder, or unread badges can appear and then clear, shortly after the reveal. Is that the intended tradeoff, and did it show up in your simulator runs? If the jump is noticeable, waiting only for channelSectionsProvider and channelSortProvider (layout) and not read state might be a middle ground.

// Guard the provider-state write in `retryDirectory` and `build`: the
// caller assigns whatever this returns, so the last check belongs here.
fence.ensureCurrent();
if (!fetchLastMessage) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Preference (simplicity). This block now carries lastMessageAt forward for metadata-only fetches. _backstopRefresh (line 944) still has its own prevLastMessage loop that does the same thing for null values, so that loop is now redundant and could be deleted.


if (sessionState.status != SessionStatus.connected) {
// Preserve cached channels only for a reconnect within the same scope.
if (_hasLoaded) return state.value ?? const [];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Preference (simplicity). With the snapshot path above, this branch is reachable only when _hasLoaded is true, state.value is null and no disk snapshot exists. In that case the listener change above means nothing refreshes on connect: connected completes, but nothing awaits it, and this path never starts the backstop timer. Earlier, the !_hasLoaded guard sent that connect to _backstopRefresh. It is an edge case, so deleting the branch (falling through to await connected.future) seems simplest.

@wesbillman
wesbillman marked this pull request as draft October 26, 2026 15:44
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 26, 2026
@wesbillman
wesbillman marked this pull request as ready for review October 26, 2026 15:55
@wesbillman
wesbillman deployed to codex-review October 26, 2026 15:55 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 26, 2026
#8589)

Opening a conversation currently waits for its live subscription to
reach EOSE (or a 500 ms fallback) before requesting history. This
registers live delivery first, then loads history while subscription
readiness settles. Existing history/live merging handles overlap;
version checks ignore retired callbacks and close subscriptions whose
readiness completes after replacement.

Stacked on #8580 so the performance comparison includes channel-list
caching. This PR changes one production file and extends the existing
real-session message journey.

Validation: `just mobile-install mobile-check mobile-test` passed (3,765
tests, 19 skipped; four additional unconfigured-push tests), along with
the repository file-size gate. The extended journey fails on the
original serial implementation and covers delayed EOSE, live/history
races, replay deduplication, sends, reconnect and late readiness after
replacement.

Before/after: five forced-uncached opens per variant on the same
signed-in iPhone 18 Pro / iOS 27.0 debug simulator. Median
tap-to-first-loaded-timeline frame improved from **322 ms to 228 ms**
(29%); median history-request start moved from **125 ms to 7 ms**.
Temporary profiling/reset hooks were identical between variants and
removed before final checks. These small-sample simulator measurements
use an uncontrolled network and do not establish release-device
performance.

Draft pending independent review and human acceptance. To try: open
streams, return/reopen, and background/resume; messages should load
without the serialized readiness wait and remain ordered without
duplicates.

Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
Co-authored-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 26, 2026

@kalvinnchau kalvinnchau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review findings below. The interrupted Me-chat recovery regression should be fixed before approval.

await _refreshInBackground();
}),
);
return snapshot;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [P2] Await fresh membership during interrupted Me-chat recovery

native_new_me_chat.dart:166–168 invalidates this provider and awaits its future to find a previously created channel. Returning cached state before background revalidation breaks that contract: if creation reached the relay but not the cache, recovery treats the channel as absent and re-signs creation, which can produce duplicate: channel already exists.

A provider-level regression test loads [alpha], adds me-chat remotely, then invalidates and awaits: this commit returns [alpha], whereas base 9e8c9fc returns [alpha, me-chat]. The downstream duplicate outcome is traced through the recovery and relay code, not reproduced end to end. Make the recovery lookup await an explicit fresh fetch that propagates failure; cached rendering can remain immediate.

debugPrint(
'[ChannelsNotifier] refresh failed; retaining cached list: $error',
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [P3] Propagate explicit refresh failures while retaining cached UI state

When membership fetching fails with cached data present, refresh() no longer exposes the failure through channelsProvider.future. Invite starter recovery (app.dart:250–254) relies on that failure propagating. It can instead continue with stale membership, attempt duplicate creation, or skip joining when stale isMember=true survives.

This consequence is established by caller tracing, not a runtime reproduction. Retain the cached list for rendering, but return or throw the fetch failure to explicit refresh callers.

}
}
}
if (fetchLastMessage) _needsSnapshotRevalidation = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [P3] Keep snapshot revalidation pending when the history batch times out

_fetchChannelHistoryBatch can return null at its deadline (:419–425). This path carries the snapshot timestamps forward but still clears _needsSnapshotRevalidation. Later reconnects and backstop refreshes are then metadata-only, leaving recent-sort and Me ordering stale until resume or a manual full refresh.

Only clear the revalidation flag after the latest-message batch succeeds (latestEvents != null). This is a static finding; the timeout scenario was not runtime-reproduced.

// Guard the provider-state write in `retryDirectory` and `build`: the
// caller assigns whatever this returns, so the last check belongs here.
fence.ensureCurrent();
if (!fetchLastMessage) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 [P3] Remove the redundant timestamp carry-forward loop

This carry-forward duplicates the backstop loop at lines 949–960. The backstop pass is now effectively redundant, leaving two implementations of the same merge policy that can drift when timestamp handling changes. Remove the backstop-specific loop and keep the rule in _fetchChannels.

@wesbillman
wesbillman marked this pull request as draft October 26, 2026 17:04
Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
@wesbillman
wesbillman marked this pull request as draft October 26, 2026 17:30
Preserve main’s live-admission-first chat history, idle detach and foreground-only polling alongside cached community recovery. Avoid duplicate membership refreshes when a reconnect finishes before the deferred resume callback.

Signed-off-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>
@wesbillman
wesbillman marked this pull request as ready for review October 26, 2026 17:47
@wesbillman
wesbillman deployed to codex-review October 26, 2026 17:47 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Oct 26, 2026
@wesbillman
wesbillman merged commit ab73cd9 into main Oct 26, 2026
54 checks passed
@wesbillman
wesbillman deleted the star-lord/mobile-loading-20261024 branch October 26, 2026 17:57
wesbillman added a commit that referenced this pull request Oct 26, 2026
## Summary

Opening an uncached community seeded each previously unseen channel's
read marker separately. Every seed copied and serialized the full
read-state snapshot, started three preference writes, and notified
listeners. Home now collects those seeds and saves/emits once, using the
existing monotonic marker admission rules.

For a 400-channel fixture with one existing marker, initial seeding
falls from 399 full snapshot saves to one. Existing unread frontiers,
local-only seeds and explicit read publishing keep their behavior. No
transport, cache, relay or unread eligibility changes. This is
independent of #8580 and #8490.

## Validation

- `just mobile-install mobile-check mobile-test`: clean analysis; 3,779
tests passed, 19 skipped, plus four checks for builds without push
configuration.
- Real Home/channel-provider/read-state journey covers 400 channels,
preservation of an existing unread highlight, persisted marks, no seed
publication, and no repeated seed save on refresh. It replaces the
earlier fake-notifier seed test.
- Restoring the old per-channel loop makes the new journey fail at the
single-snapshot assertion.
- Diff and file-size checks pass.
- Merged current main (`209fcd217`) at `d34d29627`; retained both the
cold read-seeding and cached-community landing test registrations. The
production diff is byte-for-byte unchanged from the version reviewed by
the Court at `6a232ed37`. The complete mobile gates above passed again
on the merged tree.

Device latency is unmeasured. The Court source review found no blockers
at `6a232ed37`; app/device testing and human acceptance remain
unconfirmed. To try: open a community without a saved channel list,
confirm its channels load and existing unread highlights remain correct,
then refresh and reopen it. The performance claim here is reduced save
work, not a measured end-to-end speedup.

Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Co-authored-by: Star Lord <b89298dbe87c6b3fd8a425b535d9d161f23c88d555a47925ae0b04dfe02b201e@buzz.block.builderlab.xyz>

This branch was successfully deployed

1 active deployment
codex-review — 6f5cbd2d Deployed Oct 26, 2026 by wesbillman via Run Codex Security Review #9863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants