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 mark-seen-logic #271

Merged
merged 13 commits into from
May 27, 2024
Merged

Improve mark-seen-logic #271

merged 13 commits into from
May 27, 2024

Conversation

rottabonus
Copy link
Member

@rottabonus rottabonus commented Apr 21, 2024

What has changed?

  • When message is only fully visible on the viewport for 1s, then we mark it as unseen.
  • If fetching older messages for buddy or initial messages, and we receive a response where the oldest message is unseen, keep fetching until
    a) received oldest message is not unread
    b) received all messages

Bonus:
User-report e2e-test was broken, so at the same time when fixed, I added a fix for this: https://trello.com/c/zYeyhfCO/936-keyboardavoider-to-user-report-form

Why was the change made?

When investigating messaging issues reported by users, found out this oversight.

  1. Message is always marked as read if the message is opened in the list (even if not visible!)
  2. The unseen-selector of buddy-messages checks only the last received message, but its possible that user has only seen most recent unread messages, but has not scrolled to top

Caveats?

  • Dangerous to touch this logic, but tested it pretty well

Related Trello issue

Link to the Trello ticket

Checklist

  • I have updated relevant documentation in READMES

@rottabonus rottabonus force-pushed the felix/mark-seen-improvement branch 3 times, most recently from 343a64b to b3d0264 Compare May 10, 2024 07:41
- Test not actually working currently, it was just manual check.
- Would be quite cumbersome automated test, to wait
- Also fix selectors unseen check: check all messages, not just last
  message
- If we are fetching older messages for buddy, then keep fetching until
  oldest is not unread
- It is possible that InitialMessages should spawn new requests for
  fetching messages.
- For example Mr. A has sent Mr. B 23 messages. If B fetches initially
  10, it should also fetch the 13 unread!
- Its safer that way, and also it is fine
@rottabonus rottabonus force-pushed the felix/mark-seen-improvement branch 2 times, most recently from 2d1fca6 to bce0eaa Compare May 19, 2024 08:19
- if A sends B 20 messages, then when B opens app, B wants to load all
  unread messages
- readd test for deleted user as recent chat
@rottabonus rottabonus force-pushed the felix/mark-seen-improvement branch from bce0eaa to 447ac73 Compare May 19, 2024 08:42
@rottabonus rottabonus marked this pull request as ready for review May 26, 2024 06:12

const getPreviousMessagesIfNotLoading = () => {
if (isLoading) {
if (isLoading || messageList.length < messageApi.MAX_MESSAGES_AT_ONCE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens here?

Why we do nothing if len is less than max messages at once?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there is under that many messages, we dont want to fetch older messages.
There can only be older messages if there is more than the fetch amount.

Copy link
Contributor

Choose a reason for hiding this comment

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

Taa ei oo mulle ilmiselvaa et taa on oikein mut salee on :D, jos oot varma et menee oikein ni sit fine!

Must tuntu et tas on joku edge case mut ei kyl valtsiin oo

Copy link
Contributor

@pheis pheis left a comment

Choose a reason for hiding this comment

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

Probably proper good

I did not get why we do nothing if len is less than 10 with messages gottten on UI layer.

To me that feels more like API related decision that should not be done on rendering layer. There is not enough context to understand WHY on the UI code part.

I would like to understand WHY we do that check?

LGTM otherwise

@pheis pheis self-requested a review May 27, 2024 05:52
@rottabonus rottabonus merged commit c15afb2 into master May 27, 2024
2 checks passed
@rottabonus rottabonus deleted the felix/mark-seen-improvement branch May 27, 2024 05:56
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