Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug-fix: when other user leaves, do not remove channel #937

Closed
wants to merge 4 commits into from

Conversation

aldenquimby
Copy link

@aldenquimby aldenquimby commented Jan 22, 2024

steps to reproduce:

  1. User A creates a new group channel with 3+ users
  2. User B leaves the channel

expected:

  • User A still sees the channel, with it's message history
  • members are updated, and possibly channel avatar is updated too

actual:

  • channel immediately disappears for User A
  • User A must refresh / reload the page to see the channel
  • this is particularly jarring if it was your current channel and you were actively chatting

External Contributions

@@ -66,7 +66,7 @@ function ChannelAvatar({
alt={channel?.name}
/>
)
), [channel?.members, channel?.coverUrl, theme]);
), [utils.getChannelAvatarSource(channel, userId), theme]);
Copy link
Author

@aldenquimby aldenquimby Jan 22, 2024

Choose a reason for hiding this comment

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

Problem with prior memoization - when a user leaves a group, the channel.members array appears to be the same object reference, so even though there is 1 less member, the memoization doesn't refire even though it should

This manifested as a bug: a member who just left still accidentally appears in the ChannelAvatar (both on ChannelListUI and ChannelUI) until a page refresh occurs

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes correct! thanks for spotting this as well! React does not recognize changes of instance properties. We will apply your sugestion as well!

@@ -108,28 +108,36 @@ export default function channelListReducer(
.with({ type: channelListActions.ON_USER_LEFT }, (action) => {
const { channel, isMe } = action.payload;
const { allChannels, currentUserId, currentChannel, channelListQuery, disableAutoSelect } = state;
let nextChannels = allChannels.filter((ch) => ch.url !== channel.url);
Copy link
Author

@aldenquimby aldenquimby Jan 22, 2024

Choose a reason for hiding this comment

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

The diff here is worse than I'd like, and could be reduced if we want, but I found it much easier to reason about what was happening with an explicit top-level isMe if/else

Examples:

  1. if I left, we don't need to run filterChannelListParams - we know we need to remove the channel
  2. if someone else left, we don't need to touch currentChannel

// If someone else left: update allChannels if needed (don't touch currentChannel)
else {
let nextChannels = allChannels.filter((ch) => ch.url !== channel.url);
if (!channelListQuery || filterChannelListParams(channelListQuery, channel, currentUserId)) {
Copy link
Author

Choose a reason for hiding this comment

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

There's a change in logic here... if no channelListQuery, we should not be removing the channel (but the prior logic was incorrectly removing the channel)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are correct, thanks for spotting this out.

@liamcho
Copy link
Contributor

liamcho commented Jan 29, 2024

@aldenquimby Hi thanks for providing the bug fix PR. We are onto fixing this bug and targeted to release in the next patch version, v3.10.2. Please feel free to review the PR as you wish.

liamcho added a commit that referenced this pull request Jan 29, 2024
…ist when other member leaves the channel (#947)

Fixes: [CLNP-2121](https://sendbird.atlassian.net/browse/CLNP-2121)

### Original PR
- #937

### Changelogs
- Fixed a bug where channel is being removed from my channel list when
other member leaves the channel
- Fixed a bug where channel avatar image is not updated when a member
leaves, or joins, or profileUrl changes

Co-authored-by: @aldenquimby

[CLNP-2121]:
https://sendbird.atlassian.net/browse/CLNP-2121?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

---------

Co-authored-by: Baek EunSeo <[email protected]>
@liamcho
Copy link
Contributor

liamcho commented Jan 29, 2024

Closing this PR because the fix PR has been merged

@liamcho liamcho closed this Jan 29, 2024
@aldenquimby
Copy link
Author

Thank you @liamcho ! Appreciate it 💪

@aldenquimby aldenquimby deleted the other-user-left branch January 29, 2024 19:05
@bang9 bang9 added the Merged Indicating the merging of external contributors' pull requests label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged Indicating the merging of external contributors' pull requests
Projects
None yet
3 participants