Page MenuHomePhabricator
Differential D13689

[lib] Merge consecutive membership robotext messages
ClosedPublic
Actions

Authored by ashoat on Oct 9 2024, 8:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 9:21 AM
Unknown Object (File)
Sun, Nov 17, 10:20 AM
Unknown Object (File)
Wed, Nov 13, 6:40 AM
Unknown Object (File)
Wed, Nov 13, 6:35 AM
Unknown Object (File)
Tue, Nov 12, 11:34 PM
Unknown Object (File)
Tue, Nov 12, 11:32 PM
Unknown Object (File)
Sun, Nov 10, 6:06 PM
Unknown Object (File)
Sun, Nov 10, 10:45 AM
View All 57 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.Oct 9 2024, 8:35 PM
ashoat edited the test plan for this revision. (Show Details)Oct 9 2024, 8:36 PM
ashoat added a comment.Oct 9 2024, 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.Oct 9 2024, 10:21 PM
ashoat requested review of this revision.Oct 9 2024, 10:21 PM
tomek accepted this revision.Oct 11 2024, 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.Oct 11 2024, 7:58 AM
ashoat updated this revision to Diff 45106.Oct 13 2024, 5:41 PM

Use ^ as separator instead of |

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

Undo last update and address feedback

ashoat added inline comments.Oct 13 2024, 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!Oct 13 2024, 5:54 PM
Harbormaster failed remote builds in B32211: Diff 45107!Oct 13 2024, 6:03 PM
ashoat updated this revision to Diff 45110.Oct 13 2024, 6:35 PM

Rebase

This revision was landed with ongoing or failed builds.Oct 13 2024, 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.Oct 13 2024, 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...