Sync note after restoration to update the state #1774
Conversation
After restoring a note the editor was stuck o the selected revision. This PR clears that revision from the flux state. Additionally, that restoration of a note was not causing a sync to occur. This triggers a note sync that updates everything and cements the restored version as the current version.
| const { noteBucket } = this.props; | ||
|
|
||
| noteBucket.update(note.id, updatedNote.data, {}, { sync: false }); | ||
| noteBucket.update(note.id, updatedNote.data, {}, { sync }); |
There was a problem hiding this comment.
I would think passing true here to sync would cause a sync but it doesn't.
There was a problem hiding this comment.
What indicators are telling you that it isn't?
If it's not then we're probably seeing some other problem.
Also, since we can't get rid of syncNote() anyway where do you see the tradeoffs between adding the sync param here and calling syncNote() in the revision-selector?
There was a problem hiding this comment.
What indicators are telling you that it isn't?
The note list isn't updating, if you refresh before adding more text to the note it resets back to before the revision was restored.
Also, since we can't get rid of syncNote() anyway where do you see the tradeoffs between adding the sync param here and calling syncNote() in the revision-selector?
I'm not really sure what you are asking.
There was a problem hiding this comment.
The note list isn't updating, if you refresh before adding more text to the note it resets back to before the revision was restored.
This could go back to our funny data flow. Simperium operations really ought to be watching app operations and updating independently as side-effects, likewise should probably call some app action to update a note. It's not clear throughout who owns the data - Simperium, or our local copy in app state.
I'm not really sure what you are asking.
In this PR we're adding a new boolean parameter to onUpdateContent but another function already exists to do what we want: this.syncNote()
My question was why we choose to do that instead of call syncNote() when restoring a revision. I can make a guess that it's because we'd have to pass syncNote() down the component tree which would involve more work but I didn't know if you had another conclusion I didn't see.
There was a problem hiding this comment.
This could go back to our funny data flow. Simperium operations really ought to be watching app operations and updating independently as side-effects, likewise should probably call some app action to update a note. It's not clear throughout who owns the data - Simperium, or our local copy in app state.
I don't think we can address this in this PR. I'd really like to get this in so the app isn't broken.
My question was why we choose to do that instead of call syncNote() when restoring a revision. I can make a guess that it's because we'd have to pass syncNote() down the component tree which would involve more work but I didn't know if you had another conclusion I didn't see.
- I didn't want to have to pass it down
- The whole pass it down thing is a pretty terrible practice in general
- The revision selector component shouldn't really have to worry about what to do when a revision is restored. I didn't want to add additional complexity down stream.
There was a problem hiding this comment.
- I didn't want to have to pass it down
- The whole pass it down thing is a pretty terrible practice in general
I agree, though I've been staying in line with our practice of passing things down for the sake of consistency until we get a chance to do real refactoring in the app. I personally want to see these pass-downs all replaced with predictable and semantically-oriented Redux action calls so that we can get rid of the real culprit - complicated interplay with side-effects.
- The revision selector component shouldn't really have to worry about what to do when a revision is restored. I didn't want to add additional complexity down stream.
Given the way the app works I don't agree as much on this one. The tags, for example, immediately sync once we add them or remove them. The editor is the odd-ball maybe and perhaps it should hold the exception, but there's good reason in my mind to make an explicit "sync now" when replacing the editor text with a revision vs "sync soon" when editing text directly.
If it were me writing this PR I think that I'd pass down syncNote() and that's about a 65%-75% confidence. It's not me though and I have no strong objection here, meaning that I support your choices. Mainly I wanted to get on the same page and talk about it, because it stood out to me as a bit unexpected here (plus it's introducing a new boolean function argument which I like to avoid).
Don't think too much about it.
There was a problem hiding this comment.
In <NoteDetail /> we see where we control syncNote() separate to enforce an update after checking a todo item. It's not inconsistent with this PR so it'd be good if we either updated the PR code to match the existing pattern in the codebase or updated the toggleTask() so match the new patterns introduced here.
|
Merging. We can revisit if need be. |
Fix
Fixes #1772
After restoring a note the editor was stuck o the selected revision. This PR
clears that revision from the flux state. Additionally, that restoration of a
note was not causing a sync to occur. This triggers a note sync that updates
everything and cements the restored version as the current version.
Test
Review
Only one developer is required to review these changes, but anyone can perform the review.
Release
RELEASE-NOTES.txtwas updated with: