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

follow-up(SystemMessageGroup) - improve behaviour #10758

Closed
2 of 3 tasks
Antreesy opened this issue Oct 23, 2023 · 11 comments · Fixed by #11063
Closed
2 of 3 tasks

follow-up(SystemMessageGroup) - improve behaviour #10758

Antreesy opened this issue Oct 23, 2023 · 11 comments · Fixed by #11063
Assignees
Milestone

Comments

@Antreesy
Copy link
Contributor

Antreesy commented Oct 23, 2023

How to use GitHub

  • Please use the 👍 reaction to show that you are interested into the same feature.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Follow-up / Regression fixes for #9777:

  • Showing in LeftSidebar in conversation
  • Fixing the Unread messages marker (may be hidden in the list)
  • Disable/remove/rework sorting system messages in the group

Feel free to add items to the list

@DorraJaouad
Copy link
Contributor

Just a thought if we are keeping the read marker logic :
What if we just keep the combined system message not collapsed as long as LastReadMessageId is in one of its messages ?

For LeftSidebar, either we do the same ( keep it not collapsed by default if it is the last message) or we can reflect this "nested" format ( collapsed message > combined message > MessagesSystemGroup) in the store so we can get the combined message and render it in Conversation.

@nickvergessen
Copy link
Member

This is solved for now, right?

@DorraJaouad
Copy link
Contributor

No, only sorting messages was solved.

@nickvergessen
Copy link
Member

Left sidebar I think not grouping is okay.

The readmarker could be because of mixed up sorting. But then it would be live already on our server and others reported just again that it happens, so yeah might still have issues.

@DorraJaouad
Copy link
Contributor

The readmarker could be because of mixed up sorting. But then it would be live already on our server and others reported just again that it happens, so yeah might still have issues.

As I checked last time, it was because of the collapsing messages ( read marker clears the counter only when it is visible)

@nickvergessen
Copy link
Member

maybe we then switch and make it group on the last one (or well set the last message's id as the visible message id on the element?)

@DorraJaouad
Copy link
Contributor

The thing is the grouped message is not actually a message where we can get its id. It is not stored but kinda "computed" ?
It should be added to the store so we can get it in LeftSidebar.

@nickvergessen
Copy link
Member

It should be added to the store so we can get it in LeftSidebar.

Or we keep it simple and only show non-combined messages as last message? Simplicity is key!

@DorraJaouad
Copy link
Contributor

Or we keep it simple and only show non-combined messages as last message?

The issue was that, because in the chat view, you don't see the last non-combined message but you see its "parent" combined message.

A simple solution would be the following:
Always show grouped system messages NOT collapsed by default if they are the last messages in the chat.

@Antreesy what do you think ?

@nickvergessen
Copy link
Member

The issue was that, because in the chat view, you don't see the last non-combined message but you see its "parent" combined message.

We can change that to have the last message id instead of the first as pointed out before :)

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 1, 2023

At the moment combined message inherits id from the first in group, which leads to visual bug (both are equal to getVisualLastReadMessageId):
image

Switching from first id to last won't fix it. Trying to implement fix atm

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

Successfully merging a pull request may close this issue.

3 participants