fix(sidebar): scope channel sections storage to relay URL#1477
Merged
Conversation
Each user's custom sidebar sections are semantically tied to a specific relay/workspace. Using a pubkey-only localStorage key caused sections created on one relay to appear (empty) on all others, and the mount-time seed-publish path would upload one relay's sections to a freshly-opened workspace. - storageKey() now accepts an optional relayUrl; when present the key becomes buzz-channel-sections.v1:<pubkey>:<encodedRelay>, keeping sections from different relays completely isolated. - readChannelSectionsStore() performs a one-time migration: on the first scoped read it copies any non-empty legacy (pubkey-only) data to the scoped key so existing users don't lose their sections on upgrade. The legacy key is left in place to stay compatible with older builds. - useChannelSections() now accepts relayUrl as a second param and adds it to all relevant effect/callback dep arrays so the hook re-reads and re-initialises the sync manager whenever the active workspace changes. AppSidebar passes activeWorkspace?.relayUrl; when undefined (no active workspace or workspace-rail experiment off) the hook falls back to the legacy unscoped key, preserving single-workspace behaviour. - channelSectionsStorage.test.mjs: 7 new tests covering scoped key format, cross-relay isolation, migration path, and precedence. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Two review findings addressed: 1. Relay URL normalization: storageKey() now applies normalizeRelayUrl() from selfProfileStorage before encoding the relay — matching the same trim/lowercase/trailing-slash semantics used by channelSnapshot.ts and messageSnapshot.ts. Raw relay text would have produced different keys for equivalent URLs (e.g. WSS://Relay.Example/ vs wss://relay.example). 2. Migration is globally one-time: readChannelSectionsStore() now deletes the legacy pubkey-only key immediately after a successful scoped write. Previously the legacy key was left in place, causing every new relay scope's first read to copy legacy data into itself — recreating the cross-relay contamination vector the PR was meant to kill. Tests added/updated: key normalization (case + trailing slash collapse), migration deletes legacy key after first use, globally one-time migration (relay B returns DEFAULT_STORE after relay A migrated the legacy data). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
destroy() was calling doPublish(pendingStore) asynchronously when a pending debounce was in flight. On workspace switch, the [pubkey, relayUrl] dep change triggers the effect cleanup — destroying the manager and flushing the publish — while the relayClient singleton is mid-transition to the new relay. The async doPublish therefore races against the backend socket moving to relay B and can land relay A's channel-sections event on B. Fix: destroy() now calls cancelPendingPublish() and nulls pendingStore instead of flushing. The scoped localStorage write is already durable; when the user returns to relay A the existing no-remote seed-publish path will publish A's local store to A on next mount. Tests added: destroy() cancels timer and leaves no publishEvent call, destroy() with no pending is safe, pendingStore is null after destroy. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
destroy() previously only cancelled the debounce timer, but once the timer fires the callback calls doPublish() asynchronously. If destroy() is called while doPublish() is awaiting fetchOwnBlobBeforePublish(), the timer is already gone (nulled by the callback) so destroy() could not stop the in-flight publish. doPublish() would swallow the disconnect rejection inside fetchOwnBlobBeforePublish, return the local store, and proceed to relayClient.publishEvent() against a singleton that had already moved to relay B. Fix: add a private `destroyed` boolean to ChannelSectionSyncManager, set it in destroy(), and check it at two points in doPublish(): after the fetchOwnBlobBeforePublish await and immediately before the relayClient.publishEvent() call. Either check short-circuits the publish if the manager has been torn down. Regression test added: manually fire the debounce callback, hold fetchEvents to simulate the latency window, call destroy(), release the fetch, drain microtasks — asserts publishEvent is never called. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
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
1 participant
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.
Problem
Custom sidebar sections use a pubkey-only localStorage key (
buzz-channel-sections.v1:<pubkey>), so every relay/workspace reads the same store. Two consequences:useChannelSectionspublishes the local store to the current relay when no remote event exists. With a shared store, switching to a fresh workspace uploads the old workspace's sections to it.Fix
Scope the localStorage key to relay URL:
buzz-channel-sections.v1:<pubkey>:<encodedNormalizedRelay>. Two workspaces pointing at the same relay still share sections (correct); different relays are fully isolated.Key changes:
channelSectionsStorage.ts: importsnormalizeRelayUrlfromselfProfileStorage(the same helper used bychannelSnapshot.tsandmessageSnapshot.ts) and applies it before encoding the relay in the storage key, so equivalent URL formatting (WSS://Relay.Example/vswss://relay.example) maps to the same key.readChannelSectionsStoreperforms a globally one-time migration — on the first scoped read it copies any non-empty legacy (pubkey-only) data to the scoped key, then deletes the legacy key so no subsequent relay scope can pick it up.writeChannelSectionsStoretakes optionalrelayUrlthird param.useChannelSections(pubkey, relayUrl?): threadsrelayUrlthrough all effect/callback dep arrays so the hook re-reads storage and re-initialises its sync manager when the active workspace changes.AppSidebar.tsx: passesactiveWorkspace?.relayUrl. Whenundefined(workspace-rail experiment off, or workspace not yet loaded) the hook falls back to the legacy unscoped key — single-workspace users are unaffected.channelSectionsSync.ts:destroy()now cancels any pending debounced publish instead of flushing it. The scoped localStorage write is already durable; when the user returns to relay A the existing seed-publish path re-publishes from local state on next mount. This closes the race where an in-flight publish for relay A could fire after the relayClient singleton has moved to relay B.Cross-relay publish paths closed:
sections.length === 0suppresses the seed-publish.destroy()cancels the timer without callingdoPublish, so no event is sent to any relay during teardown.Tests
9 new tests in
channelSectionsStorage.test.mjs: scoped key format, normalization (case + trailing slash collapse), cross-relay isolation, migration from legacy key, migration deletes legacy key (globally one-time), migration is blocked for relay B once relay A consumed the legacy key, migration skips empty legacy stores, scoped key takes precedence post-migration.3 new tests in
channelSectionsSync.test.mjs:destroy()cancels timer and emits nopublishEventcall,destroy()with no pending is safe,pendingStoreis null afterdestroy().