Skip to content

Fix stuck unread badge

Eric Eastwood requested to merge 2388-fix-stuck-unread-badge into develop

Fix stuck unread badge

Fix https://gitlab.com/gitlab-org/gitter/webapp/-/issues/2388

Other MRs spawned from this one:

Problem description

I think our assumption before was that if there wasn't any currentUserId option defined for ChatStrategy(options), then the user was NLI and we don't track unreads for NLI users so we set unread: false as a default. But currentUserId is not defined for the realtime updates(live-collection-chats.js) when someone is signed in.

Because of the nature of live-collection-chats.js(many users are subscribed to the same realtime channel and channel not unique to user), we can't define the currentUserId option for the ChatStrategy(options). If we use a default, then that can overwrite the value clientside for the signed in user and cause bugs like https://gitlab.com/gitlab-org/gitter/webapp/-/issues/2388

On the client, we set the unread status for each message with the unread-items-client which patches the chat collection when it is first added (create operation). But if we get another realtime update operation, do we trust the realtime update or what we already know on the client? Currently, we use the realtime update because it should hold the latest accurate data. And we run into problems if this has defaulted inaccurate state.

Now in this MR, we set unread: undefined so it's not included in the realtime create/update operations.

Ways to reproduce the bug

Anything that causes an update to the message after you create it will reproduce this bug.

Editing a message

  1. Have two separate clients open with different users signed in
  2. Visit the same room
  3. User 1 sends a message (realtime create operation)
  4. User 1 edits the message (realtime update operation to update text/html)
  5. User 2 now has a stuck unread badge and clicking has no effect

Threads

  1. Have two separate clients open with different users signed in
  2. Visit the same room
  3. User 1 sends a message (realtime create operation)
  4. User 1 sends a threaded message for that message above (realtime update operation for the first message to update threadMessageCount)
  5. User 2 now has a stuck unread badge and clicking has no effect

Testing strategy

Go through one of the reproduction instructions above. Ensure things look as expected for each scenario:

  1. Normal chat
  2. NLI chat
  3. Archive (just ensure nothing appears as unread)

Regression

Things regressed when we started trying to support unreads with threaded conversations. Git history stack:

Dev notes

At first I investigated the frontend and how messages were getting marked as read. Because I could see the messages being marked as read on the client but then refreshing showed them as unread again. The unread badge still showed the messages as being unread accurately. Clicking on the badge has no effect on marking the messages as read though. Just have to refresh to get in a good state.

The purple highlighted messages in the below gif is just some visual debugging I did around what the TroupeUnreadItemsViewportMonitor considered to be in view. Nothing seemed to be out of order there.

Then I dug more into the frontend state and store of things.

You can see that when we send the first 1 message, it is marked as unread=true in the other window as expected. But then when we send another threaded message 2 under that parent 1 message it gets marked as unread=false(unexpected). We haven't actually focused or seen the message in the other window.

On the realtime connection, I noticed that all of the /chatMessages come in as unread: false when created or updated. Which is probably what is throwing a lot of things off.

The reason the stuck unread thing is a problem with threads is because the create operation comes down like normal and our unread items client patches it properly. But then if someone replies to the thread, the parent message gets a update operation for the threadMessageCount: 1 but it also includes unread: false again and overwrites our collection with false information.

Now that I understand the problem more, I can also reproduce another way by sending a message, then editing it 😮

Realtime /bayeux downstream messages for create/update of parent message and thread child 
[{"channel":"/api/v1/rooms/5d04803fc350e0f4fce3f9a0/chatMessages","data":{"operation":"create","model":{"id":"5eb5fe08d629099d68cbf4bc","text":"1","html":"1","sent":"2020-05-09T00:49:12.529Z","fromUser":{"id":"5a87396f1543b98772a686a3","username":"MadLittleMods","displayName":"Eric Eastwood","url":"/MadLittleMods","avatarUrl":"http://localhost:5000/api/private/avatars/gh/uv/4/MadLittleMods","avatarUrlSmall":"https://avatars0.githubusercontent.com/u/558581?v=4&s=60","avatarUrlMedium":"https://avatars0.githubusercontent.com/u/558581?v=4&s=128","staff":true,"v":280,"gv":"4"},"unread":false,"readBy":0,"urls":[],"mentions":[],"issues":[],"meta":[],"v":1}},"id":"2tx","ext":{"c":15}}]
[{"channel":"/api/v1/rooms/5d04803fc350e0f4fce3f9a0/chatMessages","data":{"operation":"update","model":{"id":"5eb5fe08d629099d68cbf4bc","text":"1","html":"1","sent":"2020-05-09T00:49:12.529Z","fromUser":{"id":"5a87396f1543b98772a686a3","username":"MadLittleMods","displayName":"Eric Eastwood","url":"/MadLittleMods","avatarUrl":"http://localhost:5000/api/private/avatars/gh/uv/4/MadLittleMods","avatarUrlSmall":"https://avatars0.githubusercontent.com/u/558581?v=4&s=60","avatarUrlMedium":"https://avatars0.githubusercontent.com/u/558581?v=4&s=128","staff":true,"v":280,"gv":"4"},"threadMessageCount":1,"unread":false,"readBy":0,"urls":[],"mentions":[],"issues":[],"meta":[],"v":2}},"id":"2u3","ext":{"c":20}}]
[{"channel":"/api/v1/rooms/5d04803fc350e0f4fce3f9a0/chatMessages","data":{"operation":"create","model":{"id":"5eb5fe0bd629099d68cbf4be","text":"2","html":"2","sent":"2020-05-09T00:49:15.594Z","fromUser":{"id":"5a87396f1543b98772a686a3","username":"MadLittleMods","displayName":"Eric Eastwood","url":"/MadLittleMods","avatarUrl":"http://localhost:5000/api/private/avatars/gh/uv/4/MadLittleMods","avatarUrlSmall":"https://avatars0.githubusercontent.com/u/558581?v=4&s=60","avatarUrlMedium":"https://avatars0.githubusercontent.com/u/558581?v=4&s=128","staff":true,"v":280,"gv":"4"},"parentId":"5eb5fe08d629099d68cbf4bc","unread":false,"readBy":0,"urls":[],"mentions":[],"issues":[],"meta":[],"v":1}},"id":"2u5","ext":{"c":21}}]

Looking at the chat-strategy.js, it says:

// if there is no `currentUserId`, all the messages are going to have default {unread: false}

server/serializers/rest/chat-strategy.js#L42

And when I debug the currentUserId, it is undefined for the receiving user so it makes sense why it is always returning false for us.

node scripts/utils/delete-unreads-for-user-in-room.js --username EricGitterTester --room-uri MadLittleMods/trtrrtdshd
window.localStorage.debug = 'app:unread-banner-view,app:chat-collection-view,app:unread-items-client-store,app:unread-items-client,app:chat-collection,app:unread-items-behavior'
Edited by Eric Eastwood

Merge request reports