Page MenuHomePhabricator
Differential D13689

[lib] Merge consecutive membership robotext messages
ClosedPublic
Actions

Authored by ashoat on Wed, Oct 9, 8:35 PM.
Tags
None
Referenced Files
F3102036: D13689.diff
Wed, Oct 30, 10:21 AM
F3097658: D13689.id45130.diff
Tue, Oct 29, 3:45 PM
F3097417: D13689.diff
Tue, Oct 29, 3:11 PM
Unknown Object (File)
Wed, Oct 23, 8:34 PM
Unknown Object (File)
Wed, Oct 23, 9:16 AM
Unknown Object (File)
Wed, Oct 23, 8:48 AM
Unknown Object (File)
Wed, Oct 23, 2:39 AM
Unknown Object (File)
Wed, Oct 23, 12:47 AM
View All 38 Files
Subscribers
None

Details

Reviewers
tomek
Commits
rCOMM038702514e91: [lib] Merge consecutive membership robotext messages
Summary

This reduces a bunch of noise in community roots when people get auto-added to the communities.

Depends on D13688

Test Plan

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/plural_join_thread
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Wed, Oct 9, 8:35 PM
ashoat edited the test plan for this revision. (Show Details)Wed, Oct 9, 8:36 PM
ashoat added a comment.Wed, Oct 9, 8:38 PM

Note that the extra vertical spacing in the test video is caused by ENG-2928, and is unrelated to the work here

Harbormaster completed remote builds in B32173: Diff 45048.Wed, Oct 9, 10:21 PM
ashoat requested review of this revision.Wed, Oct 9, 10:21 PM
tomek accepted this revision.Fri, Oct 11, 7:58 AM
tomek added inline comments.
lib/shared/messages/join-thread-message-spec.js
158–170 ↗(On Diff #45048)

All but one generated robotext messages will be discarded while merging the messages, which means that we're performing O(n^2) operations where n is a number of consecutive join thread messages. This isn't a big issue unless we expect a huge number of thread joins.

lib/shared/messages/remove-members-message-spec.js
39 ↗(On Diff #45048)
This revision is now accepted and ready to land.Fri, Oct 11, 7:58 AM
ashoat updated this revision to Diff 45106.Sun, Oct 13, 5:41 PM

Use ^ as separator instead of |

ashoat updated this revision to Diff 45107.Sun, Oct 13, 5:50 PM

Undo last update and address feedback

ashoat added inline comments.Sun, Oct 13, 5:52 PM
lib/shared/messages/join-thread-message-spec.js
158–170 ↗(On Diff #45048)

Makes sense. I don't think it should be a big concern, and I didn't notice any performance issues while testing

Harbormaster failed remote builds in B32210: Diff 45106!Sun, Oct 13, 5:54 PM
Harbormaster failed remote builds in B32211: Diff 45107!Sun, Oct 13, 6:03 PM
ashoat updated this revision to Diff 45110.Sun, Oct 13, 6:35 PM

Rebase

This revision was landed with ongoing or failed builds.Sun, Oct 13, 6:44 PM
Closed by commit rCOMM038702514e91: [lib] Merge consecutive membership robotext messages (authored by ashoat). · Explain Why
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMM038702514e91: [lib] Merge consecutive membership robotext messages.
Harbormaster completed remote builds in B32214: Diff 45110.Sun, Oct 13, 6:52 PM

Revision Contents
Changeset List

Diff 45110

View Options

lib/shared/messages/add-members-message-spec.js

Loading...
View Options

lib/shared/messages/join-thread-message-spec.js

Loading...
View Options

lib/shared/messages/leave-thread-message-spec.js

Loading...
View Options

lib/shared/messages/remove-members-message-spec.js

Loading...