From a7bd21ccf24ddf3d74774d8ce600c4f24bcbe520 Mon Sep 17 00:00:00 2001
From: Christopher Chedeau <vjeuxx@gmail.com>
Date: Sat, 28 Mar 2020 21:25:40 -0700
Subject: [PATCH] Avoid broadcasting what was just received (#1116)

Fixes #1115

The issue is that replaceAllElements calls a render synchronously, preventing lastBroadcastedOrReceivedSceneVersion from being set correctly.

I tried using batchUpdate but it only takes a single argument ( https://github.com/facebook/react/blob/c5d2fc7127654e43de59fff865b74765a103c4a5/packages/react-reconciler/src/ReactFiberWorkLoop.js#L1088 ) whereas the callback takes two.

Test Plan:
- Add a console.log before `this.broadcastScene("SCENE_UPDATE");` in App.tsx
- Connect a bunch of clients
- Have one move a shape
- Make sure that this client has the console logged
- Make sure the other clients don't have it
---
 src/components/App.tsx | 89 ++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 42 deletions(-)

diff --git a/src/components/App.tsx b/src/components/App.tsx
index 22b10b3e..82cb3d32 100644
--- a/src/components/App.tsx
+++ b/src/components/App.tsx
@@ -731,58 +731,63 @@ export class App extends React.Component<any, AppState> {
           );
 
           // Reconcile
-          globalSceneState.replaceAllElements(
-            restoredState.elements
-              .reduce((elements, element) => {
-                // if the remote element references one that's currently
-                //  edited on local, skip it (it'll be added in the next
-                //  step)
-                if (
-                  element.id === this.state.editingElement?.id ||
-                  element.id === this.state.resizingElement?.id ||
-                  element.id === this.state.draggingElement?.id
-                ) {
-                  return elements;
-                }
+          const newElements = restoredState.elements
+            .reduce((elements, element) => {
+              // if the remote element references one that's currently
+              //  edited on local, skip it (it'll be added in the next
+              //  step)
+              if (
+                element.id === this.state.editingElement?.id ||
+                element.id === this.state.resizingElement?.id ||
+                element.id === this.state.draggingElement?.id
+              ) {
+                return elements;
+              }
 
+              if (
+                localElementMap.hasOwnProperty(element.id) &&
+                localElementMap[element.id].version > element.version
+              ) {
+                elements.push(localElementMap[element.id]);
+                delete localElementMap[element.id];
+              } else if (
+                localElementMap.hasOwnProperty(element.id) &&
+                localElementMap[element.id].version === element.version &&
+                localElementMap[element.id].versionNonce !==
+                  element.versionNonce
+              ) {
+                // resolve conflicting edits deterministically by taking the one with the lowest versionNonce
                 if (
-                  localElementMap.hasOwnProperty(element.id) &&
-                  localElementMap[element.id].version > element.version
+                  localElementMap[element.id].versionNonce <
+                  element.versionNonce
                 ) {
                   elements.push(localElementMap[element.id]);
-                  delete localElementMap[element.id];
-                } else if (
-                  localElementMap.hasOwnProperty(element.id) &&
-                  localElementMap[element.id].version === element.version &&
-                  localElementMap[element.id].versionNonce !==
-                    element.versionNonce
-                ) {
-                  // resolve conflicting edits deterministically by taking the one with the lowest versionNonce
-                  if (
-                    localElementMap[element.id].versionNonce <
-                    element.versionNonce
-                  ) {
-                    elements.push(localElementMap[element.id]);
-                  } else {
-                    // it should be highly unlikely that the two versionNonces are the same. if we are
-                    // really worried about this, we can replace the versionNonce with the socket id.
-                    elements.push(element);
-                  }
-                  delete localElementMap[element.id];
                 } else {
+                  // it should be highly unlikely that the two versionNonces are the same. if we are
+                  // really worried about this, we can replace the versionNonce with the socket id.
                   elements.push(element);
-                  delete localElementMap[element.id];
                 }
+                delete localElementMap[element.id];
+              } else {
+                elements.push(element);
+                delete localElementMap[element.id];
+              }
 
-                return elements;
-              }, [] as Mutable<typeof restoredState.elements>)
-              // add local elements that weren't deleted or on remote
-              .concat(...Object.values(localElementMap)),
+              return elements;
+            }, [] as Mutable<typeof restoredState.elements>)
+            // add local elements that weren't deleted or on remote
+            .concat(...Object.values(localElementMap));
+
+          // Avoid broadcasting to the rest of the collaborators the scene
+          // we just received!
+          // Note: this needs to be set before replaceAllElements as it
+          // syncronously calls render.
+          this.lastBroadcastedOrReceivedSceneVersion = getDrawingVersion(
+            newElements,
           );
+
+          globalSceneState.replaceAllElements(newElements);
         }
-        this.lastBroadcastedOrReceivedSceneVersion = getDrawingVersion(
-          globalSceneState.getAllElements(),
-        );
 
         // We haven't yet implemented multiplayer undo functionality, so we clear the undo stack
         // when we receive any messages from another peer. This UX can be pretty rough -- if you