[APP-7066] - consider local send status messages as current users' #32
Conversation
|
|
||
| // group pending messages with any message type other than failed | ||
| // Given the above is true, group by timestamp and sender id | ||
| if ([message.sendingStatus, comparingMessage.sendingStatus].includes(SendingStatus.PENDING) && |
There was a problem hiding this comment.
this fixed a different bug, but will be obsolete/unnecessary with the new fix.
| replyType = '', | ||
| }: GetMessagePartsInfoProps): OutPuts => { | ||
| currentUserId = undefined | ||
| }: GetMessagePartsInfoProps & { currentUserId?: string }): OutPuts => { |
There was a problem hiding this comment.
currentUserId will be undefined by default anyway right? Any reason to explicitly given it a default of undefined here?
There was a problem hiding this comment.
ah no ur right, I had previously had it as null and refactored it to undefined
|
Any theories on why it's undefined sometimes on pending messages? |
its not undefined, its an empty string, and I haven'ttt got a clueeeeee lol |
|
it would seem they are somewhat aware of the problem themselves, they've dealt with it in some areas where they have defined |
|
This is the PR(sendbird#23), where they fix some undescribed bug caused by an incomplete definition of |
| message: CoreMessageType, | ||
| comparingMessage: CoreMessageType, | ||
| currentChannel?: GroupChannel, | ||
| currentUserId?: string |
There was a problem hiding this comment.
What was the group being used for before?
There was a problem hiding this comment.
It was used previously to determine grouping by read status, which we removed cause it was buggy on some clients(jinen's) and we weren't using read status anyways.
https://github.com/sendbird/sendbird-uikit-react/blob/main/src/utils/messages.ts
The usage was removed but the parameter got left in
|
Can you add a test plan? I know it's intermittent, but I think it will be helpful looking back on this PR. |
lol its unfortunate because there is no reliable repro. But, I'll add a test plan to make sure we didn't break anything |
* [APP-7066] - consider local send status messages as current users' * pr fb
* [APP-7066] - consider local send status messages as current users' * pr fb
* [APP-7066] - consider local send status messages as current users' * pr fb
* [APP-7066] - consider local send status messages as current users' * pr fb
Description
The problem appears to be that occasionally, pending(local) messages sometimes don't have the
sender.userIdon it. It resolves itself once the message successfully sends, but there is a brief moment in time, where we have a message and we don't know who it is from if we are only basing it off of userId.Test Plan
Verify:
messages you send are grouped together if they are sent within the same minute.
messages you send are not grouped together with messages from other users.