Skip to content

Commit 290d2db

Browse files
authored
fix(Message): throw when a message handler is missing a parameter (#3362)
Ticket: https://linear.app/stream/issue/REACT-1264 > **Stacked on #3360.** The base is `fix/pin-failure-notification-v15`, so the diff shows only this change. Retarget to `master` once #3360 is merged. ### 🎯 Goal Follow-up from the review on #3360 and the comments on REACT-1264. The message action handlers returned early without sending a request when a required parameter was missing, and some only logged a `console.warn`. The default `MessageActions` treat a resolved handler as success, so the action showed "Message deleted", "{{ user }} has been muted" and so on even though nothing happened. This only affects custom messages and handlers, since `Message.tsx` always passes the message. ### 🛠 Implementation details The guards now throw instead of returning: | Handler | Before | Now | | --- | --- | --- | | `useMuteHandler` | `console.warn`, return | throws `missingUseMuteHandlerParamsWarning` | | `useFlagHandler` | `console.warn`, return | throws `missingUseFlagHandlerParameterWarning` | | `useDeleteHandler` | silent return | throws "useDeleteHandler was called but it is missing one or more necessary parameters." | | `usePinHandler` | silent return | throws "usePinHandler was called but it is missing the message." | | `useMarkUnreadHandler` | `console.warn`, return | throws "Mark unread handler does not have access to message id" | - The exported `missingUseMuteHandlerParamsWarning` / `missingUseFlagHandlerParameterWarning` constants are kept and used as the error message, so no exports change. - The default actions show `error.message`, so a misconfigured custom setup now shows this developer-facing message instead of a false success. That is deliberate: it only happens when integrator code is wrong, and it makes the bug visible. - Known gap: that message is untranslated English, and the end user sees it in the snackbar in every locale. Failed requests already show the server's raw error text the same way. REACT-1285 tracks showing the translated fallback (e.g. "Error deleting message") as the notification's `message` while keeping the original in its `error` field for logging. That fix covers these errors too, so this PR adds nothing extra for it. - The vite example's custom message UI catches `handleFlag` rejections, like it already does for pin and delete. - Tests: the mute and flag tests that expected a warning, and the mark-unread tests that expected a silent no-op, now expect the error and check that no request was sent. Delete and pin gain a missing-parameter test. Note for custom UIs: `handleDelete`, `handlePin`, `handleMute`, `handleFlag` and `handleMarkUnread` now reject when called without the parameters they need. ### 🎨 UI Changes No visual changes. A handler called without its required parameters now shows the error notification instead of the success one. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Deleting, flagging, marking unread, muting, or pinning a message now reports an error when required message details or client information are missing, rather than silently doing nothing. * The example flag-message button now handles failed requests and avoids leaving rejected errors unhandled. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent d88dbf0 commit 290d2db

11 files changed

Lines changed: 60 additions & 23 deletions

‎examples/vite/src/CustomMessageUi/variants.tsx‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,16 @@ const CustomMessageUiActions = () => {
274274
<button onClick={handleOpenThread} title='Open thread'>
275275
↩️
276276
</button>
277-
<button onClick={handleFlag} title='Flag message'>
277+
<button
278+
onClick={async (event) => {
279+
try {
280+
await handleFlag(event);
281+
} catch (error) {
282+
console.error('Failed to flag the message', error);
283+
}
284+
}}
285+
title='Flag message'
286+
>
278287
{' '}
279288
🚩
280289
</button>

‎src/components/Message/hooks/__tests__/useDeleteHandler.test.tsx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,19 @@ describe('useDeleteHandler custom hook', () => {
5959
expect(typeof handleDelete).toBe('function');
6060
});
6161

62+
it('should throw without deleting when the message has no id', async () => {
63+
const deleteSpy = vi.spyOn(channel.messageOperations, 'delete');
64+
const handleDelete = await renderUseDeleteHandler(
65+
generateMessage({ id: undefined }) as typeof testMessage,
66+
);
67+
await act(async () => {
68+
await expect(handleDelete()).rejects.toThrow(
69+
'useDeleteHandler was called but it is missing one or more necessary parameters.',
70+
);
71+
});
72+
expect(deleteSpy).not.toHaveBeenCalled();
73+
});
74+
6275
it('should delete a message without options via the channel local-update path', async () => {
6376
const deleteSpy = vi
6477
.spyOn(channel.messageOperations, 'delete')

‎src/components/Message/hooks/__tests__/useFlagHandler.test.tsx‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,13 @@ describe('useHandleFlag custom hook', () => {
5050
expect(typeof handleFlag).toBe('function');
5151
});
5252

53-
it('should throw a warning when there are missing parameters and the handler is called', async () => {
54-
const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementationOnce(() => null);
53+
it('should throw when there are missing parameters and the handler is called', async () => {
54+
const flagSpy = vi.spyOn(client.moderation, 'flagMessage');
5555
const handleFlag = await renderUseHandleFlagHook(undefined);
56-
await handleFlag(mouseEventMock);
57-
expect(consoleWarnSpy).toHaveBeenCalledWith(missingUseFlagHandlerParameterWarning);
56+
await expect(handleFlag(mouseEventMock)).rejects.toThrow(
57+
missingUseFlagHandlerParameterWarning,
58+
);
59+
expect(flagSpy).not.toHaveBeenCalled();
5860
});
5961

6062
it('should allow to flag a message when it is successful', async () => {

‎src/components/Message/hooks/__tests__/useMuteHandler.test.tsx‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,13 @@ describe('useHandleMute custom hook', () => {
5555
expect(typeof handleMute).toBe('function');
5656
});
5757

58-
it('should throw a warning when there are missing parameters and the handler is called', async () => {
59-
const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementationOnce(() => null);
58+
it('should throw when there are missing parameters and the handler is called', async () => {
6059
const handleMute = await renderUseHandleMuteHook(undefined);
61-
await handleMute(mouseEventMock);
62-
expect(consoleWarnSpy).toHaveBeenCalledWith(missingUseMuteHandlerParamsWarning);
60+
await expect(handleMute(mouseEventMock)).rejects.toThrow(
61+
missingUseMuteHandlerParamsWarning,
62+
);
63+
expect(mute).not.toHaveBeenCalled();
64+
expect(unmute).not.toHaveBeenCalled();
6365
});
6466

6567
it('should allow to mute a user when it is successful', async () => {

‎src/components/Message/hooks/__tests__/usePinHandler.test.tsx‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,17 @@ describe('usePinHandler custom hook', () => {
168168
expect(getStoredMessage(message.id)?.pinned_by).toEqual(newerPinner);
169169
});
170170

171+
it('should throw without pinning when the message is missing', async () => {
172+
const pinSpy = vi.spyOn(client, 'pinMessage');
173+
const { handlePin } = await renderUsePinHandler(undefined as unknown as LocalMessage);
174+
await act(async () => {
175+
await expect(handlePin(mouseEventMock)).rejects.toThrow(
176+
'usePinHandler was called but it is missing the message.',
177+
);
178+
});
179+
expect(pinSpy).not.toHaveBeenCalled();
180+
});
181+
171182
it('should keep a newer pin with the same pinned value that arrived while pinning', async () => {
172183
const rejectRequest = mockPendingRequest('pinMessage');
173184
const newerPinner = generateUser();

‎src/components/Message/hooks/__tests__/userMarkUnreadHandler.test.tsx‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,8 @@ import { generateMessage, initClientWithChannels } from '../../../../mock-builde
1010
import { Channel } from '../../../Channel';
1111
import { Chat } from '../../../Chat';
1212

13-
vi.spyOn(console, 'warn').mockImplementation(() => null);
14-
1513
const event = fromPartial<React.BaseSyntheticEvent>({ preventDefault: vi.fn() });
14+
const missingMessageIdError = 'Mark unread handler does not have access to message id';
1615
const message = generateMessage() as unknown as LocalMessage;
1716

1817
let channel: ChannelType;
@@ -47,22 +46,22 @@ describe('useMarkUnreadHandler', () => {
4746
vi.restoreAllMocks();
4847
});
4948

50-
it('does not call channel.markUnread if no message is provided', async () => {
49+
it('throws without calling channel.markUnread if no message is provided', async () => {
5150
const markUnreadSpy = vi.spyOn(channel, 'markUnread');
5251
const handleMarkUnread = renderUseMarkUnreadHandlerHook();
5352
await act(async () => {
54-
await handleMarkUnread(event);
53+
await expect(handleMarkUnread(event)).rejects.toThrow(missingMessageIdError);
5554
});
5655
expect(markUnreadSpy).not.toHaveBeenCalled();
5756
});
5857

59-
it('does not call channel.markUnread if message is missing id', async () => {
58+
it('throws without calling channel.markUnread if message is missing id', async () => {
6059
const markUnreadSpy = vi.spyOn(channel, 'markUnread');
6160
const handleMarkUnread = renderUseMarkUnreadHandlerHook({
6261
message: generateMessage({ id: undefined }) as unknown as LocalMessage,
6362
});
6463
await act(async () => {
65-
await handleMarkUnread(event);
64+
await expect(handleMarkUnread(event)).rejects.toThrow(missingMessageIdError);
6665
});
6766
expect(markUnreadSpy).not.toHaveBeenCalled();
6867
});

‎src/components/Message/hooks/useDeleteHandler.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ export const useDeleteHandler = (
2727

2828
return async (options?: DeleteMessageOptions) => {
2929
if (!message?.id || !client) {
30-
return;
30+
throw new Error(
31+
'useDeleteHandler was called but it is missing one or more necessary parameters.',
32+
);
3133
}
3234

3335
try {

‎src/components/Message/hooks/useFlagHandler.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ export const useFlagHandler = (message?: LocalMessage): ReactEventHandler => {
1515
event.preventDefault();
1616

1717
if (!client || !t || !message?.id) {
18-
console.warn(missingUseFlagHandlerParameterWarning);
19-
return;
18+
throw new Error(missingUseFlagHandlerParameterWarning);
2019
}
2120

2221
await client.moderation.flagMessage({ message_id: message.id });

‎src/components/Message/hooks/useMarkUnreadHandler.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,7 @@ export const useMarkUnreadHandler = (
2121
return async (event) => {
2222
event.preventDefault();
2323
if (!message?.id) {
24-
console.warn('Mark unread handler does not have access to message id');
25-
return;
24+
throw new Error('Mark unread handler does not have access to message id');
2625
}
2726

2827
// Request failures propagate so the caller can report them; `notify` only reports success.

‎src/components/Message/hooks/useMuteHandler.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,7 @@ export const useMuteHandler = (
3030

3131
// The action runs even without `notify` (`Message` passes none); it only reports the outcome.
3232
if (!message?.user || !client) {
33-
console.warn(missingUseMuteHandlerParamsWarning);
34-
return;
33+
throw new Error(missingUseMuteHandlerParamsWarning);
3534
}
3635
const mutes = client.mutedUsersStore.getLatestValue().mutedUsers ?? [];
3736

0 commit comments

Comments
 (0)