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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA
This commit is contained in:
@@ -42,7 +42,6 @@ import { NavItem, NavItemContent, NavItemOptions, NavLink } from '../../componen
|
|||||||
import { UnreadBadge, UnreadBadgeCenter } from '../../components/unread-badge';
|
import { UnreadBadge, UnreadBadgeCenter } from '../../components/unread-badge';
|
||||||
import { RoomAvatar, RoomIcon } from '../../components/room-avatar';
|
import { RoomAvatar, RoomIcon } from '../../components/room-avatar';
|
||||||
import { getDirectRoomAvatarUrl, getRoomAvatarUrl, getStateEvent } from '../../utils/room';
|
import { getDirectRoomAvatarUrl, getRoomAvatarUrl, getStateEvent } from '../../utils/room';
|
||||||
import { setAccountData } from '../../utils/accountData';
|
|
||||||
import { nameInitials } from '../../utils/common';
|
import { nameInitials } from '../../utils/common';
|
||||||
import { useMatrixClient } from '../../hooks/useMatrixClient';
|
import { useMatrixClient } from '../../hooks/useMatrixClient';
|
||||||
import { useRoomUnread } from '../../state/hooks/unread';
|
import { useRoomUnread } from '../../state/hooks/unread';
|
||||||
@@ -74,8 +73,8 @@ import { getRoomCreatorsForRoomId, useRoomCreators } from '../../hooks/useRoomCr
|
|||||||
import { getRoomPermissionsAPI, useRoomPermissions } from '../../hooks/useRoomPermissions';
|
import { getRoomPermissionsAPI, useRoomPermissions } from '../../hooks/useRoomPermissions';
|
||||||
import { InviteUserPrompt } from '../../components/invite-user-prompt';
|
import { InviteUserPrompt } from '../../components/invite-user-prompt';
|
||||||
import {
|
import {
|
||||||
LOCAL_ROOM_NAMES_KEY,
|
|
||||||
getLocalRoomNamesContent,
|
getLocalRoomNamesContent,
|
||||||
|
setLocalRoomName,
|
||||||
useHasLocalRoomName,
|
useHasLocalRoomName,
|
||||||
useLocalRoomName,
|
useLocalRoomName,
|
||||||
} from '../../hooks/useRoomMeta';
|
} from '../../hooks/useRoomMeta';
|
||||||
@@ -138,22 +137,16 @@ function RenameRoomDialog({ room, onClose }: RenameRoomDialogProps) {
|
|||||||
const handleSave = useCallback(() => {
|
const handleSave = useCallback(() => {
|
||||||
const newName = inputRef.current?.value.trim() ?? '';
|
const newName = inputRef.current?.value.trim() ?? '';
|
||||||
if (newName.length > 255) return;
|
if (newName.length > 255) return;
|
||||||
const existing = getLocalRoomNamesContent(mx);
|
// Routed through the shared write queue (setLocalRoomName) instead of a
|
||||||
if (newName === '') {
|
// read-modify-write against the SDK's local cache, which stays stale
|
||||||
const { [room.roomId]: _removed, ...rest } = existing.rooms;
|
// until the /sync echo lands and would otherwise let a second rename
|
||||||
setAccountData(mx, LOCAL_ROOM_NAMES_KEY, { rooms: rest });
|
// clobber a still-in-flight first rename.
|
||||||
} else {
|
setLocalRoomName(mx, room.roomId, newName);
|
||||||
setAccountData(mx, LOCAL_ROOM_NAMES_KEY, {
|
|
||||||
rooms: { ...existing.rooms, [room.roomId]: newName },
|
|
||||||
});
|
|
||||||
}
|
|
||||||
onClose();
|
onClose();
|
||||||
}, [mx, room.roomId, onClose]);
|
}, [mx, room.roomId, onClose]);
|
||||||
|
|
||||||
const handleClear = useCallback(() => {
|
const handleClear = useCallback(() => {
|
||||||
const existing = getLocalRoomNamesContent(mx);
|
setLocalRoomName(mx, room.roomId, '');
|
||||||
const { [room.roomId]: _removed, ...rest } = existing.rooms;
|
|
||||||
setAccountData(mx, LOCAL_ROOM_NAMES_KEY, { rooms: rest });
|
|
||||||
onClose();
|
onClose();
|
||||||
}, [mx, room.roomId, onClose]);
|
}, [mx, room.roomId, onClose]);
|
||||||
|
|
||||||
|
|||||||
@@ -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<string, unknown> = {};
|
||||||
|
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' });
|
||||||
|
});
|
||||||
@@ -1,10 +1,10 @@
|
|||||||
import { useCallback, useEffect, useState } from 'react';
|
import { useEffect, useState } from 'react';
|
||||||
import { RoomJoinRulesEventContent } from 'matrix-js-sdk/lib/types';
|
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 { StateEvent } from '../../types/matrix/room';
|
||||||
import { useStateEvent } from './useStateEvent';
|
import { useStateEvent } from './useStateEvent';
|
||||||
import { useMatrixClient } from './useMatrixClient';
|
import { useMatrixClient } from './useMatrixClient';
|
||||||
import { getAccountData } from '../utils/accountData';
|
import { createAccountDataListStore } from './createAccountDataListStore';
|
||||||
|
|
||||||
export const useRoomAvatar = (room: Room, dm?: boolean): string | undefined => {
|
export const useRoomAvatar = (room: Room, dm?: boolean): string | undefined => {
|
||||||
const avatarEvent = useStateEvent(room, StateEvent.RoomAvatar);
|
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<string, string> };
|
export type LocalRoomNamesContent = { rooms: Record<string, string> };
|
||||||
|
|
||||||
|
type LocalRoomNamesMap = Record<string, string>;
|
||||||
|
|
||||||
|
// 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<LocalRoomNamesMap, LocalRoomNamesContent>({
|
||||||
|
eventType: LOCAL_ROOM_NAMES_KEY,
|
||||||
|
read: (content) =>
|
||||||
|
content && typeof content === 'object' && typeof content.rooms === 'object'
|
||||||
|
? content.rooms
|
||||||
|
: {},
|
||||||
|
write: (rooms) => ({ rooms }),
|
||||||
|
});
|
||||||
|
|
||||||
export function getLocalRoomNamesContent(
|
export function getLocalRoomNamesContent(
|
||||||
mx: ReturnType<typeof useMatrixClient>,
|
mx: ReturnType<typeof useMatrixClient>,
|
||||||
): LocalRoomNamesContent {
|
): LocalRoomNamesContent {
|
||||||
const raw: unknown = getAccountData<unknown>(mx, LOCAL_ROOM_NAMES_KEY);
|
return { rooms: roomNamesStore.getLatest(mx) };
|
||||||
if (
|
}
|
||||||
raw &&
|
|
||||||
typeof raw === 'object' &&
|
/**
|
||||||
'rooms' in raw &&
|
* Set (or clear, when `name` is empty) the local display name for a room.
|
||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
* Routed through the shared write queue so back-to-back renames of different
|
||||||
typeof (raw as any).rooms === 'object'
|
* rooms are always computed from the latest snapshot instead of a stale one.
|
||||||
) {
|
*/
|
||||||
return raw as LocalRoomNamesContent;
|
export function setLocalRoomName(
|
||||||
}
|
mx: ReturnType<typeof useMatrixClient>,
|
||||||
return { rooms: {} };
|
roomId: string,
|
||||||
|
name: string,
|
||||||
|
): Promise<void> {
|
||||||
|
return roomNamesStore.enqueueWrite(mx, (current) => {
|
||||||
|
if (!name) {
|
||||||
|
const { [roomId]: _removed, ...rest } = current;
|
||||||
|
return rest;
|
||||||
|
}
|
||||||
|
return { ...current, [roomId]: name };
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
export const useLocalRoomName = (room: Room): string => {
|
export const useLocalRoomName = (room: Room): string => {
|
||||||
const mx = useMatrixClient();
|
const mx = useMatrixClient();
|
||||||
|
const localNames = roomNamesStore.useValue(mx);
|
||||||
const getLocalName = useCallback((): string => {
|
const [name, setName] = useState(room.name);
|
||||||
const content = getLocalRoomNamesContent(mx);
|
|
||||||
return content.rooms[room.roomId] ?? room.name;
|
|
||||||
}, [mx, room]);
|
|
||||||
|
|
||||||
const [name, setName] = useState(getLocalName);
|
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
setName(getLocalName());
|
setName(room.name);
|
||||||
|
|
||||||
const handleAccountData = (event: MatrixEvent) => {
|
|
||||||
if (event.getType() !== LOCAL_ROOM_NAMES_KEY) return;
|
|
||||||
setName(getLocalName());
|
|
||||||
};
|
|
||||||
mx.on(ClientEvent.AccountData, handleAccountData);
|
|
||||||
|
|
||||||
const handleRoomNameChange: RoomEventHandlerMap[RoomEvent.Name] = () => {
|
const handleRoomNameChange: RoomEventHandlerMap[RoomEvent.Name] = () => {
|
||||||
setName(getLocalName());
|
setName(room.name);
|
||||||
};
|
};
|
||||||
room.on(RoomEvent.Name, handleRoomNameChange);
|
room.on(RoomEvent.Name, handleRoomNameChange);
|
||||||
|
|
||||||
return () => {
|
return () => {
|
||||||
mx.removeListener(ClientEvent.AccountData, handleAccountData);
|
|
||||||
room.removeListener(RoomEvent.Name, handleRoomNameChange);
|
room.removeListener(RoomEvent.Name, handleRoomNameChange);
|
||||||
};
|
};
|
||||||
}, [mx, room, getLocalName]);
|
}, [room]);
|
||||||
|
|
||||||
return name;
|
return localNames[room.roomId] ?? name;
|
||||||
};
|
};
|
||||||
|
|
||||||
export const useHasLocalRoomName = (roomId: string): boolean => {
|
export const useHasLocalRoomName = (roomId: string): boolean => {
|
||||||
const mx = useMatrixClient();
|
const mx = useMatrixClient();
|
||||||
|
const localNames = roomNamesStore.useValue(mx);
|
||||||
const check = useCallback((): boolean => {
|
return !!localNames[roomId];
|
||||||
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;
|
|
||||||
};
|
};
|
||||||
|
|
||||||
export type RoomTopicContent = {
|
export type RoomTopicContent = {
|
||||||
|
|||||||
Reference in New Issue
Block a user