Page MenuHomePhabricator

[lib] Check the timestamps when changing thread settings
ClosedPublic

Authored by tomek on Aug 30 2024, 10:17 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 2:57 PM
Unknown Object (File)
Fri, Nov 22, 6:25 AM
Unknown Object (File)
Fri, Nov 22, 12:35 AM
Unknown Object (File)
Sun, Nov 10, 5:20 AM
Unknown Object (File)
Sat, Nov 9, 7:52 PM
Unknown Object (File)
Thu, Nov 7, 8:35 PM
Unknown Object (File)
Thu, Nov 7, 7:34 PM
Unknown Object (File)
Thu, Nov 7, 7:33 PM
Subscribers

Details

Summary

Only update properties when they are older than the operation that is being processed.

https://linear.app/comm/issue/ENG-9118/update-changethreadsettingsspec

Depends on D13214

Test Plan

Performed 3 operations in a row:

  1. Thread creation with timestamp T
  2. Thread name change with timestamp T + 2
  3. Thread name and color change with timestamp T + 1

Made sure that thread color matched the value from 3 and name matched the value from 2.

Diff Detail

Repository
rCOMM Comm
Branch
timestamps
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/shared/dm-ops/change-thread-settings-spec.js
106

You removed the ?, but threadInfos could be missing an entry for threadID, right?

129

What if timestamps are equal? Should we have some conflict resolution mechanism?

lib/shared/dm-ops/change-thread-settings-spec.js
106

In theory, it can, but in practice, it shouldn't because canBeProcessed would return missing_thread in such a case.

129

It is unlikely to have a conflict. Also, we have some more likely issues, e.g. https://linear.app/comm/issue/ENG-9125/membership-list-can-become-inconsistent-in-thick-threads. Created a task to track https://linear.app/comm/issue/ENG-9159/thick-threads-timestamps-conflict-resolution but we can't prioritize it before launch.

lib/shared/dm-ops/change-thread-settings-spec.js
106

Where is canBeProcessed used that would stop this?

lib/shared/dm-ops/change-thread-settings-spec.js
106
lib/shared/dm-ops/change-thread-settings-spec.js
106

I don't think this is the best practice in general to depend on code run outside of the function, since someone could call this function somewhere else. But in this case iI think t's fine, if you really don't want to change it

This revision is now accepted and ready to land.Sep 6 2024, 5:03 AM
lib/shared/dm-ops/change-thread-settings-spec.js
106

Makes sense!

lib/shared/dm-ops/change-thread-settings-spec.js
141–148 ↗(On Diff #43931)

If no changes end up being applied, is there a way we can skip this update, instead of propagating an empty update?