From e9c43f21f1e41abc122833fb7d75000fb691f352 Mon Sep 17 00:00:00 2001 From: Jannis Braun <151788261+TheZwiss@users.noreply.github.com> Date: Mon, 16 Mar 2026 20:20:25 +0100 Subject: [PATCH] fix: eliminate phantom notifications across the entire read-state pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: own messages echoed by the server marked channels unread when the user had already navigated away. Seven related bugs compounded the problem — stale read states, missing cleanup on space/DM removal, REST broadcast ignoring VIEW_CHANNEL, and no validation on channel_ack writes. Frontend: - Skip markChannelUnread for the user's own messages (federation-aware) - Walk backward past temp_ IDs in ackChannel instead of bailing - Re-fire ack timer when temp message is replaced by server-confirmed ID - Add removeChannelStates to clean up unread/read/message caches - Clean up chatStore on removeSpace, removeDmChannel, removeInstanceSpaces Server: - Use sendToChannel instead of sendToSpace for REST message creation - Clean up read_states on space deletion, member kick/leave, and ban - Validate channel membership before accepting channel_ack writes - Clean up read_states on DM leave and DM channel deletion --- packages/server/src/routes/dm.ts | 9 ++++ packages/server/src/routes/messages.ts | 2 +- packages/server/src/routes/spaces.ts | 28 +++++++++- packages/server/src/ws/events.ts | 8 +++ .../web/src/components/chat/MessageList.tsx | 5 +- packages/web/src/hooks/useWebSocket.ts | 6 ++- packages/web/src/stores/chatStore.ts | 34 +++++++++--- packages/web/src/stores/spaceStore.ts | 53 +++++++++++++------ 8 files changed, 118 insertions(+), 27 deletions(-) diff --git a/packages/server/src/routes/dm.ts b/packages/server/src/routes/dm.ts index 1098c8a7..c3995fd9 100644 --- a/packages/server/src/routes/dm.ts +++ b/packages/server/src/routes/dm.ts @@ -695,6 +695,12 @@ export async function dmRoutes(app: FastifyInstance): Promise { )) .run(); + // Clean up read_states for the departing user + db.delete(schema.readStates).where(and( + eq(schema.readStates.userId, request.userId), + eq(schema.readStates.channelId, id), + )).run(); + // Check remaining members const remainingMembers = db.select() .from(schema.dmMembers) @@ -743,6 +749,9 @@ export async function dmRoutes(app: FastifyInstance): Promise { }); } + // Clean up all read_states for this DM channel (all members' rows) + db.delete(schema.readStates).where(eq(schema.readStates.channelId, id)).run(); + // Delete the DM channel (cascades to dm_messages) db.delete(schema.dmChannels).where(eq(schema.dmChannels.id, id)).run(); diff --git a/packages/server/src/routes/messages.ts b/packages/server/src/routes/messages.ts index 8461e085..bba922d1 100644 --- a/packages/server/src/routes/messages.ts +++ b/packages/server/src/routes/messages.ts @@ -343,7 +343,7 @@ export async function messageRoutes(app: FastifyInstance): Promise { const messageWithUser = buildMessageWithUser(message, user, attachmentRows, [], replyTo); // Broadcast via WebSocket - connectionManager.sendToSpace(spaceId, { + connectionManager.sendToChannel(spaceId, id, { type: 'message_created', message: messageWithUser, }); diff --git a/packages/server/src/routes/spaces.ts b/packages/server/src/routes/spaces.ts index b586fd7e..4bdbc355 100644 --- a/packages/server/src/routes/spaces.ts +++ b/packages/server/src/routes/spaces.ts @@ -508,8 +508,12 @@ export async function spaceRoutes(app: FastifyInstance): Promise { const spaceIcon = server.icon; const spaceBanner = server.banner; - // Delete all channels (messages cascade), members, folder refs, then space atomically + // Delete all channels (messages cascade), members, folder refs, read states, then space atomically db.transaction((tx) => { + // Clean up read_states for all channels in this space (no FK cascade — channelId is plain text) + if (channelIds.length > 0) { + tx.delete(schema.readStates).where(inArray(schema.readStates.channelId, channelIds)).run(); + } tx.delete(schema.channels).where(eq(schema.channels.spaceId, id)).run(); tx.delete(schema.spaceMembers).where(eq(schema.spaceMembers.spaceId, id)).run(); tx.delete(schema.spaceFolderMembers).where(eq(schema.spaceFolderMembers.spaceId, id)).run(); @@ -941,6 +945,16 @@ export async function spaceRoutes(app: FastifyInstance): Promise { ) ).run(); + // Clean up read_states for the departing user in this space's channels + const spaceChannelIds = db.select({ id: schema.channels.id }) + .from(schema.channels).where(eq(schema.channels.spaceId, id)).all().map(c => c.id); + if (spaceChannelIds.length > 0) { + db.delete(schema.readStates).where(and( + eq(schema.readStates.userId, uid), + inArray(schema.readStates.channelId, spaceChannelIds), + )).run(); + } + // Broadcast member_left event connectionManager.sendToSpace(id, { type: 'member_left', @@ -1269,6 +1283,10 @@ export async function spaceRoutes(app: FastifyInstance): Promise { const now = Date.now(); + // Fetch channel IDs before the transaction for read_states cleanup + const banChannelIds = db.select({ id: schema.channels.id }) + .from(schema.channels).where(eq(schema.channels.spaceId, id)).all().map(c => c.id); + db.transaction((tx) => { // Insert ban record tx.insert(schema.bans).values({ @@ -1291,6 +1309,14 @@ export async function spaceRoutes(app: FastifyInstance): Promise { eq(schema.memberRoles.userId, targetId), )).run(); + // Clean up read_states for the banned user in this space's channels + if (banChannelIds.length > 0) { + tx.delete(schema.readStates).where(and( + eq(schema.readStates.userId, targetId), + inArray(schema.readStates.channelId, banChannelIds), + )).run(); + } + // Clean up any voice restrictions for the banned member tx.delete(schema.voiceRestrictions).where(and( eq(schema.voiceRestrictions.spaceId, id), diff --git a/packages/server/src/ws/events.ts b/packages/server/src/ws/events.ts index eaa5df5d..f14cf87c 100644 --- a/packages/server/src/ws/events.ts +++ b/packages/server/src/ws/events.ts @@ -1040,6 +1040,14 @@ function handleChannelAck(event: Record, userId: string): void // Validate messageId is a valid snowflake (numeric string) — reject temp/garbage IDs if (!/^\d+$/.test(messageId)) return; + // Validate channel membership — reject acks for channels the user doesn't belong to + const spaceId = getChannelSpaceId(channelId); + if (spaceId) { + if (!isMember(spaceId, userId)) return; + } else { + if (!isDmMember(channelId, userId)) return; + } + const db = getDb(); const existing = db.select() diff --git a/packages/web/src/components/chat/MessageList.tsx b/packages/web/src/components/chat/MessageList.tsx index 979e5c97..39e0fe65 100644 --- a/packages/web/src/components/chat/MessageList.tsx +++ b/packages/web/src/components/chat/MessageList.tsx @@ -69,6 +69,9 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess } }, [channelId, loadMessages, canReadHistory]); + // Track the last message ID so the ack re-fires when a temp message is replaced by its server-confirmed ID + const lastMessageId = messages.length > 0 ? messages[messages.length - 1]?.id ?? '' : ''; + // Ack channel when messages load or when new messages arrive while near bottom useEffect(() => { if (messages.length > 0 && isNearBottom) { @@ -76,7 +79,7 @@ export function MessageList({ channelId, jumpToMessageId, onJumpComplete }: Mess ackTimerRef.current = setTimeout(() => ackChannel(channelId), 200); } return () => clearTimeout(ackTimerRef.current); - }, [channelId, messages.length, isNearBottom, ackChannel]); + }, [channelId, messages.length, lastMessageId, isNearBottom, ackChannel]); // Reset scroll tracking on channel switch so initial-load scroll fires useEffect(() => { diff --git a/packages/web/src/hooks/useWebSocket.ts b/packages/web/src/hooks/useWebSocket.ts index 515ab6da..a99fb0cd 100644 --- a/packages/web/src/hooks/useWebSocket.ts +++ b/packages/web/src/hooks/useWebSocket.ts @@ -284,7 +284,8 @@ function handleEvent(origin: string, event: ServerEvent): void { addRealtimeMessage(event.message.channelId, event.message); { const { currentChannelId, markChannelUnread } = useChatStore.getState(); - if (event.message.channelId !== currentChannelId) { + const myId = isHome ? useAuthStore.getState().user?.id : getMyUserIdForOrigin(origin); + if (event.message.channelId !== currentChannelId && event.message.userId !== myId) { markChannelUnread(event.message.channelId); } } @@ -440,7 +441,8 @@ function handleEvent(origin: string, event: ServerEvent): void { } { const { currentChannelId, markChannelUnread } = useChatStore.getState(); - if (event.message.dmChannelId !== currentChannelId) { + const myId = isHome ? useAuthStore.getState().user?.id : getMyUserIdForOrigin(origin); + if (event.message.dmChannelId !== currentChannelId && event.message.userId !== myId) { markChannelUnread(event.message.dmChannelId); } } diff --git a/packages/web/src/stores/chatStore.ts b/packages/web/src/stores/chatStore.ts index c1bc211f..cb564cc0 100644 --- a/packages/web/src/stores/chatStore.ts +++ b/packages/web/src/stores/chatStore.ts @@ -57,6 +57,7 @@ interface ChatState { markChannelUnread: (channelId: string) => void; ackChannel: (channelId: string) => void; onChannelAck: (channelId: string, messageId: string) => void; + removeChannelStates: (channelIds: Set) => void; updateUserInMessages: (user: { id: string; [key: string]: any }) => void; } @@ -565,16 +566,22 @@ export const useChatStore = create((set, get) => ({ ackChannel: (channelId: string) => { const msgs = get().messages.get(channelId); if (!msgs || msgs.length === 0) return; - const lastMsg = msgs[msgs.length - 1]; - if (!lastMsg) return; - const messageId = lastMsg.id; - // Don't ack optimistic/temp messages — wait for the real server ID - if (messageId.startsWith('temp_')) return; + + // Walk backward to find the last server-confirmed (non-temp) message + let messageId: string | null = null; + for (let i = msgs.length - 1; i >= 0; i--) { + const msg = msgs[i]; + if (msg && !msg.id.startsWith('temp_')) { + messageId = msg.id; + break; + } + } + if (!messageId) return; // All messages are temp — nothing to ack yet // Update local state immediately set((state) => { const newReadStates = new Map(state.readStates); - newReadStates.set(channelId, messageId); + newReadStates.set(channelId, messageId!); const newUnread = new Set(state.unreadChannels); newUnread.delete(channelId); return { readStates: newReadStates, unreadChannels: newUnread }; @@ -595,6 +602,21 @@ export const useChatStore = create((set, get) => ({ }); }, + removeChannelStates: (channelIds: Set) => { + if (channelIds.size === 0) return; + set((state) => { + const newUnread = new Set(state.unreadChannels); + const newReadStates = new Map(state.readStates); + const newMessages = new Map(state.messages); + for (const channelId of channelIds) { + newUnread.delete(channelId); + newReadStates.delete(channelId); + newMessages.delete(channelId); + } + return { unreadChannels: newUnread, readStates: newReadStates, messages: newMessages }; + }); + }, + updateUserInMessages: (user: { id: string; homeUserId?: string | null; [key: string]: any }) => { set((state) => { const newMessages = new Map(state.messages); diff --git a/packages/web/src/stores/spaceStore.ts b/packages/web/src/stores/spaceStore.ts index cbb02cf1..397e4543 100644 --- a/packages/web/src/stores/spaceStore.ts +++ b/packages/web/src/stores/spaceStore.ts @@ -4,6 +4,7 @@ import { api, BackspaceApiClient } from '../api/client'; import { resolveAssetUrl, normalizeUserAssets } from '../utils/assetUrls'; import { isSelf } from '../utils/identity'; import { useAuthStore } from './authStore'; +import { useChatStore } from './chatStore'; // ─── Instance-aware types ───────────────────────────────────────────────────── @@ -147,9 +148,13 @@ export const useSpaceStore = create((set, get) => ({ }; }), - removeDmChannel: (id) => set((state) => ({ - dmChannels: state.dmChannels.filter(c => c.id !== id) - })), + removeDmChannel: (id) => { + set((state) => ({ + dmChannels: state.dmChannels.filter(c => c.id !== id) + })); + // Clean up unread/read state for the closed DM + useChatStore.getState().removeChannelStates(new Set([id])); + }, addDmMember: (dmChannelId, user) => set((state) => ({ dmChannels: state.dmChannels.map(dm => @@ -408,13 +413,14 @@ export const useSpaceStore = create((set, get) => ({ }, removeSpace: (spaceId: string) => { - set((state) => { - // Collect channel IDs belonging to this space for map cleanup - const channelIdsToRemove = new Set(); - for (const [channelId, sid] of state.channelToSpaceMap) { - if (sid === spaceId) channelIdsToRemove.add(channelId); - } + // Collect channel IDs before set() so we can clean up chatStore after + const currentState = get(); + const channelIdsToRemove = new Set(); + for (const [channelId, sid] of currentState.channelToSpaceMap) { + if (sid === spaceId) channelIdsToRemove.add(channelId); + } + set((state) => { const channelToSpaceMap = new Map(state.channelToSpaceMap); const channelPermissions = new Map(state.channelPermissions); const channelOriginMap = new Map(state.channelOriginMap); @@ -439,6 +445,11 @@ export const useSpaceStore = create((set, get) => ({ spacePermissions, }; }); + + // Clean up orphaned unread/read states and cached messages in chatStore + if (channelIdsToRemove.size > 0) { + useChatStore.getState().removeChannelStates(channelIdsToRemove); + } }, updateMemberPresence: (userId: string, status: string) => { @@ -739,6 +750,13 @@ export const useSpaceStore = create((set, get) => ({ }, removeInstanceSpaces: (origin: string) => { + // Collect channel IDs before set() for chatStore cleanup + const currentState = get(); + const channelIdsToRemove = new Set(); + for (const [channelId, chOrigin] of currentState.channelOriginMap) { + if (chOrigin === origin) channelIdsToRemove.add(channelId); + } + set((state) => { const remainingSpaces = state.spaces.filter(s => s._instanceOrigin !== origin); @@ -749,13 +767,11 @@ export const useSpaceStore = create((set, get) => ({ const channelOriginMap = new Map(state.channelOriginMap); const spacePermissions = new Map(state.spacePermissions); - for (const [channelId, chOrigin] of state.channelOriginMap) { - if (chOrigin === origin) { - channelToSpaceMap.delete(channelId); - channelLastMessageIds.delete(channelId); - channelPermissions.delete(channelId); - channelOriginMap.delete(channelId); - } + for (const channelId of channelIdsToRemove) { + channelToSpaceMap.delete(channelId); + channelLastMessageIds.delete(channelId); + channelPermissions.delete(channelId); + channelOriginMap.delete(channelId); } for (const s of state.spaces) { if (s._instanceOrigin === origin) { @@ -775,6 +791,11 @@ export const useSpaceStore = create((set, get) => ({ : null, }; }); + + // Clean up orphaned unread/read states and cached messages in chatStore + if (channelIdsToRemove.size > 0) { + useChatStore.getState().removeChannelStates(channelIdsToRemove); + } }, }));