Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/components/Channel/Channel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ import {
} from './utils';
import { useThreadContext } from '../Threads';
import { getChannel } from '../../utils';
import { getChannelConfig } from '../../utils/getChannelConfig';
import type {
ChannelUnreadUiState,
ImageAttachmentSizeHandler,
Expand Down Expand Up @@ -248,7 +249,7 @@ const ChannelInner = (
const windowsEmojiClass = useImageFlagEmojisOnWindowsClass();
const thread = useThreadContext();

const [channelConfig, setChannelConfig] = useState(channel.getConfig());
const [channelConfig, setChannelConfig] = useState(() => getChannelConfig(channel));

const [channelUnreadUiState, _setChannelUnreadUiState] =
useState<ChannelUnreadUiState>();
Expand Down Expand Up @@ -357,6 +358,11 @@ const ChannelInner = (
);

const handleEvent = async (event: Event) => {
// Client-level subscriptions keep delivering events after the channel has
// been disconnected (current user removed / channel deleted). Reading from
// or querying such a channel throws, so there is nothing useful left to do.
if (channel.disconnected) return;

if (event.message) {
dispatch({
channel,
Expand Down Expand Up @@ -660,6 +666,7 @@ const ChannelInner = (

const loadMoreNewer = async (limit = DEFAULT_NEXT_CHANNEL_PAGE_SIZE) => {
if (
channel.disconnected ||
!online.current ||
!window.navigator.onLine ||
!channel.state.messagePagination.hasNext
Expand Down
109 changes: 108 additions & 1 deletion src/components/Channel/__tests__/Channel.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { fromPartial } from '@total-typescript/shoehorn';
import { nanoid } from 'nanoid';
import React, { useEffect } from 'react';
import React, { useEffect, useState } from 'react';
import { ErrorFromResponse, SearchController } from 'stream-chat';
import type {
ChannelAPIResponse,
Channel as ChannelType,
Event,
GiphyVersions,
LocalMessage,
Message,
MessageResponse,
Expand Down Expand Up @@ -823,6 +826,110 @@ describe('Channel', () => {

expect(querySpy).not.toHaveBeenCalled();
});

it('does not paginate newer (query) when the client is disconnected', async () => {
let loadMoreNewer: ChannelActionContextValue['loadMoreNewer'] | undefined;
await renderComponent(
{ channel, channelQueryOptions: { messages: { limit: 25 } }, chatClient },
(c) => {
loadMoreNewer = c.loadMoreNewer;
},
);

// loadMoreNewer bails out early unless there is a newer page to fetch
channel.state.messagePagination.hasNext = true;

const querySpy = vi.spyOn(channel, 'query');
channel.disconnected = true;

await act(async () => {
await loadMoreNewer?.();
});

expect(querySpy).not.toHaveBeenCalled();
});

it('does not throw during render when the channel is disconnected (#3254)', async () => {
// initClient stubs channel.getConfig; restore the real implementation so
// that the disconnect guard inside channel.getClient() is reachable
vi.mocked(channel.getConfig).mockRestore();

let channelConfig: ChannelStateContextValue['channelConfig'] | 'unset' = 'unset';
const ConfigProbe = () => {
channelConfig = useChannelStateContext().channelConfig;
return <div>probe</div>;
};

let setGiphyVersion: (version: GiphyVersions) => void = () => {};
const Wrapper = () => {
const [giphyVersion, _setGiphyVersion] = useState<GiphyVersions>('fixed_height');
setGiphyVersion = _setGiphyVersion;
return (
<Chat client={chatClient}>
<Channel channel={channel} giphyVersion={giphyVersion}>
<ConfigProbe />
</Channel>
</Chat>
);
};

await act(() => {
render(<Wrapper />);
});
await waitFor(() => expect(screen.getByText('probe')).toBeInTheDocument());

// the channel is mounted and initialized; it then gets disconnected, as it
// would be by channel.deleted / notification.removed_from_channel
channel.disconnected = true;

// changing a Channel prop bypasses React.memo and re-renders ChannelInner
expect(() =>
act(() => {
setGiphyVersion('original');
}),
).not.toThrow();

// the subtree survives, and the config captured while connected is retained
expect(screen.getByText('probe')).toBeInTheDocument();
expect(channelConfig).toEqual(expect.objectContaining({ read_events: true }));
});

it('provides an undefined channelConfig when mounting an already disconnected channel (#3254)', async () => {
// the channel must already be initialized, otherwise Channel tries to query
// it on mount and legitimately ends up in its error state instead
await channel.watch();
vi.mocked(channel.getConfig).mockRestore();
channel.disconnected = true;

let channelConfig: ChannelStateContextValue['channelConfig'] | 'unset' = 'unset';
const ConfigProbe = () => {
channelConfig = useChannelStateContext().channelConfig;
return <div>probe</div>;
};

await renderComponent({ channel, chatClient, children: <ConfigProbe /> });

await waitFor(() => expect(screen.getByText('probe')).toBeInTheDocument());
expect(channelConfig).toBeUndefined();
});

it('does not query a disconnected channel on user.deleted (#3254)', async () => {
await renderComponent({ channel, chatClient });

const querySpy = vi
.spyOn(channel, 'query')
.mockResolvedValue(fromPartial<ChannelAPIResponse>({}));
channel.disconnected = true;

// client-level subscriptions keep delivering events to the mounted Channel
// even after stream-chat drops the channel from client.activeChannels
await act(async () => {
chatClient.dispatchEvent(fromPartial<Event>({ type: 'user.deleted' }));
await Promise.resolve();
});

expect(querySpy).not.toHaveBeenCalled();
});
});

describe('Children that consume the contexts set in Channel', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
AttachmentSelectorContextProvider,
useAttachmentSelectorContext,
} from '../../../context/AttachmentSelectorContext';
import { getChannelConfig } from '../../../utils/getChannelConfig';
import { useStableId } from '../../UtilityComponents/useStableId';
import { useInertWhenHidden } from '../../Accessibility';
import { useStateStore } from '../../../store';
Expand Down Expand Up @@ -283,7 +284,7 @@ const useAttachmentSelectorActionsFiltered = (original: AttachmentSelectorAction
const { channelCapabilities } = useChannelStateContext();
const { isUploadEnabled } = useAttachmentManagerState();
const messageComposer = useMessageComposerController();
const channelConfig = messageComposer.channel.getConfig();
const channelConfig = getChannelConfig(messageComposer.channel);

return useMemo(
() =>
Expand Down
7 changes: 7 additions & 0 deletions src/components/MessageComposer/MessageComposer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,13 @@ const MessageComposerProvider = (props: PropsWithChildren<MessageComposerProps>)

useEffect(
() => () => {
// A disconnected channel (current user removed / channel deleted) cannot
// accept a draft, and neither createDraft() nor clear() are safe to call:
// both reach channel.getConfig(), which throws "You can't use a channel
// after client.disconnect() was called". The composer is going away with
// the channel, so there is nothing left worth persisting or resetting.
if (messageComposer.channel.disconnected) return;

messageComposer.createDraft().finally(() => messageComposer.clear());
Comment on lines +98 to 105

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚑ Quick win

Recheck disconnection before clear().

Line 103 only protects the start of cleanup. A WebSocket event can disconnect the channel while createDraft() is pending. The finally callback then calls messageComposer.clear(), which reaches channel.getConfig() after client.disconnect() and can throw.

Recheck messageComposer.channel.disconnected inside finally. Add a test that leaves createDraft() pending, unmounts, disconnects the channel, resolves the draft promise, and verifies that clear() is not called.

Proposed fix
-      messageComposer.createDraft().finally(() => messageComposer.clear());
+      void messageComposer.createDraft().finally(() => {
+        if (!messageComposer.channel.disconnected) {
+          messageComposer.clear();
+        }
+      });
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// A disconnected channel (current user removed / channel deleted) cannot
// accept a draft, and neither createDraft() nor clear() are safe to call:
// both reach channel.getConfig(), which throws "You can't use a channel
// after client.disconnect() was called". The composer is going away with
// the channel, so there is nothing left worth persisting or resetting.
if (messageComposer.channel.disconnected) return;
messageComposer.createDraft().finally(() => messageComposer.clear());
// A disconnected channel (current user removed / channel deleted) cannot
// accept a draft, and neither createDraft() nor clear() are safe to call:
// both reach channel.getConfig(), which throws "You can't use a channel
// after client.disconnect() was called". The composer is going away with
// the channel, so there is nothing left worth persisting or resetting.
if (messageComposer.channel.disconnected) return;
void messageComposer.createDraft().finally(() => {
if (!messageComposer.channel.disconnected) {
messageComposer.clear();
}
});
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/MessageComposer/MessageComposer.tsx` around lines 98 - 105,
Recheck messageComposer.channel.disconnected inside the finally callback before
calling messageComposer.clear(), while preserving the existing early return
before createDraft(). Add a test covering an unresolved createDraft(),
unmounting and disconnecting the channel, then resolving the draft and verifying
clear() is not called.

},
[messageComposer],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,27 @@ describe('AttachmentSelector', () => {
expect(screen.getByTestId(SHARE_LOCATION_DIALOG_TEST_ID)).toBeInTheDocument();
});
});

it('does not throw when the channel disconnects while mounted (#3254)', async () => {
const { channel } = await renderComponent();

// initClientWithChannels stubs channel.getConfig; restore the real
// implementation so that the disconnect guard in getClient() is reachable
vi.mocked(channel.getConfig).mockRestore();
channel.disconnected = true;

// opening the menu re-renders the selector, which re-reads the channel config
await expect(invokeMenu()).resolves.toBeUndefined();

// no config means no available actions, so the selector renders nothing
// instead of tearing down the surrounding subtree
expect(
screen.queryByTestId(SIMPLE_ATTACHMENT_SELECTOR_TEST_ID),
).not.toBeInTheDocument();
expect(
screen.queryByTestId(ATTACHMENT_SELECTOR__ACTIONS_MENU_TEST_ID),
).not.toBeInTheDocument();
});
});

const AttachmentSelectorInitiationButtonContents = () => (
Expand Down
17 changes: 17 additions & 0 deletions src/components/MessageComposer/__tests__/MessageInput.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2130,3 +2130,20 @@ describe(`MessageInputFlat`, () => {
});
});
});

describe('MessageComposer draft creation on unmount', () => {
afterEach(tearDown);

it('does not create a draft for a disconnected channel (#3254)', async () => {
const { channel, unmount } = await renderComponent();
const createDraftSpy = vi.spyOn(channel!.messageComposer, 'createDraft');

channel!.disconnected = true;

await act(() => {
unmount();
});

expect(createDraftSpy).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,17 @@ describe('useMessageComposerCommands', () => {
{ command: expect.objectContaining({ name: 'ban' }), enabled: false },
]);
});
it('returns no commands for a disconnected channel without calling getConfig (#3254)', () => {
// channel.getConfig() calls channel.getClient(), which throws once the
// channel is disconnected
vi.spyOn(messageComposer.channel, 'getConfig').mockImplementation(() => {
throw new Error("You can't use a channel after client.disconnect() was called");
});
(messageComposer.channel as { disconnected?: boolean }).disconnected = true;

const { result } = renderHook(() => useMessageComposerCommands());

expect(result.current).toEqual([]);
expect(messageComposer.channel.getConfig).not.toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { useMemo } from 'react';
import type { CommandResponse, MessageComposerState } from 'stream-chat';

import { useStateStore } from '../../../store';
import { getChannelConfig } from '../../../utils/getChannelConfig';
import { useMessageComposerController } from './useMessageComposerController';

const messageComposerStateSelector = ({
Expand All @@ -19,7 +20,7 @@ export type MessageComposerCommand = {

export const useMessageComposerCommands = () => {
const messageComposer = useMessageComposerController();
const channelConfig = messageComposer.channel.getConfig();
const channelConfig = getChannelConfig(messageComposer.channel);
const { editedMessage, quotedMessage } = useStateStore(
messageComposer.state,
messageComposerStateSelector,
Expand Down
21 changes: 21 additions & 0 deletions src/components/MessageList/hooks/__tests__/useMarkRead.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -834,4 +834,25 @@ describe('useMarkRead', () => {
});
});
});

it('does not throw when the channel is disconnected (#3254)', async () => {
const {
channels: [channel],
client,
} = await initClientWithChannels();
// initClientWithChannels stubs channel.getConfig; restore the real
// implementation so that the disconnect guard in getClient() is reachable
vi.mocked(channel.getConfig).mockRestore();
channel.disconnected = true;

expect(() =>
render({
channel,
client,
params: { isMessageListScrolledToBottom: true, messageListIsThread: false },
}),
).not.toThrow();

expect(markRead).not.toHaveBeenCalled();
});
});
3 changes: 2 additions & 1 deletion src/components/MessageList/hooks/useMarkRead.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
useChatContext,
} from '../../../context';
import type { Channel, Event, LocalMessage, MessageResponse } from 'stream-chat';
import { getChannelConfig } from '../../../utils/getChannelConfig';

const hasReadLastMessage = (channel: Channel, userId: string) => {
const latestMessageIdInChannel = channel.state.latestMessages.slice(-1)[0]?.id;
Expand Down Expand Up @@ -38,7 +39,7 @@ export const useMarkRead = ({

useEffect(() => {
const unreadNotificationSupported =
channel.getConfig()?.read_events || client.options.isLocalUnreadCountEnabled;
getChannelConfig(channel)?.read_events || client.options.isLocalUnreadCountEnabled;

if (!unreadNotificationSupported) return;

Expand Down
29 changes: 29 additions & 0 deletions src/utils/__tests__/getChannelConfig.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { fromPartial } from '@total-typescript/shoehorn';
import type { Channel, ChannelConfigWithInfo } from 'stream-chat';
import { describe, expect, it, vi } from 'vitest';
import { getChannelConfig } from '../getChannelConfig';

const config = fromPartial<ChannelConfigWithInfo>({ read_events: true });

describe('getChannelConfig', () => {
it('returns the channel config for a connected channel', () => {
const channel = fromPartial<Channel>({
disconnected: false,
getConfig: () => config,
});

expect(getChannelConfig(channel)).toBe(config);
});

it('returns undefined for a disconnected channel without calling getConfig', () => {
// channel.getConfig() calls channel.getClient(), which throws
// "You can't use a channel after client.disconnect() was called"
const getConfig = vi.fn(() => {
throw new Error("You can't use a channel after client.disconnect() was called");
});
const channel = fromPartial<Channel>({ disconnected: true, getConfig });

expect(getChannelConfig(channel)).toBeUndefined();
expect(getConfig).not.toHaveBeenCalled();
});
});
19 changes: 19 additions & 0 deletions src/utils/getChannelConfig.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import type { Channel, ChannelConfigWithInfo } from 'stream-chat';

/**
* `channel.getConfig()` calls `channel.getClient()`, which throws
* "You can't use a channel after client.disconnect() was called" once the
* channel is disconnected (e.g. the current user was removed from the channel
* or the channel was deleted - see `channel.deleted`,
* `notification.channel_deleted` and `notification.removed_from_channel`).
*
* The `disconnected` flag is flipped from an asynchronous WS event, so there is
* always a window between the flag becoming true and React unmounting the
* subtree that renders the channel. Any render inside that window would throw,
* so callers must never reach `getConfig()` for a disconnected channel.
*
* `undefined` is already part of `getConfig()`'s return type, so consumers need
* no extra handling beyond what they do for a not-yet-configured channel.
*/
export const getChannelConfig = (channel: Channel): ChannelConfigWithInfo | undefined =>
channel.disconnected ? undefined : channel.getConfig();
Loading