Clean up frontend change tracker and snackbars related to undoing operations#3480
Conversation
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
@rtibbles Interested in your thoughts on this use of liveQuery. I added this to detect when copying is finished so we can show the snackbars appropriately.
1d9ea97 to
e7b7215
Compare
rtibbles
left a comment
There was a problem hiding this comment.
This looks like good cleanup - one comment about possible future directions for undoing, but I don't think it affects the code as is. Will manually test.
| return promiseChunk(this._changes.reverse(), 1, ([change]) => { | ||
| const table = db[change.table]; | ||
| return db.transaction('rw', change.table, () => { | ||
| // This source inherits from `IGNORED_SOURCE` so this will be ignored |
There was a problem hiding this comment.
Thinking about the future, if we are immediately sending changes to the backend using websockets, maybe we should be emitting 'reversions' as full changes that also get propagated to the backend, rather than trying to catch them before they're applied?
More like git reverting a commit, rather than dropping a commit. The main challenge comes for things like publishing and syncing where there is not a clear path for what a 'restore' looks like.
There was a problem hiding this comment.
This is a great catch because by removing the change locks, I did change the behavior to be more like a git revert, but I did not update this logic accordingly! This logic should no longer use an ignored source for the updates. At the moment, we're only wrapping copy and move operations, so events like publishing and syncing shouldn't even occur here, but it would be good to be defensive against those things.
| .toArray(); | ||
| }); | ||
|
|
||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
I think your comment got lost in a rebase, but this does look like a neat way to watch for changes.
facfacc to
a980a00
Compare
a980a00 to
90fb223
Compare
rtibbles
left a comment
There was a problem hiding this comment.
Code review looks good, manual testing shows no errors, and I am seeing snackbars!
| if (this._isBlocking) { | ||
| this._isBlocking = false; | ||
| this.emit('unblocked'); | ||
| doRevert() { |
There was a problem hiding this comment.
I think this logic could also be useful for handling changes that error on the backend too (but in that case I suppose it would be an ignored change).
2022-09-27_16-12-39.mp4cc @radinamatic |
Summary
Description of the change(s) you made
Manual verification steps performed
References
Comments
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)