Refactor updating note content to stop directly mutating note object (#…

…1634)

See #1614

As part of a broader effort to resolve data-flow issues in the app this PR is a
first step in removing direct mutation where transactional atomic updates
should be occurring.

It's not clear if the existing code is the source of existing defects in the software
and this is part of why the code is problematic; we have created inherent
concurrency flaws that open up extremely-difficult-to-reproduce bugs.

Resolving this may or may not resolve any existing bugs but it will definitely
help guard us from introducing new ones.

---

Previously we have been directly mutating the note object when updating
its content. This may have been an attempt to work around confusing
data-flow issues that thankfully don't exist anymore. We have also been
performing inline checks to make sure that we update the editor's
contents if we receive these updates.

This mutation can lead to concurrency defects which expose themselves as
inconsistent UI state. This breaks our Redux model which assumes that
all UI updates happen atomically.

In this patch we're building a new note object when we update a note
in order to maintain our consistency. In light of #1598 we're also
removing some work-around code that attempted to force consistency when
it didn't exist; that consistency now exists since we're tracking the
underlying Simperium data closely now vs. storing it in separate
places.

When updating checklist items we're forcing a sync so that those changes
will propagate immediately. We don't have a need to debounce those
clicks.

lib/app.jsx

@@ -320,12 +320,28 @@ export const App = connect(
       });
     };
 
-    onUpdateContent = (note, content) =>
-      this.props.actions.updateNoteContent({
-        noteBucket: this.props.noteBucket,
-        note,
-        content,
-      });
+    onUpdateContent = (note, content) => {
+      if (!note) {
+        return;
+      }
+
+      const updatedNote = {
+        ...note,
+        data: {
+          ...note.data,
+          content,
+          modificationDate: Math.floor(Date.now() / 1000),
+        },
+      };
+
+      // update the bucket but don't force sync right away
+      // as this happens per keystroke when the user is editing
+      // a note. The NoteEditor will notify via props when
+      // it's time to sync via Simperium
+      const { noteBucket } = this.props;
+
+      noteBucket.update(note.id, updatedNote.data, {}, { sync: false });
+    };
 
     syncNote = noteId => {
       this.props.noteBucket.touch(noteId);

lib/flux/app-state.js

@@ -404,8 +404,6 @@ export const actionMap = new ActionMap({
             return;
           }
 
-          state.note.data = data;
-
           dispatch(
             this.action('loadAndSelectNote', {
               noteBucket,
@@ -439,28 +437,6 @@ export const actionMap = new ActionMap({
       },
     },
 
-    updateNoteContent: {
-      creator({ noteBucket, note, content }) {
-        return (dispatch, getState) => {
-          if (note) {
-            note.data.content = content;
-            note.data.modificationDate = Math.floor(Date.now() / 1000);
-
-            // update the bucket don't sync right away
-            // as this happens per keystroke when the user is editing
-            // a note. The NoteEditor notify via props when its time
-            // to sync via Simperium
-            noteBucket.update(note.id, note.data, {}, { sync: false });
-
-            // Check if note is still selected (to avoid race conditions)
-            if (get(getState().appState, 'note.id') === note.id) {
-              dispatch(this.action('selectNote', { note }));
-            }
-          }
-        };
-      },
-    },
-
     trashNote: {
       creator({ noteBucket, note, previousIndex }) {
         return dispatch => {

lib/note-detail/index.jsx

@@ -33,7 +33,6 @@ export class NoteDetail extends Component {
     spellCheckEnabled: PropTypes.bool.isRequired,
     storeFocusEditor: PropTypes.func,
     storeHasFocus: PropTypes.func,
-    updateNoteContent: PropTypes.func.isRequired,
   };
 
   static defaultProps = {
@@ -130,25 +129,25 @@ export class NoteDetail extends Component {
   hasFocus = () => this.editorHasFocus && this.editorHasFocus();
 
   onPreviewClick = event => {
+    const { note, onChangeContent, syncNote } = this.props;
+
     for (let node = event.target; node !== null; node = node.parentNode) {
       // open markdown preview links in a new window
       if (node.tagName === 'A') {
         event.preventDefault();
         viewExternalUrl(node.href);
         break;
       }
+
       // handle task list items
       if (node.className === 'task-list-item') {
         event.preventDefault();
-        const { note, noteBucket, updateNoteContent } = this.props;
-        toggleTask({
-          taskNode: node,
-          text: note.data.content,
-        })
-          .then(newNoteContent => {
-            updateNoteContent({ noteBucket, note, content: newNoteContent });
-          })
-          .catch(console.log);
+        toggleTask({ taskNode: node, text: note.data.content }).then(
+          newContent => {
+            onChangeContent(note, newContent);
+            syncNote(note.id);
+          }
+        );
         break;
       }
     }
@@ -258,11 +257,10 @@ const mapStateToProps = ({ appState: state, settings }) => ({
   spellCheckEnabled: settings.spellCheckEnabled,
 });
 
-const { setShouldPrintNote, updateNoteContent } = appState.actionCreators;
+const { setShouldPrintNote } = appState.actionCreators;
 
 const mapDispatchToProps = {
   onNotePrinted: () => setShouldPrintNote({ shouldPrint: false }),
-  updateNoteContent,
 };
 
 export default connect(