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

Load history and first set of new messages with 1 API request #8717

Merged
merged 14 commits into from
Feb 10, 2023

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Feb 7, 2023

Ref #8710

🚧 TODO

  • Add endpoint to get the message context
    • Integration tests
  • Use for first load of messages when entering a chat
  • Revert not setting the read marker
  • JS/Frontend Test
    • Check guests behaviour
    • Check self-joined users behaviour
    • Check revisiting a check room
    • Check behaviour with more than 100 unread messages (context end)
    • Check behaviour with more than 300 unread messages (first future end)

🏁 Checklist

@nickvergessen nickvergessen added this to the 💚 Next Major (26) milestone Feb 7, 2023
@nickvergessen nickvergessen self-assigned this Feb 7, 2023
@nickvergessen nickvergessen added enhancement feature: chat 💬 Chat and system messages feature: api 🛠️ OCS API for conversations, chats and participants feature: frontend 🖌️ "Web UI" client performance 🚀 labels Feb 7, 2023
@nickvergessen
Copy link
Member Author

Scenarios I tested:

🗯 Message 1-150
🗯 Message 151  === lastReadMessage (from API)
🗯 Message 152-170
🗯 Message 1-150
🗯 Message 151  === lastReadMessage (from API)
🗯 Message 152-300
🗯 Message 1-150
😄 Reactions 1-150 on Message 150
🗯 Message 151  === lastReadMessage (from API)
🗯 Message 152-300
🗯 Message 1-150
🗯 Message 151  === lastReadMessage (from API)
😄 Reactions 1-150 on Message 151
🗯 Message 152-300
🗯 Message 1-150
😄 Reactions 1-150 on Message 150
😄 Reactions 150  === lastReadMessage (from API)
🗯 Message 152-300

All of them produce an acceptable state after loading the chat (better or equal to the previous behaviour). Some could still be improved in terms of scrolling off-set but that was already the case before, so I think we can keep it like that for now.

Any (edge) cases I forgot?

@danxuliu
Copy link
Member

danxuliu commented Feb 8, 2023

Any (edge) cases I forgot?

I would also test the reaction scenarios with a deleted message instead of a reaction, and with a command from another participant that is not visible for the user loading the chat instead of a reaction.

@nickvergessen
Copy link
Member Author

I would also test the reaction scenarios with a deleted message instead of a reaction

Deleted messages are in the DOM, so they are not any different from normal messages

and with a command from another participant that is not visible for the user loading the chat instead of a reaction.

Similar to messages that are not in the DOM but in the store, completely non-existing messages will just fall back to the previous visible one, so should be covered as well.

@nickvergessen nickvergessen force-pushed the perf/8710/message-context branch from 7d05367 to 0920722 Compare February 9, 2023 14:56
@nickvergessen nickvergessen marked this pull request as ready for review February 9, 2023 14:57
@nickvergessen nickvergessen changed the title Perf/8710/message context Load history and first set of new messages with 1 API request Feb 9, 2023
@nickvergessen nickvergessen force-pushed the perf/8710/message-context branch 2 times, most recently from ce65da3 to 53ed7c8 Compare February 10, 2023 08:25
First usage will be entering a chat room where we get the context
of the read marker

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
…ffset

Signed-off-by: Joas Schilling <coding@schilljs.com>
…age is not there

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
This allows some kind of A-B testing

Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the perf/8710/message-context branch from 53ed7c8 to 2be7deb Compare February 10, 2023 10:42
@nickvergessen nickvergessen merged commit d9af7be into master Feb 10, 2023
@nickvergessen nickvergessen deleted the perf/8710/message-context branch February 10, 2023 11:30
@SystemKeeper
Copy link
Contributor

Could this be used (or easily extended) to solve #6046 and be used in mobile clients?

@nickvergessen
Copy link
Member Author

We are doing it in the web now, if you did not open the conversation before :)
But otherwise we can't as it might create a hole in the list of loaded messages :(

@SystemKeeper
Copy link
Contributor

But otherwise we can't as it might create a hole in the list of loaded messages :(

Could we remove the stored messages for that conversation when we open it from search to get the correct context ?

@nickvergessen
Copy link
Member Author

Put as an idea into #6046 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement feature: api 🛠️ OCS API for conversations, chats and participants feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants