diff --git a/desktop/src/app/App.tsx b/desktop/src/app/App.tsx index 0f311f3a650..f0dec3dee88 100644 --- a/desktop/src/app/App.tsx +++ b/desktop/src/app/App.tsx @@ -45,7 +45,10 @@ import { ResetFailedScreen } from "@/features/onboarding/ui/ResetFailedScreen"; import { loadCommunityDiscoveryAfterLeave } from "@/features/communities/communityStorage"; import { useCommunityInit } from "@/features/communities/useCommunityInit"; import { useNestNotifications } from "@/features/communities/useNestNotifications"; -import { useCommunities } from "@/features/communities/useCommunities"; +import { + hasCommunityForRelay, + useCommunities, +} from "@/features/communities/useCommunities"; import { loadCommunityDestination, markPendingCommunityRestore, @@ -387,8 +390,13 @@ function CommunityApp({ return; } const previousCommunityId = activeCommunity?.id; - const relayAlreadyExists = communities.some( - (community) => community.relayUrl === transaction.relayUrl, + // Must match how `addCommunity` decides the relay is already here: it + // folds a storage-equivalent spelling into the existing community, so a + // raw string compare would record `addedCommunity: true` for a community + // the connect did not create, and cancelling would then delete it. + const relayAlreadyExists = hasCommunityForRelay( + communities, + transaction.relayUrl, ); const id = addCommunity({ id: crypto.randomUUID(), diff --git a/desktop/src/features/communities/resolveCommunityUpdateResult.test.mjs b/desktop/src/features/communities/resolveCommunityUpdateResult.test.mjs index 0a52ef845d4..4f9cab50132 100644 --- a/desktop/src/features/communities/resolveCommunityUpdateResult.test.mjs +++ b/desktop/src/features/communities/resolveCommunityUpdateResult.test.mjs @@ -5,7 +5,12 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { resolveCommunityUpdateResult } from "./useCommunities.tsx"; +import { + hasCommunityForRelay, + isSameRelay, + resolveCommunityUpdateResult, +} from "./useCommunities.tsx"; +import { storageKey } from "@/features/profile/lib/selfProfileStorage"; const COMMUNITIES = [ { @@ -105,3 +110,129 @@ test("resolveCommunityUpdateResult_same_relay_url_is_not_duplicate_of_self", () }); assert.deepEqual(result, { kind: "unchanged" }); }); + +// --------------------------------------------------------------------------- +// Relay identity is the storage notion of sameness, not string equality +// --------------------------------------------------------------------------- +// +// Every per-relay slot — self profile, read state, channel sections, sort +// preference, sidebar watermark, thread activity, observed unread — is keyed +// through `normalizeRelayUrl` (trim, strip trailing slashes, lowercase). Two +// communities whose URLs normalize alike therefore already share all of that +// data, so admitting the second one does not separate them, it hides that +// they are one. + +test("isSameRelay_treatsStorageEquivalentSpellingsAsOneRelay", () => { + const canonical = "wss://relay-a.example.com"; + for (const spelling of [ + "wss://relay-a.example.com/", + "wss://relay-a.example.com//", + "WSS://Relay-A.Example.com", + " wss://relay-a.example.com ", + ]) { + assert.equal( + isSameRelay(canonical, spelling), + true, + `${spelling} keys to the same storage slot and must be the same relay`, + ); + assert.equal( + storageKey(canonical, "pk") === storageKey(spelling, "pk"), + true, + `${spelling} must actually collide in storage — the premise of this test`, + ); + } +}); + +test("isSameRelay_keepsGenuinelyDifferentRelaysApart", () => { + assert.equal( + isSameRelay("wss://relay-a.example.com", "wss://relay-b.example.com"), + false, + ); + assert.equal( + isSameRelay("wss://relay-a.example.com", "ws://relay-a.example.com"), + false, + ); +}); + +test("resolveCommunityUpdateResult_trailingSlashOfAnotherRelay_isADuplicate", () => { + // Before: raw === missed this, so the edit was accepted and the two + // communities silently shared every per-relay storage slot. + const result = resolveCommunityUpdateResult(COMMUNITIES, "ws-1", "ws-1", { + relayUrl: "wss://relay-b.example.com/", + }); + assert.deepEqual(result, { kind: "duplicate-relay" }); +}); + +test("resolveCommunityUpdateResult_caseOnlyEditOfAnotherRelay_isADuplicate", () => { + const result = resolveCommunityUpdateResult(COMMUNITIES, "ws-1", "ws-1", { + relayUrl: "WSS://Relay-B.Example.com", + }); + assert.deepEqual(result, { kind: "duplicate-relay" }); +}); + +test("resolveCommunityUpdateResult_reSpellingOwnRelay_isStillAnUpdate", () => { + // Editing your own relay to an equivalent spelling is not a duplicate of + // yourself — it stays a normal update, and on the active community it still + // reinitialises the backend, because the connection URL really did change. + const result = resolveCommunityUpdateResult(COMMUNITIES, "ws-1", "ws-1", { + relayUrl: "wss://relay-a.example.com/", + }); + assert.deepEqual(result, { kind: "updated", requiresReinit: true }); +}); + +// --------------------------------------------------------------------------- +// The onboarding rollback flag must agree with addCommunity +// --------------------------------------------------------------------------- +// +// `handleCommunityOnboardingConnect` records `addedCommunity` from this +// predicate and `handleCommunityOnboardingCancel` acts on it: a true flag lets +// cancel remove the community, or `clearCommunities()` it when it is the only +// one. Since `addCommunity` folds an equivalent spelling into the existing +// community rather than creating a new one, asking with raw `===` here would +// arm that rollback against a community the connect never created. + +test("hasCommunityForRelay_reportsAnEquivalentSpellingAsAlreadyPresent", () => { + for (const spelling of [ + "wss://relay-a.example.com/", + "wss://relay-a.example.com//", + "WSS://Relay-A.Example.com", + " wss://relay-a.example.com ", + ]) { + assert.equal( + hasCommunityForRelay(COMMUNITIES, spelling), + true, + `${spelling} is the existing relay, so the connect added nothing`, + ); + } +}); + +test("hasCommunityForRelay_reportsAGenuinelyNewRelayAsAbsent", () => { + assert.equal( + hasCommunityForRelay(COMMUNITIES, "wss://relay-c.example.com"), + false, + ); + assert.equal( + hasCommunityForRelay(COMMUNITIES, "ws://relay-a.example.com"), + false, + ); + assert.equal(hasCommunityForRelay([], "wss://relay-a.example.com"), false); +}); + +test("hasCommunityForRelay_agreesWithTheMatchAddCommunityUses", () => { + // addCommunity folds via isSameRelay; the rollback flag must not disagree + // with it for any spelling, or cancel deletes a pre-existing community. + for (const spelling of [ + "wss://relay-a.example.com", + "WSS://Relay-A.Example.com/", + "wss://relay-b.example.com//", + "wss://relay-c.example.com", + ]) { + assert.equal( + hasCommunityForRelay(COMMUNITIES, spelling), + COMMUNITIES.some((community) => + isSameRelay(community.relayUrl, spelling), + ), + `${spelling} must resolve the same way for both`, + ); + } +}); diff --git a/desktop/src/features/communities/useCommunities.tsx b/desktop/src/features/communities/useCommunities.tsx index e0a10017883..05969868347 100644 --- a/desktop/src/features/communities/useCommunities.tsx +++ b/desktop/src/features/communities/useCommunities.tsx @@ -16,6 +16,7 @@ import { saveActiveCommunityId, saveCommunities, } from "./communityStorage"; +import { normalizeRelayUrl } from "@/shared/lib/normalizeRelayUrl"; import { removeSelfProfileCachesForRelay } from "@/features/profile/lib/selfProfileStorage"; import { removeUserLabelCacheForRelay } from "@/features/profile/lib/userLabelStorage"; import { removeChannelSnapshotForRelay } from "@/features/channels/channelSnapshot"; @@ -32,6 +33,42 @@ export type UpdateCommunityResult = | { kind: "duplicate-relay" } | { kind: "not-found" }; +/** + * Whether two relay URLs identify the same community. + * + * Deliberately the *storage* notion of sameness, not string equality: every + * per-relay slot — self profile, read state, channel sections, sort + * preference, sidebar watermark, thread activity, observed unread — is keyed + * through `normalizeRelayUrl`, so two URLs that normalize alike already share + * all of that data. Letting them exist as two communities does not separate + * them; it just hides that they are one. + * + * `wss://relay.example`, `wss://relay.example/`, `WSS://Relay.Example` and + * `wss://relay.example//` are all one relay by that measure. + */ +export function isSameRelay(left: string, right: string): boolean { + return normalizeRelayUrl(left) === normalizeRelayUrl(right); +} + +/** + * Whether the community list already holds the relay `relayUrl` names. + * + * Callers that decide *whether an add created a community* must ask this, not + * compare raw strings: `addCommunity` folds a storage-equivalent spelling into + * the existing community and hands back its id, so a raw comparison marks a + * pre-existing community as freshly added. Onboarding's rollback then treats + * it as temporary and can remove it — or `clearCommunities()` when it is the + * only one. Keep this predicate and `addCommunity`'s match in lockstep. + */ +export function hasCommunityForRelay( + communities: readonly Pick[], + relayUrl: string, +): boolean { + return communities.some((community) => + isSameRelay(community.relayUrl, relayUrl), + ); +} + /** * Pure decision logic for updateCommunity — determines the outcome from a * synchronous snapshot of communities without side effects. Extracted so the @@ -48,10 +85,14 @@ export function resolveCommunityUpdateResult( const current = communities.find((w) => w.id === id); if (!current) return { kind: "not-found" }; + // Hoisted so the narrowing survives into the callback below. + const nextRelayUrl = updates.relayUrl; if ( - updates.relayUrl !== undefined && - updates.relayUrl !== current.relayUrl && - communities.some((w) => w.id !== id && w.relayUrl === updates.relayUrl) + nextRelayUrl !== undefined && + nextRelayUrl !== current.relayUrl && + communities.some( + (w) => w.id !== id && isSameRelay(w.relayUrl, nextRelayUrl), + ) ) { return { kind: "duplicate-relay" }; } @@ -187,12 +228,12 @@ function useCommunitiesInternal(): UseCommunitiesReturn { ); const addCommunity = useCallback((community: Community): string => { - const existing = communitiesRef.current.find( - (w) => w.relayUrl === community.relayUrl, + const existing = communitiesRef.current.find((w) => + isSameRelay(w.relayUrl, community.relayUrl), ); const resolvedId = existing?.id ?? community.id; setCommunitiesState((prev) => { - const dup = prev.find((w) => w.relayUrl === community.relayUrl); + const dup = prev.find((w) => isSameRelay(w.relayUrl, community.relayUrl)); let next: Community[]; if (dup) { next = prev.map((w) =>