Page MenuHomePhabricator

[lib] Check the timestamps when updating read status for all the specs
ClosedPublic

Authored by tomek on Sep 4 2024, 9:58 AM.
Tags
None
Referenced Files
F3530778: D13241.id.diff
Wed, Dec 25, 4:24 AM
Unknown Object (File)
Wed, Dec 18, 6:19 PM
Unknown Object (File)
Wed, Dec 18, 6:19 PM
Unknown Object (File)
Wed, Dec 18, 6:19 PM
Unknown Object (File)
Wed, Dec 18, 6:19 PM
Unknown Object (File)
Wed, Dec 18, 6:04 PM
Unknown Object (File)
Wed, Dec 18, 4:41 PM
Unknown Object (File)
Nov 25 2024, 7:02 AM
Subscribers

Details

Summary

Only update the status when it is more recent than the saved timestamp.

https://linear.app/comm/issue/ENG-9112/handle-unread-status

Depends on D13234

Test Plan

Process a couple of operations with text messages and check if the unread status and timestamps are correct.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/shared/dm-ops/process-dm-ops.js
247–275 ↗(On Diff #43885)

This is to satisfy Flow.

tomek requested review of this revision.Sep 4 2024, 10:16 AM
This revision is now accepted and ready to land.Sep 5 2024, 12:48 AM

Don't override the timestamps

lib/shared/dm-ops/process-dm-ops.js
199

What's the context on this change?

lib/shared/dm-ops/process-dm-ops.js
199

When we process the operation, we create a map updatedThreadInfosByThreadID with the most recent thread infos. We need this map so that we can correctly create UPDATE_THREAD update that don't override changes made by the specs. UPDATE_THREAD update overrides completely a thread in the store, with its content.

Before this diff, we were using UPDATE_THREAD to update the replies count, which was only affecting the sidebars - that's why this condition existed. For the unread status, we were using UPDATE_THREAD_READ_STATUS which doesn't override the thread and updates only one of its properties.

In this diff, we're replacing UPDATE_THREAD_READ_STATUS with UPDATE_THREAD for all the other threads, which means that updatedThreadInfosByThreadID map has to include all the threads.

lib/shared/dm-ops/process-dm-ops.js
199

That makes sense – thanks for explaining!