Skip to content

Commit

Permalink
Refactor note tag operation to stop directly mutating note object
Browse files Browse the repository at this point in the history
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 note objects when editing
their tags. 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 new note objects when we make these updates
in order to maintain our consistency.

---

There should be no significant visual or behavioral changes with this PR. We
are changing code related to removing tags, renaming tags, and
reordering tags.

In testing verify that with separate sessions the updates appear as expected.
Add, reorder, and remove tags to make sure the changes synchronize.
  • Loading branch information
dmsnell committed Oct 8, 2019
1 parent ec12a92 commit 4200928
Showing 1 changed file with 15 additions and 23 deletions.
38 changes: 15 additions & 23 deletions lib/state/domain/notes.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,33 +3,25 @@ import appState from '../../flux/app-state';
import isEmailTag from '../../utils/is-email-tag';
import { createTag } from './tags';

const { selectNote } = appState.actionCreators;

export const updateNoteTags = ({ note, tags }) => {
return (dispatch, getState) => {
if (note) {
let state = getState().appState;

note.data.tags = tags;
note.data.modificationDate = Math.floor(Date.now() / 1000);
noteBucket().update(note.id, note.data);

dispatch(selectNote({ note }));

let currentTagNames = state.tags.map(tag => tag.data.name);
for (let i = 0; i < tags.length; i++) {
let tag = tags[i];
if (!note) {
return;
}

if (currentTagNames.indexOf(tag) !== -1) {
continue;
}
noteBucket().update(note.id, {
...note.data,
tags,
modificationDate: Math.floor(Date.now() / 1000),
});

if (isEmailTag(tag)) {
continue;
}
const existingTagNames = new Set(
getState().appState.tags.map(tag => tag.data.name)
);

createTag({ name: tag });
}
}
tags
.filter(name => !existingTagNames.has(name))
.filter(name => !isEmailTag(name))
.forEach(name => createTag({ name }));
};
};

0 comments on commit 4200928

Please sign in to comment.