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

fix: channel.visible not taking sort and pinned channels into account #2925

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

isekovanic
Copy link
Contributor

@isekovanic isekovanic commented Jan 31, 2025

🎯 Goal

This PR fixes an issue with channel pinning where pinned channels aren't respected if a channel goes from hidden to visible.

Steps to reproduce:

  • Pin 1 or more channels within ChannelList
  • Take another channel (not a pinned one) and channel.hide() it
  • After that, run channel.show() without reloading the app (so that no HTTP requests are done in the meantime)
  • The recently shown channel will not respect pinned ones

Also, pretty sure that notification.message_new handler is also incorrect since it doesn't take archiving into account, but will look into that on Monday.

Update: This was indeed an issue with notification.message_new as well and is fixed in this PR.

This is already fixed in the recent reactive channel list state PR in the LLC, but porting it here too so that we don't leave the SDK with a bug until that one gets merged.

🛠 Implementation details

🎨 UI Changes

iOS
Before After
Android
Before After

🧪 Testing

☑️ Checklist

  • I have signed the Stream CLA (required)
  • PR targets the develop branch
  • Documentation is updated
  • New code is tested in main example apps, including all possible scenarios
    • SampleApp iOS and Android
    • Expo iOS and Android

@Stream-SDK-Bot
Copy link
Contributor

Stream-SDK-Bot commented Jan 31, 2025

SDK Size

title develop branch diff status
js_bundle_size 468 KB 468 KB 0 B 🟢

@khushal87
Copy link
Member

Also, pretty sure that notification.message_new handler is also incorrect since it doesn't take archiving into account, but will look into that on Monday.

I am not sure if I understand this correctly. We added a condition for handling archiving channels in the notification.message_new handler though. Let me know if I am missing something.

@isekovanic
Copy link
Contributor Author

Also, pretty sure that notification.message_new handler is also incorrect since it doesn't take archiving into account, but will look into that on Monday.

I am not sure if I understand this correctly. We added a condition for handling archiving channels in the notification.message_new handler though. Let me know if I am missing something.

That's my bad, I meant to say that pinning isn't respected *

@isekovanic isekovanic merged commit f99d602 into develop Feb 5, 2025
6 checks passed
@isekovanic isekovanic deleted the fix/improper-channel-reorder-on-pinned branch February 5, 2025 13:27
@github-actions github-actions bot mentioned this pull request Feb 5, 2025
6 tasks
@stream-ci-bot
Copy link
Contributor

🎉 This PR is included in version 6.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

arnautov-anton added a commit to GetStream/stream-chat-react that referenced this pull request Feb 6, 2025
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