From 7c52027afbcf56efcd08ef0ba85bc317986421f0 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Sat, 12 Sep 2026 14:48:35 -0400 Subject: [PATCH] fix(rooms): serialize per-room rename writes so back-to-back renames survive Local room names did a read-modify-write of io.lotus.room_names against the SDK's local cache, which is stale until the /sync echo, so a second rename issued before the first echoed overwrote it. Route through createAccountDataListStore like user notes. Unit-tested with a client whose setAccountData does not update the local store. Fixes #17 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA --- src/app/features/room-nav/RoomNavItem.tsx | 21 ++--- src/app/hooks/useRoomMeta.test.ts | 93 +++++++++++++++++++ src/app/hooks/useRoomMeta.ts | 103 ++++++++++------------ 3 files changed, 148 insertions(+), 69 deletions(-) create mode 100644 src/app/hooks/useRoomMeta.test.ts diff --git a/src/app/features/room-nav/RoomNavItem.tsx b/src/app/features/room-nav/RoomNavItem.tsx index a4746c417..26866b3de 100644 --- a/src/app/features/room-nav/RoomNavItem.tsx +++ b/src/app/features/room-nav/RoomNavItem.tsx @@ -42,7 +42,6 @@ import { NavItem, NavItemContent, NavItemOptions, NavLink } from '../../componen import { UnreadBadge, UnreadBadgeCenter } from '../../components/unread-badge'; import { RoomAvatar, RoomIcon } from '../../components/room-avatar'; import { getDirectRoomAvatarUrl, getRoomAvatarUrl, getStateEvent } from '../../utils/room'; -import { setAccountData } from '../../utils/accountData'; import { nameInitials } from '../../utils/common'; import { useMatrixClient } from '../../hooks/useMatrixClient'; import { useRoomUnread } from '../../state/hooks/unread'; @@ -74,8 +73,8 @@ import { getRoomCreatorsForRoomId, useRoomCreators } from '../../hooks/useRoomCr import { getRoomPermissionsAPI, useRoomPermissions } from '../../hooks/useRoomPermissions'; import { InviteUserPrompt } from '../../components/invite-user-prompt'; import { - LOCAL_ROOM_NAMES_KEY, getLocalRoomNamesContent, + setLocalRoomName, useHasLocalRoomName, useLocalRoomName, } from '../../hooks/useRoomMeta'; @@ -138,22 +137,16 @@ function RenameRoomDialog({ room, onClose }: RenameRoomDialogProps) { const handleSave = useCallback(() => { const newName = inputRef.current?.value.trim() ?? ''; if (newName.length > 255) return; - const existing = getLocalRoomNamesContent(mx); - if (newName === '') { - const { [room.roomId]: _removed, ...rest } = existing.rooms; - setAccountData(mx, LOCAL_ROOM_NAMES_KEY, { rooms: rest }); - } else { - setAccountData(mx, LOCAL_ROOM_NAMES_KEY, { - rooms: { ...existing.rooms, [room.roomId]: newName }, - }); - } + // Routed through the shared write queue (setLocalRoomName) instead of a + // read-modify-write against the SDK's local cache, which stays stale + // until the /sync echo lands and would otherwise let a second rename + // clobber a still-in-flight first rename. + setLocalRoomName(mx, room.roomId, newName); onClose(); }, [mx, room.roomId, onClose]); const handleClear = useCallback(() => { - const existing = getLocalRoomNamesContent(mx); - const { [room.roomId]: _removed, ...rest } = existing.rooms; - setAccountData(mx, LOCAL_ROOM_NAMES_KEY, { rooms: rest }); + setLocalRoomName(mx, room.roomId, ''); onClose(); }, [mx, room.roomId, onClose]); diff --git a/src/app/hooks/useRoomMeta.test.ts b/src/app/hooks/useRoomMeta.test.ts new file mode 100644 index 000000000..d7d7825b3 --- /dev/null +++ b/src/app/hooks/useRoomMeta.test.ts @@ -0,0 +1,93 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import type { MatrixClient } from 'matrix-js-sdk'; +import { getLocalRoomNamesContent, setLocalRoomName } from './useRoomMeta'; + +// Minimal fake client. Mirrors the real SDK behavior that matters here: +// setAccountData resolves WITHOUT updating what getAccountData returns — the +// local cache only updates once the /sync echo is delivered via the +// AccountData listener. This is exactly the staleness that let two +// back-to-back renames clobber each other before the fix (issue #17). +const makeFakeMx = () => { + const accountData: Record = {}; + const listeners: Array<(e: { getType: () => string; getContent: () => unknown }) => void> = []; + const setAccountDataCalls: Array<{ type: string; content: unknown }> = []; + + const mx = { + getAccountData: (type: string) => { + const content = accountData[type]; + return content ? { getContent: () => content } : undefined; + }, + setAccountData: (type: string, content: unknown) => { + setAccountDataCalls.push({ type, content }); + // Deliberately do NOT update `accountData` here — the real SDK doesn't + // either. It only updates on the emitted echo below. + return Promise.resolve(); + }, + on: (_event: unknown, h: (e: { getType: () => string; getContent: () => unknown }) => void) => { + listeners.push(h); + }, + removeListener: ( + _event: unknown, + h: (e: { getType: () => string; getContent: () => unknown }) => void, + ) => { + const i = listeners.indexOf(h); + if (i >= 0) listeners.splice(i, 1); + }, + }; + + const emitEcho = (type: string, content: unknown) => { + accountData[type] = content; + listeners.forEach((h) => h({ getType: () => type, getContent: () => content })); + }; + + return { + mx: mx as unknown as MatrixClient, + emitEcho, + setAccountDataCalls, + }; +}; + +test('back-to-back renames of different rooms both survive with no echo in between', async () => { + const { mx } = makeFakeMx(); + + // Rename room A, then room B, before either write's /sync echo has landed — + // the exact scenario from issue #17. + const writeA = setLocalRoomName(mx, '!a:example.org', 'Room A renamed'); + const writeB = setLocalRoomName(mx, '!b:example.org', 'Room B renamed'); + await Promise.all([writeA, writeB]); + + const content = getLocalRoomNamesContent(mx); + assert.deepEqual(content.rooms, { + '!a:example.org': 'Room A renamed', + '!b:example.org': 'Room B renamed', + }); +}); + +test("writes are serialized: the second write computes from the first write's result", async () => { + const { mx, setAccountDataCalls } = makeFakeMx(); + + await Promise.all([ + setLocalRoomName(mx, '!a:example.org', 'A'), + setLocalRoomName(mx, '!b:example.org', 'B'), + ]); + + // The last PUT to the server must carry both renames — proof the second + // write's compute() saw the first write's in-memory result rather than a + // stale snapshot from before it landed. + const lastCall = setAccountDataCalls[setAccountDataCalls.length - 1]; + assert.deepEqual(lastCall.content, { + rooms: { '!a:example.org': 'A', '!b:example.org': 'B' }, + }); +}); + +test('clearing a local name removes only that room', async () => { + const { mx } = makeFakeMx(); + + await setLocalRoomName(mx, '!a:example.org', 'A'); + await setLocalRoomName(mx, '!b:example.org', 'B'); + await setLocalRoomName(mx, '!a:example.org', ''); + + const content = getLocalRoomNamesContent(mx); + assert.deepEqual(content.rooms, { '!b:example.org': 'B' }); +}); diff --git a/src/app/hooks/useRoomMeta.ts b/src/app/hooks/useRoomMeta.ts index d3fecd61b..abdd83c30 100644 --- a/src/app/hooks/useRoomMeta.ts +++ b/src/app/hooks/useRoomMeta.ts @@ -1,10 +1,10 @@ -import { useCallback, useEffect, useState } from 'react'; +import { useEffect, useState } from 'react'; import { RoomJoinRulesEventContent } from 'matrix-js-sdk/lib/types'; -import { ClientEvent, MatrixEvent, Room, RoomEvent, RoomEventHandlerMap } from 'matrix-js-sdk'; +import { Room, RoomEvent, RoomEventHandlerMap } from 'matrix-js-sdk'; import { StateEvent } from '../../types/matrix/room'; import { useStateEvent } from './useStateEvent'; import { useMatrixClient } from './useMatrixClient'; -import { getAccountData } from '../utils/accountData'; +import { createAccountDataListStore } from './createAccountDataListStore'; export const useRoomAvatar = (room: Room, dm?: boolean): string | undefined => { const avatarEvent = useStateEvent(room, StateEvent.RoomAvatar); @@ -40,79 +40,72 @@ export const LOCAL_ROOM_NAMES_KEY = 'io.lotus.room_names'; export type LocalRoomNamesContent = { rooms: Record }; +type LocalRoomNamesMap = Record; + +// Shared, concurrency-safe store. See createAccountDataListStore for why the +// snapshot + write queue must be module-scoped: setAccountData does not update +// the SDK's local cache (it only resolves once the /sync echo lands), so a +// plain read-modify-write against getAccountData can lose a rename that is +// still in flight when a second rename is issued (fixed: back-to-back renames +// of different rooms no longer clobber each other). +const roomNamesStore = createAccountDataListStore({ + eventType: LOCAL_ROOM_NAMES_KEY, + read: (content) => + content && typeof content === 'object' && typeof content.rooms === 'object' + ? content.rooms + : {}, + write: (rooms) => ({ rooms }), +}); + export function getLocalRoomNamesContent( mx: ReturnType, ): LocalRoomNamesContent { - const raw: unknown = getAccountData(mx, LOCAL_ROOM_NAMES_KEY); - if ( - raw && - typeof raw === 'object' && - 'rooms' in raw && - // eslint-disable-next-line @typescript-eslint/no-explicit-any - typeof (raw as any).rooms === 'object' - ) { - return raw as LocalRoomNamesContent; - } - return { rooms: {} }; + return { rooms: roomNamesStore.getLatest(mx) }; +} + +/** + * Set (or clear, when `name` is empty) the local display name for a room. + * Routed through the shared write queue so back-to-back renames of different + * rooms are always computed from the latest snapshot instead of a stale one. + */ +export function setLocalRoomName( + mx: ReturnType, + roomId: string, + name: string, +): Promise { + return roomNamesStore.enqueueWrite(mx, (current) => { + if (!name) { + const { [roomId]: _removed, ...rest } = current; + return rest; + } + return { ...current, [roomId]: name }; + }); } export const useLocalRoomName = (room: Room): string => { const mx = useMatrixClient(); - - const getLocalName = useCallback((): string => { - const content = getLocalRoomNamesContent(mx); - return content.rooms[room.roomId] ?? room.name; - }, [mx, room]); - - const [name, setName] = useState(getLocalName); + const localNames = roomNamesStore.useValue(mx); + const [name, setName] = useState(room.name); useEffect(() => { - setName(getLocalName()); - - const handleAccountData = (event: MatrixEvent) => { - if (event.getType() !== LOCAL_ROOM_NAMES_KEY) return; - setName(getLocalName()); - }; - mx.on(ClientEvent.AccountData, handleAccountData); + setName(room.name); const handleRoomNameChange: RoomEventHandlerMap[RoomEvent.Name] = () => { - setName(getLocalName()); + setName(room.name); }; room.on(RoomEvent.Name, handleRoomNameChange); - return () => { - mx.removeListener(ClientEvent.AccountData, handleAccountData); room.removeListener(RoomEvent.Name, handleRoomNameChange); }; - }, [mx, room, getLocalName]); + }, [room]); - return name; + return localNames[room.roomId] ?? name; }; export const useHasLocalRoomName = (roomId: string): boolean => { const mx = useMatrixClient(); - - const check = useCallback((): boolean => { - const content = getLocalRoomNamesContent(mx); - return !!content.rooms[roomId]; - }, [mx, roomId]); - - const [hasLocal, setHasLocal] = useState(check); - - useEffect(() => { - setHasLocal(check()); - - const handleAccountData = (event: MatrixEvent) => { - if (event.getType() !== LOCAL_ROOM_NAMES_KEY) return; - setHasLocal(check()); - }; - mx.on(ClientEvent.AccountData, handleAccountData); - return () => { - mx.removeListener(ClientEvent.AccountData, handleAccountData); - }; - }, [mx, check]); - - return hasLocal; + const localNames = roomNamesStore.useValue(mx); + return !!localNames[roomId]; }; export type RoomTopicContent = {