Only send reaction update notification on change (#13234)

keybase · Aug 10, 2018 · 6f9358d · 6f9358d
1 parent f759f48
commit 6f9358d
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 29 deletions.
diff --git a/go/chat/server_test.go b/go/chat/server_test.go
@@ -2218,11 +2218,6 @@ func TestChatSrvPostLocalNonblock(t *testing.T) {
 				require.Fail(t, "no event received")
 			}
 			consumeNewMsgLocal(t, listener, chat1.MessageType_DELETE)
-			switch mt {
-			case chat1.ConversationMembersType_KBFS:
-			default:
-				assertReactionUpdate(created.Id, textUnboxed.GetMessageID(), expectedReactionMap)
-			}
 			assertReactionUpdate(created.Id, textUnboxed.GetMessageID(), chat1.ReactionMap{})
 
 			t.Logf("delete the message")

diff --git a/go/chat/storage/storage.go b/go/chat/storage/storage.go
@@ -475,38 +475,43 @@ func (s *Storage) updateAllSupersededBy(ctx context.Context, convID chat1.Conver
 				s.Debug(ctx, "updateSupersededBy: writing: id: %d superseded: %d", msgid, supersededID)
 				mvalid := superMsg.Valid()
 
-				// reactions don't update SupersededBy, instead they rely on
-				// ReactionIDs
-				if msg.GetMessageType() == chat1.MessageType_REACTION {
-					mvalid.ServerHeader.ReactionIDs = s.updateReactionIDs(mvalid.ServerHeader.ReactionIDs, msgid)
-				} else {
+				newMsgs := []chat1.MessageUnboxed{}
+				switch msg.GetMessageType() {
+				case chat1.MessageType_REACTION:
+					// If we haven't modified any reaction data, we don't want
+					// to send it up for a notification.
+					reactionUpdate := false
+					// reactions don't update SupersededBy, instead they rely
+					// on ReactionIDs
+					mvalid.ServerHeader.ReactionIDs, reactionUpdate = s.updateReactionIDs(mvalid.ServerHeader.ReactionIDs, msgid)
+					newMsg := chat1.NewMessageUnboxedWithValid(mvalid)
+					newMsgs = append(newMsgs, newMsg)
+					if reactionUpdate {
+						updatedReactionTargets[superMsg.GetMessageID()] = newMsg
+					}
+				case chat1.MessageType_DELETE:
 					mvalid.ServerHeader.SupersededBy = msgid
-				}
-
-				var newMsgs []chat1.MessageUnboxed
-				if msg.GetMessageType() == chat1.MessageType_DELETE {
 					// We have to find the message we are reacting to and
 					// update it's ReactionIDs as well.
 					if superMsg.GetMessageType() == chat1.MessageType_REACTION {
-						newTargetMsg, err := s.updateReactionTargetOnDelete(ctx, convID, uid, superMsg)
+						newTargetMsg, reactionUpdate, err := s.updateReactionTargetOnDelete(ctx, convID, uid, superMsg)
 						if err != nil {
 							return nil, err
 						} else if newTargetMsg != nil {
-							updatedReactionTargets[newTargetMsg.GetMessageID()] = *newTargetMsg
+							if reactionUpdate {
+								updatedReactionTargets[newTargetMsg.GetMessageID()] = *newTargetMsg
+							}
 							newMsgs = append(newMsgs, *newTargetMsg)
 						}
 					}
 					msgPurged, assets := s.purgeMessage(mvalid)
 					allAssets = append(allAssets, assets...)
 					newMsgs = append(newMsgs, msgPurged)
-				} else {
+				default:
+					mvalid.ServerHeader.SupersededBy = msgid
 					newMsg := chat1.NewMessageUnboxedWithValid(mvalid)
 					newMsgs = append(newMsgs, newMsg)
-					if msg.GetMessageType() == chat1.MessageType_REACTION {
-						updatedReactionTargets[superMsg.GetMessageID()] = newMsg
-					}
 				}
-
 				if err = s.engine.WriteMessages(ctx, convID, uid, newMsgs); err != nil {
 					return nil, err
 				}
@@ -993,29 +998,29 @@ func (s *Storage) getMessage(ctx context.Context, convID chat1.ConversationID, u
 
 // updateReactionIDs appends `msgid` to `reactionIDs` if it is not already
 // present.
-func (s *Storage) updateReactionIDs(reactionIDs []chat1.MessageID, msgid chat1.MessageID) []chat1.MessageID {
+func (s *Storage) updateReactionIDs(reactionIDs []chat1.MessageID, msgid chat1.MessageID) ([]chat1.MessageID, bool) {
 	for _, reactionID := range reactionIDs {
 		if reactionID == msgid {
-			return reactionIDs
+			return reactionIDs, false
 		}
 	}
-	return append(reactionIDs, msgid)
+	return append(reactionIDs, msgid), true
 }
 
 // updateReactionTargetOnDelete modifies the reaction's target message when the
 // reaction itself is deleted
 func (s *Storage) updateReactionTargetOnDelete(ctx context.Context, convID chat1.ConversationID,
-	uid gregor1.UID, reactionMsg *chat1.MessageUnboxed) (*chat1.MessageUnboxed, Error) {
+	uid gregor1.UID, reactionMsg *chat1.MessageUnboxed) (*chat1.MessageUnboxed, bool, Error) {
 	s.Debug(ctx, "updateReactionTargetOnDelete: reationMsg: %v", reactionMsg)
 
 	if reactionMsg.Valid().MessageBody.IsNil() {
-		return nil, nil
+		return nil, false, nil
 	}
 
 	targetMsgID := reactionMsg.Valid().MessageBody.Reaction().MessageID
 	targetMsg, err := s.getMessage(ctx, convID, uid, targetMsgID)
 	if err != nil || targetMsg == nil {
-		return nil, err
+		return nil, false, err
 	}
 	if targetMsg.IsValid() {
 		mvalid := targetMsg.Valid()
@@ -1025,11 +1030,12 @@ func (s *Storage) updateReactionTargetOnDelete(ctx context.Context, convID chat1
 				reactionIDs = append(reactionIDs, msgID)
 			}
 		}
+		updated := len(mvalid.ServerHeader.ReactionIDs) != len(reactionIDs)
 		mvalid.ServerHeader.ReactionIDs = reactionIDs
 		newMsg := chat1.NewMessageUnboxedWithValid(mvalid)
-		return &newMsg, nil
+		return &newMsg, updated, nil
 	}
-	return nil, nil
+	return nil, false, nil
 }
 
 // Clears the body of a message and returns any assets to be deleted.