Skip to content

Conversation

@AhyoungRyu
Copy link
Contributor

@AhyoungRyu AhyoungRyu commented Nov 20, 2024

Fixes some of comments in SBISSUE-17017

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.

  • All tests pass locally with my changes
  • 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)

@netlify
Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for sendbird-uikit-react ready!

Name Link
🔨 Latest commit 1a40572
🔍 Latest deploy log https://app.netlify.com/sites/sendbird-uikit-react/deploys/673e94709216090008413711
😎 Deploy Preview https://deploy-preview-1254--sendbird-uikit-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AhyoungRyu AhyoungRyu self-assigned this Nov 20, 2024
Copy link
Contributor

@HoonBaek HoonBaek left a comment

Choose a reason for hiding this comment

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

LGTM!

@AhyoungRyu AhyoungRyu merged commit 29112c5 into main Nov 21, 2024
10 checks passed
@AhyoungRyu AhyoungRyu deleted the fix/SBISSUE-17017 branch November 21, 2024 04:59
@bang9
Copy link
Contributor

bang9 commented Nov 22, 2024

@HoonBaek @AhyoungRyu The onBeforeHandler is essentially something the user injects, so I don't think we need to handle errors for them—they should be able to handle that themselves.

What the customer seems to want is to handle failures when sending messages using SDK.
Shouldn't it be handled like this?

try {
  const params = await onBeforeSendUserMessage(...);
  return await sendUserMessage(params); // -> here
} catch (error) {
  // Error: Receiver is blocked
  eventHandlers.message.onSendMessageFailed(error);
}

@bang9
Copy link
Contributor

bang9 commented Nov 22, 2024

I also reviewed the ticket that led to this requirement, and it seems they don’t actually need error handling but rather additional processing after a file message has been successfully sent.

In the onBeforeHandler, they are sending the file message separately, performing additional processing for it, and then throwing an error to cancel the actual message send operation.

This error ends up being an uncaught error, which is why they are requesting a handler.

This implementation is quite strange, so wouldn’t it be better to provide a handler for post-send operations instead?

const params = await onBeforeSendUserMessage(...);
const message = await sendUserMessage(params);
await onAfterSendUserMessage(message, params);
return message;
onAfterSendFileMessage={async (message, params) => {
  if (isValidImage(params)) {
      uploadImageToS3({
        params,
        channelId: currentChannelId.current,
        messageId: message.messageId.toString(),
      }).then(() => {
        handleImageSentAnalytics(sbChannel, userId, env.name);    
      });
  }
}}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants