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

feat: register first unread message id on message.new & when initializing Channel state #1211

Closed
wants to merge 5 commits into from

Conversation

MartinCupela
Copy link
Contributor

Description of the changes, What, Why and How?

To achieve consistency in the UI concerning the display of unread messages indicators, we need to provide the first_unread_message_id value consistently. The value is not provided when querying channels, neither when a new message arrives. Therefore, we have to calculate it client-side.
In this PR we register the first_unread_message_id in message.new handler and in Channel._initializeState(). The latter is invoked every time a Channel instance is created or channel data is queried from the BE. In Channel._initializeState() we inspect the whole message set and not only the recently queried messages.

This PR also removes dead code, that was anyways overwritten on the following lines.

Copy link
Contributor

github-actions bot commented Jan 16, 2024

Size Change: +915 B (0%)

Total Size: 329 kB

Filename Size Change
dist/browser.es.js 70.8 kB +191 B (0%)
dist/browser.full-bundle.min.js 44 kB +129 B (0%)
dist/browser.js 71.8 kB +205 B (0%)
dist/index.es.js 70.8 kB +185 B (0%)
dist/index.js 71.8 kB +205 B (0%)

compressed-size-action

Copy link
Contributor

@szuperaz szuperaz left a comment

Choose a reason for hiding this comment

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

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

@MartinCupela
Copy link
Contributor Author

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

@szuperaz
Copy link
Contributor

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

Did you find a scenario where the implementation based on last_read_message_id wasn't working? I'm asking this because iOS uses that algorithm (code) and mark unread feature is already available there. So if the code works for them, it can work for us too. It feels safer to copy an algorithm already used in an SDK, than to create a new one.

It also feels more consistent, even though it's possible your algorithm does the same as iOS but it's hard to compare the two .

@MartinCupela
Copy link
Contributor Author

I

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

Did you find a scenario where the implementation based on last_read_message_id wasn't working? I'm asking this because iOS uses that algorithm (code) and mark unread feature is already available there. So if the code works for them, it can work for us too. It feels safer to copy an algorithm already used in an SDK, than to create a new one.

It also feels more consistent, even though it's possible your algorithm does the same as iOS but it's hard to compare the two .

I personally have some read states in queried channels without last_read_message_id + the type is declared with it as optional whereas last_read is mandatory.

image

@szuperaz
Copy link
Contributor

I

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

Did you find a scenario where the implementation based on last_read_message_id wasn't working? I'm asking this because iOS uses that algorithm (code) and mark unread feature is already available there. So if the code works for them, it can work for us too. It feels safer to copy an algorithm already used in an SDK, than to create a new one.
It also feels more consistent, even though it's possible your algorithm does the same as iOS but it's hard to compare the two .

I personally have some read states in queried channels without last_read_message_id + the type is declared with it as optional whereas last_read is mandatory.

image

But this is expected, and the iOS algorithm handles it (Scenario 3). I think the last_read_message_id is missing in this specific example because you haven't opened the channel since the last_read_message_id field has been implemented.

@MartinCupela
Copy link
Contributor Author

I

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

Did you find a scenario where the implementation based on last_read_message_id wasn't working? I'm asking this because iOS uses that algorithm (code) and mark unread feature is already available there. So if the code works for them, it can work for us too. It feels safer to copy an algorithm already used in an SDK, than to create a new one.
It also feels more consistent, even though it's possible your algorithm does the same as iOS but it's hard to compare the two .

I personally have some read states in queried channels without last_read_message_id + the type is declared with it as optional whereas last_read is mandatory.
image

But this is expected, and the iOS algorithm handles it (Scenario 3). I think the last_read_message_id is missing in this specific example because you haven't opened the channel since the last_read_message_id field has been implemented.

So you are suggesting to use last_read as a fallback in case last_read_message_id does not exist?

@szuperaz
Copy link
Contributor

I

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

Did you find a scenario where the implementation based on last_read_message_id wasn't working? I'm asking this because iOS uses that algorithm (code) and mark unread feature is already available there. So if the code works for them, it can work for us too. It feels safer to copy an algorithm already used in an SDK, than to create a new one.
It also feels more consistent, even though it's possible your algorithm does the same as iOS but it's hard to compare the two .

I personally have some read states in queried channels without last_read_message_id + the type is declared with it as optional whereas last_read is mandatory.
image

But this is expected, and the iOS algorithm handles it (Scenario 3). I think the last_read_message_id is missing in this specific example because you haven't opened the channel since the last_read_message_id field has been implemented.

So you are suggesting to use last_read as a fallback in case last_read_message_id does not exist?

No, I'm suggesting to use the algorithm that iOS has implemented. In the case last_read_message_id is not defined it uses Scenario 3:

Screenshot 2024-01-19 at 09 53 24

I'm trying to understand why we aren't using the algorithm specified in the Notion document. You mentioned the edge case of last_read_message_id not being defined, but the iOS algorithm handles that specific edge case (see above).

@MartinCupela
Copy link
Contributor Author

MartinCupela commented Jan 19, 2024

I

Can't we use this algorithm:https://www.notion.so/stream-wiki/Mark-Unread-Developer-Specs-3b05e4fb069a4e2b9355216a5e6085a2?pvs=4#04bc470a33e249c6ba9e7e6c8d6c9279? That algorithm computes first_unread_message_id based on last_read_message_id so we would have to run that algorithm whenever the last_read_message_id can change, which are these places based on the code:

  • message.read event
  • notification.mark_unread event
  • _initializeState function

I have tested that last_read_message_id is not guaranteed to be provided. I actually had an implementation with that one, but as it does not have to be always available I decided to go the safe path.

Did you find a scenario where the implementation based on last_read_message_id wasn't working? I'm asking this because iOS uses that algorithm (code) and mark unread feature is already available there. So if the code works for them, it can work for us too. It feels safer to copy an algorithm already used in an SDK, than to create a new one.
It also feels more consistent, even though it's possible your algorithm does the same as iOS but it's hard to compare the two .

I personally have some read states in queried channels without last_read_message_id + the type is declared with it as optional whereas last_read is mandatory.
image

But this is expected, and the iOS algorithm handles it (Scenario 3). I think the last_read_message_id is missing in this specific example because you haven't opened the channel since the last_read_message_id field has been implemented.

So you are suggesting to use last_read as a fallback in case last_read_message_id does not exist?

No, I'm suggesting to use the algorithm that iOS has implemented. In the case last_read_message_id is not defined it uses Scenario 3:

Screenshot 2024-01-19 at 09 53 24

I'm trying to understand why we aren't using the algorithm specified in the Notion document. You mentioned the edge case of last_read_message_id not being defined, but the iOS algorithm handles that specific edge case (see above).

I have considered the fact that Scenario 3 relies on Scenario 1. Scenario 1 requires knowledge of whether all messages have been loaded. JS client does not handle pagination and so it does not have information about whether more messages can be queried.

The algorithm in this PR covers all what iOS spec mentions. An exception could be Scenario 1 - we do not have read status for the current user. They mention the cause for that is that the channel has never been marked read. In that case the UI SDK has to count that channel.state.read[client.user.id] can be undefined and not show any UI elements regarding the unread count / unread messages.

src/channel.ts Outdated Show resolved Hide resolved
src/channel.ts Show resolved Hide resolved
src/channel.ts Show resolved Hide resolved
@MartinCupela
Copy link
Contributor Author

It will be unnecessary to calculate first unread message id. If not available at the moment of jumping to the first unread message, it will be determined as a message following the last read message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants