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

Sending a reaction bolds the room #9952

Closed
turt2live opened this issue Jun 4, 2019 · 11 comments · Fixed by matrix-org/matrix-react-sdk#3184
Closed

Sending a reaction bolds the room #9952

turt2live opened this issue Jun 4, 2019 · 11 comments · Fixed by matrix-org/matrix-react-sdk#3184

Comments

@turt2live
Copy link
Member

until I move my mouse again.

@jryans
Copy link
Collaborator

jryans commented Jun 4, 2019

Hmm, does the bolding happen immediately after sending the reaction? I don't think I have noticed this in testing yet, but perhaps we have different account settings for the room list, etc.?

@turt2live
Copy link
Member Author

Immediately after, yes. I have a slow homeserver, which may be contributing to the visibility of the problem.

@jryans
Copy link
Collaborator

jryans commented Jun 5, 2019

We may need to tweak the unread state processing for reactions (or relations more generally). Our current UX thinking is that relations should not affect the unread state of the room.

@jryans jryans added the phase:2 label Jun 6, 2019
@ara4n
Copy link
Member

ara4n commented Jun 12, 2019

i can't reproduce this - @turt2live, pics or it didn't happen?

@turt2live
Copy link
Member Author

Will get a gif later today. It's definitely happening.

@turt2live
Copy link
Member Author

Note how adding a +1 to a random message bolds the room on the left momentarily. While testing, I noticed that this only happens when there's been no other reactions after the most recent message (meaning I'm the first "hidden" event to be sent).

reactionbold mp4

@jryans
Copy link
Collaborator

jryans commented Jun 26, 2019

So far, I can't seem to reproduce this after trying various accounts, room types, messages, etc. I tried the case of ensuring the reaction is the first hidden event, but that did not seem to be enough to trigger it for me.

@turt2live I think I may need some help isolating the condition to reproduce this.

  • Are you using https://riot.im/develop above, or something with a custom config?
  • Do you see this issue with a new test account on your HS (with reactions enabled)?
  • Do you see this issue with a new test account on matrix.org HS (with reactions enabled)?
  • Does it happen in both clear text and E2E rooms?
  • Does it happen in both group and DM rooms?
  • Does the message you react to need to be from someone other than yourself?
  • Does the message have to have a URL preview?

@jryans
Copy link
Collaborator

jryans commented Jun 26, 2019

Hmm, well, I did just observe this happening in a DM room with @nadonomy. I'll try to isolate why it happens there.

@turt2live
Copy link
Member Author

In answer to your questions:

  • Yes, on develop with the labs settings. Using Chrome.
  • Yes, tested with t2l.io on riot.im/develop using an alt account.
  • Yes, tested with a new matrix.org account
  • Yes, both e2e and non-e2e rooms
  • Yes, both group and DM rooms
  • Yes, it needs to be someone else's message
  • No, it does not need a preview

@jryans jryans self-assigned this Jun 28, 2019
@jryans
Copy link
Collaborator

jryans commented Jul 2, 2019

This occurs while the reaction is in local echo and clears up once we receive the remote echo.

Possible solutions include one or more of:

  • Teach doesRoomHaveUnreadMessages about the pending event list and set unread to false if there are pending events
  • Include reactions in shouldHideEvent
  • Ban reactions from triggering in eventTriggersUnreadCount

(We really need to de-duplicate the N different interconnected functions for show / hide / trigger. See also #9948.)

jryans added a commit to matrix-org/matrix-react-sdk that referenced this issue Jul 3, 2019
This changes the room unread logic to mark any room with pending events as read,
under the assumption that you are active in the room. This also ensures that
local echoes of pending events do not cause rooms to temporarily appear unread.

Fixes element-hq/element-web#9952
@jryans
Copy link
Collaborator

jryans commented Jul 4, 2019

I now have more detail about what's going wrong here:

  1. We recently added support to local echo reactions
  2. We also changed the read receipt behaviour to advance past hidden events like reactions
  3. It can happen that your read receipt advances past a pending reaction
  4. The unread state partly relies on the read receipt, but (correctly) doesn't expect it to descend into pending events

So, my new approach is to ensure the read receipt cannot advance into pending events.

jryans added a commit to matrix-org/matrix-js-sdk that referenced this issue Jul 5, 2019
This changes the methods that update the read marker and read receipts to
prevent advancing into pending events.

Part of element-hq/element-web#9952
jryans added a commit to matrix-org/matrix-react-sdk that referenced this issue Jul 5, 2019
This changes the `TimelinePanel` to track live events (that have committed to
the server and been remote echoed) as well as the full list of events (which
includes pending events).

The code paths that advance read receipt and read markers are then changed to
only use the live events so that these cannot advance into pending events.

Fixes element-hq/element-web#9952
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment