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

Improve (and fix) unread messages and navigation #123

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

brichet
Copy link
Collaborator

@brichet brichet commented Dec 16, 2024

This PR fixes the autoscroll when opening a chat, by scrolling to the first unread message.

Fixes #121

Peek 2024-12-16 11-05

  • wait for all the message to be rendered the first time (rendermime is async) before updating the unread messages, and displaying the navigation component. This avoid inconsistencies in the viewport computation because more messages are display before their content is fully rendered.
  • in the case of jupyterlab_chat, wait for the chat to have an ID before adding messages. The ID is used to save and restore the last read message, and to populate unread message list when a message is added. If the ID is not set, this lead to inconsistency in the unread list.
  • refactor the codebase handling the viewport, by not sending the IntersectionObserver to each message.

Copy link
Contributor

Binder 👈 Launch a Binder on branch brichet/jupyter-chat/scroll_to_unread

@brichet brichet added the bug Something isn't working label Dec 16, 2024
@brichet
Copy link
Collaborator Author

brichet commented Dec 16, 2024

please update snapshots

@brichet brichet marked this pull request as ready for review December 16, 2024 13:50
Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@brichet Thank you for working on this while I was away! I appreciate the creative twist you added to the original feature request. ❤️

While the new feature proposed here is unique and is clearly a labor of love, I am worried that it may not be intuitive to users. Every chat application I'm aware of always defaults to scrolling all the way to the bottom, with the most recent message in view. There are good reasons for always opening at the bottom of the scroll container:

  • Scrolling to the last unread message may confuse users, because it may be somewhere in the middle of the chat. If the user doesn't remember the last message they read before exiting, a user will ask why they're seeing a random set of messages instead of the most recent ones.
  • When a user opens a chat, a user generally does so with the intention of sending a new message. If the viewport isn't already at the bottom of the scroll container, the user would have to scroll all the way down to see their new message.
  • I am having some difficulty reviewing this PR due to the complexity of the changes proposed. If we do decide to prefer always scrolling to the bottom, I think the PR could be simplified, making the frontend code easier to maintain in the future.

Slack does offer a "unread messages" button that floats at the top of the scroll container when there are unread messages above the viewport. This also provides the capability for users to catch up on all unread messages, and we should consider this as an alternative. Let's discuss this at standup tmrw! 👍

@brichet
Copy link
Collaborator Author

brichet commented Dec 18, 2024

Thanks @dlqqq for the comment.

Every chat application I'm aware of always defaults to scrolling all the way to the bottom, with the most recent message in view. There are good reasons for always opening at the bottom of the scroll container:

Right, I can update it. I think I often use the notifications to open the chat, which lead me to the unread message and not the end of the chat, this is why I thought it make sense.

Slack does offer a "unread messages" button that floats at the top of the scroll container when there are unread messages above the viewport.

This is also included in this chat, here.

  • I am having some difficulty reviewing this PR due to the complexity of the changes proposed. If we do decide to prefer always scrolling to the bottom, I think the PR could be simplified, making the frontend code easier to maintain in the future.

Unfortunately not, the main issue that this PR is fixing is to not change the unread message list until all the messages are rendered, when you open a chat. Before all the message are rendered, the viewport contains a lot more messages because they are all empty. The unread message list was modified during that step, even if the messages have not been displayed.
The only part that will change with you suggestion is https://github.com/jupyterlab/jupyter-chat/pull/123/files#diff-d2e543e917525f21fbd1a87840f9770cbd228af378d969e84ca7f8a1730c2a38R530

@brichet
Copy link
Collaborator Author

brichet commented Dec 18, 2024

@dlqqq I updated it to move to the last message when opening the chat.

There is a drawback with this, because of the way we save the unread messages in the browser storage.
To avoid saving extra information, only the timestamp of the most recent read message is saved in the storage.
This value is used to set up the list of unread messages when opening the chat: all messages with a date after this value are considered unread. Therefore, if you open the chat twice, there is no more unread message, because the last message has been read.

I don't think this really important at the moment, but it should be fixed in the future.

@dlqqq
Copy link
Member

dlqqq commented Dec 19, 2024

@brichet

Therefore, if you open the chat twice, there is no more unread message, because the last message has been read.

but it should be fixed in the future.

We could change this in the future, but I think this behavior is actually expected, since this is how iMessage, Discord, and Slack work. Even if you don't read all the messages, opening & closing the chat channel marks everything as read.

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@brichet This looks good, thank you for incorporating some of my suggestions! I still think there are opportunities to simplify the frontend implementation, but we can address that later. 👍

@brichet
Copy link
Collaborator Author

brichet commented Dec 19, 2024

I still think there are opportunities to simplify the frontend implementation, but we can address that later.

👍 Agree

Thanks for the review

@brichet brichet merged commit af64776 into jupyterlab:main Dec 20, 2024
11 checks passed
@brichet brichet deleted the scroll_to_unread branch December 20, 2024 12:12
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.

Autoscroll to the bottom when opening an existing chat
2 participants