Fix stuck unread badge
Fix stuck unread badge
Fix https://gitlab.com/gitlab-org/gitter/webapp/-/issues/2388
Other MRs spawned from this one:
- https://gitlab.com/gitlab-org/gitter/webapp/-/merge_requests/1872
- https://gitlab.com/gitlab-org/gitter/webapp/-/merge_requests/1873
- https://gitlab.com/gitlab-org/gitter/webapp/-/merge_requests/1874
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
- Have two separate clients open with different users signed in
- Visit the same room
- User 1 sends a message (realtime
create
operation) - User 1 edits the message (realtime
update
operation to updatetext
/html
) - User 2 now has a stuck unread badge and clicking has no effect
Threads
- Have two separate clients open with different users signed in
- Visit the same room
- User 1 sends a message (realtime
create
operation) - User 1 sends a threaded message for that message above (realtime
update
operation for the first message to updatethreadMessageCount
) - 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:
- Normal chat
- NLI chat
- Archive (just ensure nothing appears as unread)
Regression
Things regressed when we started trying to support unreads with threaded conversations. Git history stack:
-
❌ 5 months ago, defaulted tounread: false
-> https://gitlab.com/gitlab-org/gitter/webapp/-/merge_requests/1661 (https://gitlab.com/gitlab-org/gitter/webapp/-/commit/b3757db8f8a446f493b0b9d59afc65383ca1d935#599467d575c29c2e3f0a16206d775b119311d8c0_128_114) -
❌ 5 months ago, defaulted tounread: false
-> https://gitlab.com/gitlab-org/gitter/webapp/-/commit/bb31ef5a4b9454875ed7c39b3410c26a5a2ce021#599467d575c29c2e3f0a16206d775b119311d8c0_75_74 -
❌ 5 months ago, defaulted tounread: true
-> https://gitlab.com/gitlab-org/gitter/webapp/-/commit/f6934cc8b8080903ccd68a1bc1a681b646fdb14b#599467d575c29c2e3f0a16206d775b119311d8c0_63_74 -
✔ 5 years ago, defaultedunread: undefined
-> https://gitlab.com/gitlab-org/gitter/webapp/-/commit/92e283d26293c8a7163c94c6261650f55ed29b47#599467d575c29c2e3f0a16206d775b119311d8c0_49_51
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}
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'