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)
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
Unknown Object (File)
Thu, Nov 7, 5:24 PM
Unknown Object (File)
Tue, Oct 29, 11:10 PM
Unknown Object (File)
Sun, Oct 27, 4:22 AM
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
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/shared/dm-ops/change-thread-settings-spec.js
106 ↗(On Diff #43821)

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

129 ↗(On Diff #43821)

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

lib/shared/dm-ops/change-thread-settings-spec.js
106 ↗(On Diff #43821)

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

129 ↗(On Diff #43821)

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

Where is canBeProcessed used that would stop this?

lib/shared/dm-ops/change-thread-settings-spec.js
106 ↗(On Diff #43821)
lib/shared/dm-ops/change-thread-settings-spec.js
106 ↗(On Diff #43821)

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

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?