Skip to content

fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot - #5086

Merged
wpfleger96 merged 13 commits into
mainfrom
duncan/sidebar-revert-fix
Aug 6, 2026
Merged

fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot#5086
wpfleger96 merged 13 commits into
mainfrom
duncan/sidebar-revert-fix

Conversation

@wpfleger96

@wpfleger96 wpfleger96 commented Aug 6, 2026

Copy link
Copy Markdown
Member

Fixes the bug where running a dev build with stale localStorage would publish outdated channel sections, sort preferences, starred channels, and muted channels to the relay, clobbering the DMG installation's live state.

Root cause

All four sidebar-preference sync managers (channelSectionsSync, channelSortSync, channelStarsSync, channelMutesSync) collapsed five distinct fetch outcomes — no event, timeout, error, auth-race empty result, decrypt/parse failure — into a single null. Each hook's boot effect treated null as "no remote exists" and seed-published whatever was in localStorage, stamped at max(now, lastRemoteCreatedAt+1) with lastRemoteCreatedAt reset to 0 on every boot. A dev build with stale localStorage therefore re-signed old state as newer, and the DMG's live subscription applied it.

Two guards

1. Tri-state fetch result (found | absent | failed) — decrypt failure on an existing event reports failed and records event.created_at, so seed-publish is blocked even when the payload is unreadable.

2. Persisted head watermark (sidebarSyncWatermark.ts) — keyed {blobType, pubkey, normalizedRelayUrl}, written to localStorage on every observed remote event (before decrypt on all paths: initial fetch, live subscription, fetchOwnBlobBeforePublish), hydrated at construction. Any session that has ever seen a remote blob skips seed-publish on the next boot even when the fetch returns empty. Relay URLs are normalised via shared/lib/normalizeRelayUrl (also used by profile storage) so the same relay written two ways never produces two keys.

Bootstrap owns the seed. Each manager exposes bootstrap(localStore) that fetches, records the raw head, and delegates the decision to the single runBootstrap policy: hold on failed or absent + prior watermark, seed on genuine first-sync (absent + zero watermark + non-empty local), apply-remote when a blob was found. Hooks only act on apply-remote; they cannot publish during bootstrap. First-time sync is unchanged: successful EOSE with no event, zero watermark, and non-empty local state still seeds.

LWW baseline preservation

fetchOwnBlobBeforePublish for sections/sort snapshots the watermark before recordRemoteHead advances it, then compares the fetched event against the snapshot — advancing first would make remote.createdAt > lastRemoteCreatedAt always false and silently kill the whole-blob LWW merge. Stars/mutes merge per-entry via mergeStores, so no snapshot is needed there.

Relay lifecycle

All four hooks require a defined relayUrl (plumbed from communitiesHook.activeCommunity?.relayUrl in AppShell.tsx); while it is undefined no manager is constructed and no boot/live/reconnect effect binds. All effects depend on [pubkey, relayUrl], so community switches tear down and rebind. destroy() cancels pending publishes without flushing — flushing would race community switching and could publish relay A's state to relay B via the shared relayClient singleton. Pending debounce-window edits are intentionally dropped: stars/mutes entries survive via per-entry merge on the next publish; a dropped sections/sort edit is lost because bootstrap whole-blob-replaces from remote on return.

Known trade-off: a first boot with the relay unreachable holds (never seeds) until the user's next explicit edit — preferred over risking a stale seed-publish.

Files

  • sidebarSyncWatermark.ts — watermark persistence + runBootstrap policy (tri-state FetchResult, readWatermark, advanceWatermark)
  • shared/lib/normalizeRelayUrl.ts — relay-URL normalisation shared by watermark keys and profile storage
  • channelSectionsSync.ts, channelSortSync.ts, channelStarsSync.ts, channelMutesSync.ts — tri-state fetch, pre-decrypt recordRemoteHead on all paths, sections/sort watermark snapshot for LWW, bootstrap(), cancel-without-flush destroy()
  • useChannelSections.ts, useChannelSortPreference.ts, useChannelStars.ts, useChannelMutes.ts — act on bootstrap() results, gate on relayUrl, [pubkey, relayUrl] deps on all effects
  • AppShell.tsx — passes activeCommunity?.relayUrl to useChannelMutes and useChannelStars
  • sidebarSyncTestHelpers.mjs — shared fake-window/localStorage/Tauri mocks for the four manager suites
  • Test suites — mutation-sensitive coverage: failed→hold, absent+watermark→hold, first-sync seeds, undecryptable head recorded on all paths, relay-A/B watermark isolation, watermark restart round-trip, sections/sort LWW baseline

…age boot

All four sidebar-preference sync managers (sections, sort, stars, mutes)
collapsed five distinct fetch outcomes into a single null, causing the boot
effect to seed-publish stale localStorage on any failed or empty response.
A dev build with old localStorage but the same key + relay would re-sign
those stale prefs as fresh, and the DMG's live subscription applied them.

Two invariants now applied to all four managers:

1. Tri-state fetch result (found / absent / failed) — decrypt/parse
   failure on an existing event reports failed, not absent, and records
   the event's created_at so seed-publish is blocked even for unreadable
   blobs.

2. Persisted head watermark (sidebarSyncWatermark.ts, key scoped by
   pubkey + relay + blob type, stored in localStorage) — hydrated in
   the manager constructor so a session that has ever seen a remote blob
   refuses to seed-publish on subsequent boots, even when the fetch
   returns empty (auth-race, timeout, reconnect).

Seed logic unchanged for genuine first-time sync: absent fetch + zero
watermark + non-empty local state still seed-publishes.

Existing destroy() flush-on-destroy behaviour in channelStarsSync and
channelMutesSync aligned with channelSectionsSync/channelSortSync
(cancel + destroyed flag, no flush) to prevent cross-relay publish when
the community switches mid-flight.

Tests: 47 new passing tests across the five affected modules (five
required regression scenarios × four managers, plus watermark unit tests).

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 requested a review from a team as a code owner August 6, 2026 17:33
Duncan and others added 12 commits August 6, 2026 14:21
…d, bootstrap owns seed)

Three defects from Thufir's plan review passes are closed:

1. Relay scoping is mandatory. All four sync managers (sections, sort,
   stars, mutes) now require a defined relayUrl. Stars/mutes hooks gate
   on !pubkey || !relayUrl and construct no manager until both are
   available, preventing cross-relay watermark bleed. AppShell passes
   communitiesHook.activeCommunity?.relayUrl to useChannelMutes and
   useChannelStars. Watermark keys are always {blobType, pubkey,
   normalizedRelayUrl} — the pubkey-only fallback is removed. All four
   hooks' effects depend on [pubkey, relayUrl] so community switches
   tear down and rebind.

2. Raw head recorded before decrypt on all three observation paths.
   Initial fetch, live subscription, and fetchOwnBlobBeforePublish all
   call recordRemoteHead(event.created_at) before decryptAndParse.
   Sections/sort snapshot headBeforeFetch before advancing so the
   whole-blob LWW comparison uses the pre-fetch baseline — advancing
   first would make remote.createdAt > lastRemoteCreatedAt always false
   and silently kill the merge. Stars/mutes use per-entry mergeStores
   (timestamp-independent) so no snapshot is needed there.

3. bootstrap() owns the seed side effect. Each manager exposes
   bootstrap(localStore) that (a) calls fetchRemote*, (b) records raw
   head pre-decrypt, (c) on failed or absent+watermark>0 returns hold,
   (d) on genuine first-sync (absent + zero watermark + non-empty
   local) calls publishSections/Stars/Mutes/SortPrefs itself, (e)
   returns apply-remote when a blob was found. Hooks only act on
   apply-remote; they cannot publish during bootstrap. The simulation
   pattern (// Simulate the hook:) is gone — tests drive production
   code.

Stars/mutes destroy() already aligned with sections/sort cancel-and-
flag pattern in the prior commit; kept as-is.

Tests: all four sync managers have bootstrap()-driven mutation-sensitive
suites covering failed->hold, absent+watermark->hold, first-sync seed
observed, undecryptable head recorded, relay-A/B isolation, and
watermark restart round-trip. Sections/sort add LWW-baseline and
headBeforeFetch tests. Watermark helper tests verify normalisation and
relay isolation. 4436/4436 passing. biome check clean.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…add missing test coverage

- Extract runBootstrap() into sidebarSyncWatermark.ts; all four managers
  delegate to it instead of duplicating the 10-line policy body
- Import normalizeRelayUrl from selfProfileStorage instead of re-implementing
  it inline (resolves Paul's MINOR finding)
- channelSortSync.test.mjs: migrate all revert-fix tests to drive bootstrap()
  directly; add getPendingStore()===null on both hold paths (failed +
  absent+head>0); add LWW-baseline test; add live-sub head-before-decrypt test
- channelSectionsSync.test.mjs: add getPendingStore()===null to tests 1 and 2;
  add live-sub head-before-decrypt test
- Fix destroy-test constructors in sections/sort to pass relayUrl (required
  after relay became mandatory)
- sidebarSyncWatermark.test.mjs: add runBootstrap policy tests (5 cases)
  covering failed/absent+head/first-sync/empty-local/found branches

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…rmark tests

Delete the redundant test cases from each surface suite that are now fully
covered by sidebarSyncWatermark.test.mjs (runBootstrap policy tests):
- Remove test 1b (undecryptable event details via fetchRemote*) from all 4
- Remove test 4 (decrypt failure records head) from sections, sort, stars, mutes
- Remove test 5 (watermark restart round-trip) from sections, sort, stars, mutes

Per-surface suites now keep only load-bearing wiring tests:
  sections/sort: failed hold, absent+head hold, first-sync seed, LWW-baseline,
                 live-sub head recording
  stars/mutes:   failed hold, absent+head hold, first-sync seed,
                 relay-A/B isolation, destroy-alignment

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Move makeFakeWindow/installFakeWindow to top of sections and sort test
files so destroy tests can use the same helper instead of inline setup.
Trim verbose assertion messages in stars/mutes wiring tests 1-3.
No coverage change — all guards, mutations, and assertions preserved.
Net: -509 lines across four test files.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ntercept

Replace the watermark-advancement assertion in the sections/sort LWW
tests with a Tauri invoke intercept that captures the nip44_encrypt_to_self
plaintext. When headBeforeFetch is correctly snapshotted the remote store
wins the merge and its content is encrypted; when the snapshot is removed
(mutation: headBeforeFetch → this.lastRemoteCreatedAt) the comparison
becomes 200 > 200 → false, local wins, and the wrong content is encrypted.
This makes M4 fail rather than silently pass.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ranch to one line

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The sync manager constructors require relayUrl, but the sections/sort
hooks passed activeCommunity?.relayUrl unguarded. A boot where identity
resolves before the community constructed managers with undefined
relayUrl, silently zeroing the watermark guard and re-enabling the
stale seed-publish this PR prevents. Mirrors the mutes/stars guard.

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Extract shared test helpers into sidebarSyncTestHelpers.mjs; move
normalizeRelayUrl to shared/lib so watermark keys no longer depend on
the profile feature; drop the dead BootstrapResult re-exports, the
test-only getPersistedWatermark(), and redundant post-decrypt
recordRemoteHead calls; align advanceWatermark's parameter order with
readWatermark; correct destroy() comments to state the real trade-off.

Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
@wpfleger96
wpfleger96 merged commit b08c8b1 into main Aug 6, 2026
2 checks passed
@wpfleger96
wpfleger96 deleted the duncan/sidebar-revert-fix branch August 6, 2026 21:21
wpfleger96 pushed a commit that referenced this pull request Aug 6, 2026
* origin/main:
  Alert community owners and admins when a new key joins (#4900)
  fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot (#5086)
  chore(hooks): run desktop typecheck in pre-push (#5110)
  feat(identity): recover desktop identity from a signed-in phone (#4845)

Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
wpfleger96 added a commit that referenced this pull request Aug 6, 2026
* origin/main:
  Alert community owners and admins when a new key joins (#4900)
  fix(desktop): prevent sidebar prefs from reverting on stale-localStorage boot (#5086)
  chore(hooks): run desktop typecheck in pre-push (#5110)
  feat(identity): recover desktop identity from a signed-in phone (#4845)

Signed-off-by: Hayt <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz>
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant