From 00a2876e9635c4f5c632fecfd97bdb4759bf165f Mon Sep 17 00:00:00 2001 From: Jannis Braun <151788261+TheZwiss@users.noreply.github.com> Date: Thu, 25 Jun 2026 12:34:11 +0200 Subject: [PATCH] fix(channels): newly created channel sometimes hidden until space reopened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sidebar's visibleChannels filter is keyed on the channelPermissions Map. Creating a channel raced two state updates: the optimistic create (added to channels with no permission entry) and the channel_created WS event (the only thing that set the permission). When the optimistic add won the race, the WS handler hit its dedup guard, skipped setChannels, and set the permission by mutating the Map in place — no new reference, so visibleChannels never recomputed and the channel stayed hidden until loadSpace rebuilt the maps (i.e. leaving and returning to the space). Centralize the logic in a new upsertChannel store action that replaces channels and channelPermissions with fresh references, used by both the create path and the channel_created handler. Also return the creator's computed myPermissions (and isPrivate) from POST so the channel renders immediately from the response, independent of WS timing. Adds spaceStore.upsertChannel.test.ts covering the reference-identity regression and the optimistic-reconcile path. --- docs/systems/spaces.md | 4 + packages/server/src/routes/channels.ts | 10 +- packages/web/src/hooks/useWebSocket.ts | 15 +-- packages/web/src/stores/spaceStore.ts | 42 ++++++- .../stores/spaceStore.upsertChannel.test.ts | 111 ++++++++++++++++++ 5 files changed, 163 insertions(+), 19 deletions(-) create mode 100644 packages/web/src/stores/spaceStore.upsertChannel.test.ts diff --git a/docs/systems/spaces.md b/docs/systems/spaces.md index 6dc22029..be64861b 100644 --- a/docs/systems/spaces.md +++ b/docs/systems/spaces.md @@ -444,8 +444,12 @@ Updates `spaces.ownerId`, broadcasts `space_updated` WS event. Position: `max(existing positions) + 1`. +**Response (201):** the created channel including the creator's computed `myPermissions` and `isPrivate: false` — same shape as the `channel_created` event payload — so the creating client can render it immediately without waiting for the broadcast to round-trip. + **Broadcast:** `channel_created` sent per-user (only to users with VIEW_CHANNEL on the new channel). Each user's event includes their computed `myPermissions`. +**Client reconciliation:** both the create response and the `channel_created` event are applied through the `upsertChannel` store action, which replaces `channels` and `channelPermissions` with fresh references. This is required because the sidebar's `visibleChannels` filter is keyed on `channelPermissions`; mutating that Map in place would set the value without triggering a re-render, leaving a freshly created channel hidden until the space was reopened. + ### Update Channel **Endpoint:** `PATCH /api/channels/:id` diff --git a/packages/server/src/routes/channels.ts b/packages/server/src/routes/channels.ts index 98590b67..01dbaae5 100644 --- a/packages/server/src/routes/channels.ts +++ b/packages/server/src/routes/channels.ts @@ -254,7 +254,15 @@ export async function channelRoutes(app: FastifyInstance): Promise { } } - return reply.code(201).send(channelData); + // Return the channel with the creator's computed permissions (same shape as + // the channel_created WS event) so the client can render it immediately + // without waiting for the broadcast to round-trip. + const creatorPerms = computePermissions(request.userId, id, channelId); + return reply.code(201).send({ + ...channelData, + isPrivate: false, + myPermissions: permissionsToString(creatorPerms), + }); }); // PATCH /api/channels/:id - Update a channel (admin+) diff --git a/packages/web/src/hooks/useWebSocket.ts b/packages/web/src/hooks/useWebSocket.ts index 7cd57db3..9e8826d8 100644 --- a/packages/web/src/hooks/useWebSocket.ts +++ b/packages/web/src/hooks/useWebSocket.ts @@ -1138,20 +1138,7 @@ function handleEvent(origin: string, event: ServerEvent): void { // ─── Channel/space events (all origins) ───────────────────────────────── case 'channel_created': { - const { currentSpaceId: curSpaceId, channels: curChannels, setChannels, channelToSpaceMap, channelPermissions, channelOriginMap, voiceChannelIds } = useSpaceStore.getState(); - if (event.spaceId === curSpaceId) { - if (!curChannels.find(c => c.id === event.channel.id)) { - setChannels([...curChannels, event.channel].sort((a, b) => a.position - b.position)); - } - } - channelToSpaceMap.set(event.channel.id, event.spaceId); - channelOriginMap.set(event.channel.id, origin); - if (event.channel.type === 'voice') { - voiceChannelIds.add(event.channel.id); - } - if (event.channel.myPermissions) { - channelPermissions.set(event.channel.id, event.channel.myPermissions); - } + useSpaceStore.getState().upsertChannel(event.channel, event.spaceId, origin); break; } diff --git a/packages/web/src/stores/spaceStore.ts b/packages/web/src/stores/spaceStore.ts index a24923b6..f093250d 100644 --- a/packages/web/src/stores/spaceStore.ts +++ b/packages/web/src/stores/spaceStore.ts @@ -148,6 +148,7 @@ interface SpaceState { joinByCode: (inviteCode: string, origin?: string) => Promise; generateInvite: (spaceId: string) => Promise; createChannel: (spaceId: string, name: string, type: 'text' | 'voice', topic?: string, categoryId?: string) => Promise; + upsertChannel: (channel: Channel, spaceId: string, origin: string) => void; deleteChannel: (channelId: string) => Promise; createCategory: (spaceId: string, name: string) => Promise; updateCategory: (categoryId: string, data: { name?: string; position?: number }) => Promise; @@ -554,13 +555,46 @@ export const useSpaceStore = create((set, get) => ({ const origin = space?._instanceOrigin ?? ''; const client = getApiForOrigin(origin); const channel = await client.channels.create(spaceId, { name, type, topic, categoryId }); - set((state) => { - if (state.channels.some(c => c.id === channel.id)) return state; - return { channels: [...state.channels, channel].sort((a, b) => a.position - b.position) }; - }); + // Reconcile through upsertChannel so the new channel's permission entry is + // written with a fresh map reference (see upsertChannel). The create + // response carries the creator's myPermissions, so the channel renders + // immediately without waiting for the channel_created WS event. + get().upsertChannel(channel, spaceId, origin); return channel; }, + // Add or replace a channel and its derived lookup state. channels and + // channelPermissions are render inputs for the sidebar's visibleChannels memo, + // so they MUST be replaced with fresh references — mutating the existing + // channelPermissions Map in place sets the value but never triggers a + // re-render, which is why a freshly created channel could stay hidden until + // the space was reloaded. + upsertChannel: (channel: Channel, spaceId: string, origin: string) => { + set((state) => { + // Lookup maps are read imperatively (routing/voice), not render inputs — + // in-place mutation matches their usage everywhere else. + state.channelToSpaceMap.set(channel.id, spaceId); + state.channelOriginMap.set(channel.id, origin); + if (channel.type === 'voice') state.voiceChannelIds.add(channel.id); + + const channelPermissions = new Map(state.channelPermissions); + if (channel.myPermissions) { + channelPermissions.set(channel.id, channel.myPermissions); + } + + // channels only holds the currently-open space's list. + if (state.currentSpaceId !== spaceId) { + return { channelPermissions }; + } + const exists = state.channels.some(c => c.id === channel.id); + const channels = (exists + ? state.channels.map(c => (c.id === channel.id ? channel : c)) + : [...state.channels, channel] + ).sort((a, b) => a.position - b.position); + return { channels, channelPermissions }; + }); + }, + deleteChannel: async (channelId: string) => { const origin = get().channelOriginMap.get(channelId) ?? ''; const channelApi = getApiForOrigin(origin); diff --git a/packages/web/src/stores/spaceStore.upsertChannel.test.ts b/packages/web/src/stores/spaceStore.upsertChannel.test.ts new file mode 100644 index 00000000..334879e0 --- /dev/null +++ b/packages/web/src/stores/spaceStore.upsertChannel.test.ts @@ -0,0 +1,111 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +vi.mock('../audio/AudioManager', () => ({ + AudioManager: { + getInstance: vi.fn().mockReturnValue({ + setOutputDevice: vi.fn(), + setVolume: vi.fn(), + }), + }, +})); + +vi.mock('./instanceStore', () => ({ + useInstanceStore: Object.assign( + (selector: (s: unknown) => unknown) => selector({ instances: [], _autoConnectDone: true }), + { + getState: () => ({ instances: [], _autoConnectDone: true }), + setState: vi.fn(), + subscribe: vi.fn(), + } + ), +})); + +vi.mock('./authStore', () => ({ + useAuthStore: Object.assign( + (selector: (s: unknown) => unknown) => selector({ user: null, token: null }), + { + getState: () => ({ user: null, token: null }), + setState: vi.fn(), + subscribe: vi.fn(), + } + ), +})); + +import { useSpaceStore } from './spaceStore'; +import type { Channel } from '@backspace/shared'; + +const SPACE = 'space-1'; + +function makeChannel(extras: Partial & Pick): Channel { + return { + spaceId: SPACE, + name: extras.id, + type: 'text', + topic: null, + position: 0, + categoryId: null, + createdAt: 0, + ...extras, + }; +} + +beforeEach(() => { + useSpaceStore.getState().reset(); + useSpaceStore.setState({ currentSpaceId: SPACE }); +}); + +describe('upsertChannel', () => { + it('adds a new channel to the open space with its permission entry', () => { + const ch = makeChannel({ id: 'c1', position: 3, myPermissions: '1' }); + useSpaceStore.getState().upsertChannel(ch, SPACE, ''); + + const s = useSpaceStore.getState(); + expect(s.channels.map(c => c.id)).toEqual(['c1']); + expect(s.channelPermissions.get('c1')).toBe('1'); + expect(s.channelToSpaceMap.get('c1')).toBe(SPACE); + }); + + it('replaces channelPermissions with a FRESH reference (the re-render trigger)', () => { + // This is the regression guard: an in-place Map mutation sets the value but + // never changes identity, so the visibleChannels memo never recomputes. + const before = useSpaceStore.getState().channelPermissions; + useSpaceStore.getState().upsertChannel(makeChannel({ id: 'c1', myPermissions: '1' }), SPACE, ''); + const after = useSpaceStore.getState().channelPermissions; + + expect(after).not.toBe(before); + expect(after.get('c1')).toBe('1'); + }); + + it('reconciles a channel already added optimistically without a permission entry', () => { + // Simulate the race: optimistic create inserted the channel into `channels` + // but no permission was known yet, so it was filtered out of the sidebar. + useSpaceStore.setState({ channels: [makeChannel({ id: 'c1' })] }); + expect(useSpaceStore.getState().channelPermissions.has('c1')).toBe(false); + + // The channel_created event (or the create response) arrives with perms. + useSpaceStore.getState().upsertChannel(makeChannel({ id: 'c1', myPermissions: '1' }), SPACE, ''); + + const s = useSpaceStore.getState(); + expect(s.channels.filter(c => c.id === 'c1')).toHaveLength(1); // no duplicate + expect(s.channelPermissions.get('c1')).toBe('1'); + }); + + it('keeps voice channels out of the channels list updates but tracks them globally', () => { + useSpaceStore.getState().upsertChannel(makeChannel({ id: 'v1', type: 'voice', myPermissions: '1' }), SPACE, ''); + const s = useSpaceStore.getState(); + expect(s.voiceChannelIds.has('v1')).toBe(true); + expect(s.channels.map(c => c.id)).toContain('v1'); + }); + + it('updates lookup maps but not the channels list for a non-open space', () => { + useSpaceStore.getState().upsertChannel( + makeChannel({ id: 'c2', spaceId: 'space-2', myPermissions: '1' }), + 'space-2', + '', + ); + const s = useSpaceStore.getState(); + expect(s.channels.map(c => c.id)).not.toContain('c2'); // different space, list untouched + expect(s.channelToSpaceMap.get('c2')).toBe('space-2'); + expect(s.channelPermissions.get('c2')).toBe('1'); // perms still tracked + }); +});