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
Unknown Object (File)
Sat, Nov 9, 5:57 PM
Unknown Object (File)
Sat, Nov 9, 2:58 PM
Unknown Object (File)
Sat, Nov 9, 2:44 PM
Unknown Object (File)
Sat, Nov 9, 2:37 PM
Unknown Object (File)
Sat, Nov 9, 2:37 PM
Unknown Object (File)
Sat, Nov 9, 10:59 AM
Unknown Object (File)
Sat, Nov 9, 7:40 AM
Unknown Object (File)
Oct 22 2024, 6:29 PM
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
Branch
timestamps
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/shared/dm-ops/process-dm-ops.js
247–275

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 ↗(On Diff #43939)

What's the context on this change?

lib/shared/dm-ops/process-dm-ops.js
199 ↗(On Diff #43939)

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 ↗(On Diff #43939)

That makes sense – thanks for explaining!