Skip to content

Commit 29112c5

Browse files
AhyoungRyuAhyoungRyu
authored
fix: improve error handling in message handlers (#1254)
Fixes some of comments in [SBISSUE-17017](https://sendbird.atlassian.net/browse/SBISSUE-17017?focusedCommentId=298138) ## Problem Users currently need to return an empty object from `onBeforeSendFileMessage` due to type constraints. Additionally, error handling for message sending failures needs improvement to ensure `eventHandlers.message` callbacks (onSendMessageFailed, onUpdateMessageFailed, onFileUploadFailed) are properly triggered when errors occur within their corresponding `onBefore` handlers (`onBeforeSendUserMessage`, `onBeforeSendFileMessage`, etc). ## Solution - Allow void return type in `onBeforeXXX` handler - Add proper error handling via `eventHandlers.message` for: - onSendMessageFailed - onUpdateMessageFailed - onFileUploadFailed ### Checklist Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If unsure, ask the members. This is a reminder of what we look for before merging your code. - [x] **All tests pass locally with my changes** - [x] **I have added tests that prove my fix is effective or that my feature works** - [ ] **Public components / utils / props are appropriately exported** - [ ] I have added necessary documentation (if appropriate)
1 parent 55846b0 commit 29112c5

File tree

5 files changed

+253
-19
lines changed

5 files changed

+253
-19
lines changed

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,7 @@
102102
"@svgr/rollup": "^8.1.0",
103103
"@testing-library/jest-dom": "^5.16.5",
104104
"@testing-library/react": "^13.4.0",
105+
"@testing-library/react-hooks": "^8.0.1",
105106
"@testing-library/user-event": "^14.4.3",
106107
"@types/node": "^22.7.2",
107108
"@typescript-eslint/eslint-plugin": "^6.17.0",

‎src/modules/GroupChannel/context/GroupChannelProvider.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import { getIsReactionEnabled } from '../../../utils/getIsReactionEnabled';
2929

3030
export { ThreadReplySelectType } from './const'; // export for external usage
3131

32-
type OnBeforeHandler<T> = (params: T) => T | Promise<T>;
32+
export type OnBeforeHandler<T> = (params: T) => T | Promise<T> | void | Promise<void>;
3333
type MessageListQueryParamsType = Omit<MessageCollectionParams, 'filter'> & MessageFilterParams;
3434
type MessageActions = ReturnType<typeof useMessageActions>;
3535
type MessageListDataSourceWithoutActions = Omit<ReturnType<typeof useGroupChannelMessages>, keyof MessageActions | `_dangerous_${string}`>;

‎src/modules/GroupChannel/context/__tests__/useMessageActions.spec.ts‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import { renderHook } from '@testing-library/react-hooks';
2+
import { useMessageActions } from '../hooks/useMessageActions';
3+
import { UserMessageCreateParams, FileMessageCreateParams } from '@sendbird/chat/message';
4+
5+
const mockEventHandlers = {
6+
message: {
7+
onSendMessageFailed: jest.fn(),
8+
onUpdateMessageFailed: jest.fn(),
9+
onFileUploadFailed: jest.fn(),
10+
},
11+
};
12+
13+
jest.mock('../../../../hooks/useSendbirdStateContext', () => ({
14+
__esModule: true,
15+
default: () => ({
16+
eventHandlers: mockEventHandlers,
17+
}),
18+
}));
19+
20+
describe('useMessageActions', () => {
21+
const mockParams = {
22+
sendUserMessage: jest.fn(),
23+
sendFileMessage: jest.fn(),
24+
sendMultipleFilesMessage: jest.fn(),
25+
updateUserMessage: jest.fn(),
26+
scrollToBottom: jest.fn(),
27+
replyType: 'NONE',
28+
};
29+
30+
beforeEach(() => {
31+
jest.clearAllMocks();
32+
});
33+
34+
describe('processParams', () => {
35+
it('should handle successful user message', async () => {
36+
const { result } = renderHook(() => useMessageActions(mockParams));
37+
const params: UserMessageCreateParams = { message: 'test' };
38+
39+
await result.current.sendUserMessage(params);
40+
41+
expect(mockParams.sendUserMessage).toHaveBeenCalledWith(
42+
expect.objectContaining({ message: 'test' }),
43+
expect.any(Function),
44+
);
45+
});
46+
47+
it('should handle void return from onBeforeSendFileMessage', async () => {
48+
const onBeforeSendFileMessage = jest.fn();
49+
const { result } = renderHook(() => useMessageActions({
50+
...mockParams,
51+
onBeforeSendFileMessage,
52+
}),
53+
);
54+
55+
const fileParams: FileMessageCreateParams = {
56+
file: new File([], 'test.txt'),
57+
};
58+
59+
await result.current.sendFileMessage(fileParams);
60+
61+
expect(onBeforeSendFileMessage).toHaveBeenCalled();
62+
expect(mockParams.sendFileMessage).toHaveBeenCalledWith(
63+
expect.objectContaining(fileParams),
64+
expect.any(Function),
65+
);
66+
});
67+
68+
it('should handle file upload error', async () => {
69+
// Arrange
70+
const error = new Error('Upload failed');
71+
const onBeforeSendFileMessage = jest.fn().mockRejectedValue(error);
72+
const fileParams: FileMessageCreateParams = {
73+
file: new File([], 'test.txt'),
74+
fileName: 'test.txt',
75+
};
76+
77+
const { result } = renderHook(() => useMessageActions({
78+
...mockParams,
79+
onBeforeSendFileMessage,
80+
}),
81+
);
82+
83+
await expect(async () => {
84+
await result.current.sendFileMessage(fileParams);
85+
}).rejects.toThrow('Upload failed');
86+
87+
// Wait for next tick to ensure all promises are resolved
88+
await new Promise(process.nextTick);
89+
90+
expect(onBeforeSendFileMessage).toHaveBeenCalled();
91+
expect(mockEventHandlers.message.onFileUploadFailed).toHaveBeenCalledWith(error);
92+
expect(mockEventHandlers.message.onSendMessageFailed).toHaveBeenCalledWith(
93+
expect.objectContaining({
94+
file: fileParams.file,
95+
fileName: fileParams.fileName,
96+
}),
97+
error,
98+
);
99+
});
100+
101+
it('should handle message update error', async () => {
102+
// Arrange
103+
const error = new Error('Update failed');
104+
const onBeforeUpdateUserMessage = jest.fn().mockRejectedValue(error);
105+
const messageParams = {
106+
messageId: 1,
107+
message: 'update message',
108+
};
109+
110+
const { result } = renderHook(() => useMessageActions({
111+
...mockParams,
112+
onBeforeUpdateUserMessage,
113+
}),
114+
);
115+
116+
await expect(async () => {
117+
await result.current.updateUserMessage(messageParams.messageId, {
118+
message: messageParams.message,
119+
});
120+
}).rejects.toThrow('Update failed');
121+
122+
// Wait for next tick to ensure all promises are resolved
123+
await new Promise(process.nextTick);
124+
125+
expect(onBeforeUpdateUserMessage).toHaveBeenCalled();
126+
expect(mockEventHandlers.message.onUpdateMessageFailed).toHaveBeenCalledWith(
127+
expect.objectContaining({
128+
message: messageParams.message,
129+
}),
130+
error,
131+
);
132+
});
133+
134+
it('should preserve modified params from onBefore handlers', async () => {
135+
const onBeforeSendUserMessage = jest.fn().mockImplementation((params) => ({
136+
...params,
137+
message: 'modified',
138+
}));
139+
140+
const { result } = renderHook(() => useMessageActions({
141+
...mockParams,
142+
onBeforeSendUserMessage,
143+
}),
144+
);
145+
146+
await result.current.sendUserMessage({ message: 'original' });
147+
148+
expect(mockParams.sendUserMessage).toHaveBeenCalledWith(
149+
expect.objectContaining({ message: 'modified' }),
150+
expect.any(Function),
151+
);
152+
});
153+
});
154+
});

‎src/modules/GroupChannel/context/hooks/useMessageActions.ts‎

Lines changed: 63 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { match } from 'ts-pattern';
12
import { useCallback } from 'react';
23
import { useGroupChannelMessages } from '@sendbird/uikit-tools';
34
import { MessageMetaArray } from '@sendbird/chat/message';
@@ -19,9 +20,10 @@ import {
1920
VOICE_MESSAGE_FILE_NAME,
2021
VOICE_MESSAGE_MIME_TYPE,
2122
} from '../../../../utils/consts';
22-
import type { SendableMessageType } from '../../../../utils';
23+
import type { SendableMessageType, CoreMessageType } from '../../../../utils';
2324
import type { ReplyType } from '../../../../types';
24-
import type { GroupChannelProviderProps } from '../GroupChannelProvider';
25+
import type { GroupChannelProviderProps, OnBeforeHandler } from '../GroupChannelProvider';
26+
import useSendbirdStateContext from '../../../../hooks/useSendbirdStateContext';
2527

2628
type MessageListDataSource = ReturnType<typeof useGroupChannelMessages>;
2729
type MessageActions = {
@@ -39,6 +41,13 @@ interface Params extends GroupChannelProviderProps, MessageListDataSource {
3941
}
4042

4143
const pass = <T>(value: T) => value;
44+
type MessageParamsByType = {
45+
user: UserMessageCreateParams;
46+
file: FileMessageCreateParams;
47+
multipleFiles: MultipleFilesMessageCreateParams;
48+
voice: FileMessageCreateParams;
49+
update: UserMessageUpdateParams;
50+
};
4251

4352
/**
4453
* @description This hook controls common processes related to message sending, updating.
@@ -60,7 +69,7 @@ export function useMessageActions(params: Params): MessageActions {
6069
quoteMessage,
6170
replyType,
6271
} = params;
63-
72+
const { eventHandlers } = useSendbirdStateContext();
6473
const buildInternalMessageParams = useCallback(
6574
<T extends BaseMessageCreateParams>(basicParams: T): T => {
6675
const messageParams = { ...basicParams } as T;
@@ -84,33 +93,71 @@ export function useMessageActions(params: Params): MessageActions {
8493
[],
8594
);
8695

96+
const processParams = useCallback(async <T extends keyof MessageParamsByType>(
97+
handler: OnBeforeHandler<MessageParamsByType[T]>,
98+
params: ReturnType<typeof buildInternalMessageParams>,
99+
type: keyof MessageParamsByType,
100+
): Promise<MessageParamsByType[T]> => {
101+
try {
102+
const result = await handler(params as MessageParamsByType[T]);
103+
return (result === undefined ? params : result) as MessageParamsByType[T];
104+
} catch (error) {
105+
if (typeof eventHandlers?.message === 'object') {
106+
match(type)
107+
.with('file', 'voice', () => {
108+
if ((params as any).file) {
109+
eventHandlers.message.onFileUploadFailed?.(error);
110+
}
111+
eventHandlers.message.onSendMessageFailed?.(params as CoreMessageType, error);
112+
})
113+
.with('multipleFiles', () => {
114+
if ((params as MultipleFilesMessageCreateParams).fileInfoList) {
115+
eventHandlers.message.onFileUploadFailed?.(error);
116+
}
117+
eventHandlers.message.onSendMessageFailed?.(params as CoreMessageType, error);
118+
})
119+
.with('user', () => {
120+
eventHandlers.message.onSendMessageFailed?.(
121+
params as CoreMessageType,
122+
error,
123+
);
124+
})
125+
.with('update', () => {
126+
eventHandlers.message.onUpdateMessageFailed?.(
127+
params as CoreMessageType,
128+
error,
129+
);
130+
})
131+
.exhaustive();
132+
}
133+
throw error;
134+
}
135+
}, [eventHandlers]);
136+
87137
return {
88138
sendUserMessage: useCallback(
89139
async (params) => {
90140
const internalParams = buildInternalMessageParams<UserMessageCreateParams>(params);
91-
const processedParams = await onBeforeSendUserMessage(internalParams);
92-
141+
const processedParams = await processParams(onBeforeSendUserMessage, internalParams, 'user') as UserMessageCreateParams;
93142
return sendUserMessage(processedParams, asyncScrollToBottom);
94143
},
95-
[buildInternalMessageParams, sendUserMessage, scrollToBottom],
144+
[buildInternalMessageParams, sendUserMessage, scrollToBottom, processParams],
96145
),
97146
sendFileMessage: useCallback(
98147
async (params) => {
99148
const internalParams = buildInternalMessageParams<FileMessageCreateParams>(params);
100-
const processedParams = await onBeforeSendFileMessage(internalParams);
101-
149+
const processedParams = await processParams(onBeforeSendFileMessage, internalParams, 'file') as FileMessageCreateParams;
102150
return sendFileMessage(processedParams, asyncScrollToBottom);
103151
},
104-
[buildInternalMessageParams, sendFileMessage, scrollToBottom],
152+
[buildInternalMessageParams, sendFileMessage, scrollToBottom, processParams],
105153
),
106154
sendMultipleFilesMessage: useCallback(
107155
async (params) => {
108156
const internalParams = buildInternalMessageParams<MultipleFilesMessageCreateParams>(params);
109-
const processedParams = await onBeforeSendMultipleFilesMessage(internalParams);
110-
157+
const processedParams = await processParams(onBeforeSendMultipleFilesMessage, internalParams, 'multipleFiles') as MultipleFilesMessageCreateParams;
111158
return sendMultipleFilesMessage(processedParams, asyncScrollToBottom);
112159
},
113-
[buildInternalMessageParams, sendMultipleFilesMessage, scrollToBottom],
160+
[buildInternalMessageParams, sendMultipleFilesMessage, scrollToBottom, processParams],
114161
),
115162
sendVoiceMessage: useCallback(
116163
async (params: FileMessageCreateParams, duration: number) => {
@@ -129,20 +176,18 @@ export function useMessageActions(params: Params): MessageActions {
129176
}),
130177
],
131178
});
132-
const processedParams = await onBeforeSendVoiceMessage(internalParams);
133-
179+
const processedParams = await processParams(onBeforeSendVoiceMessage, internalParams, 'voice');
134180
return sendFileMessage(processedParams, asyncScrollToBottom);
135181
},
136-
[buildInternalMessageParams, sendFileMessage, scrollToBottom],
182+
[buildInternalMessageParams, sendFileMessage, scrollToBottom, processParams],
137183
),
138184
updateUserMessage: useCallback(
139185
async (messageId: number, params: UserMessageUpdateParams) => {
140186
const internalParams = buildInternalMessageParams<UserMessageUpdateParams>(params);
141-
const processedParams = await onBeforeUpdateUserMessage(internalParams);
142-
187+
const processedParams = await processParams(onBeforeUpdateUserMessage, internalParams, 'update');
143188
return updateUserMessage(messageId, processedParams);
144189
},
145-
[buildInternalMessageParams, updateUserMessage],
190+
[buildInternalMessageParams, updateUserMessage, processParams],
146191
),
147192
};
148193
}

‎yarn.lock‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2778,6 +2778,7 @@ __metadata:
27782778
"@svgr/rollup": ^8.1.0
27792779
"@testing-library/jest-dom": ^5.16.5
27802780
"@testing-library/react": ^13.4.0
2781+
"@testing-library/react-hooks": ^8.0.1
27812782
"@testing-library/user-event": ^14.4.3
27822783
"@types/node": ^22.7.2
27832784
"@typescript-eslint/eslint-plugin": ^6.17.0
@@ -3468,6 +3469,28 @@ __metadata:
34683469
languageName: node
34693470
linkType: hard
34703471

3472+
"@testing-library/react-hooks@npm:^8.0.1":
3473+
version: 8.0.1
3474+
resolution: "@testing-library/react-hooks@npm:8.0.1"
3475+
dependencies:
3476+
"@babel/runtime": ^7.12.5
3477+
react-error-boundary: ^3.1.0
3478+
peerDependencies:
3479+
"@types/react": ^16.9.0 || ^17.0.0
3480+
react: ^16.9.0 || ^17.0.0
3481+
react-dom: ^16.9.0 || ^17.0.0
3482+
react-test-renderer: ^16.9.0 || ^17.0.0
3483+
peerDependenciesMeta:
3484+
"@types/react":
3485+
optional: true
3486+
react-dom:
3487+
optional: true
3488+
react-test-renderer:
3489+
optional: true
3490+
checksum: 7fe44352e920deb5cb1876f80d64e48615232072c9d5382f1e0284b3aab46bb1c659a040b774c45cdf084a5257b8fe463f7e08695ad8480d8a15635d4d3d1f6d
3491+
languageName: node
3492+
linkType: hard
3493+
34713494
"@testing-library/react@npm:^13.4.0":
34723495
version: 13.4.0
34733496
resolution: "@testing-library/react@npm:13.4.0"
@@ -12532,6 +12555,17 @@ __metadata:
1253212555
languageName: node
1253312556
linkType: hard
1253412557

12558+
"react-error-boundary@npm:^3.1.0":
12559+
version: 3.1.4
12560+
resolution: "react-error-boundary@npm:3.1.4"
12561+
dependencies:
12562+
"@babel/runtime": ^7.12.5
12563+
peerDependencies:
12564+
react: ">=16.13.1"
12565+
checksum: f36270a5d775a25c8920f854c0d91649ceea417b15b5bc51e270a959b0476647bb79abb4da3be7dd9a4597b029214e8fe43ea914a7f16fa7543c91f784977f1b
12566+
languageName: node
12567+
linkType: hard
12568+
1253512569
"react-is@npm:18.1.0":
1253612570
version: 18.1.0
1253712571
resolution: "react-is@npm:18.1.0"

0 commit comments

Comments
 (0)