From 3a31728f55beab36eac81864691cc4c7ca19214a Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Sun, 13 Oct 2019 16:28:43 -0700 Subject: [PATCH] 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 | 28 ++++++++++++++++++++++------ lib/flux/app-state.js | 24 ------------------------ lib/note-detail/index.jsx | 22 ++++++++++------------ 3 files changed, 32 insertions(+), 42 deletions(-) diff --git a/lib/app.jsx b/lib/app.jsx index 6fb0ca6b9..b9e8dfbd7 100644 --- a/lib/app.jsx +++ b/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); diff --git a/lib/flux/app-state.js b/lib/flux/app-state.js index 76218d4d0..39377d95b 100644 --- a/lib/flux/app-state.js +++ b/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 => { diff --git a/lib/note-detail/index.jsx b/lib/note-detail/index.jsx index dc1f01ae8..f5ba5a9ec 100644 --- a/lib/note-detail/index.jsx +++ b/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,6 +129,8 @@ 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') { @@ -137,18 +138,16 @@ export class NoteDetail extends Component { 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(