-
Notifications
You must be signed in to change notification settings - Fork 972
handle empty objects in sync DELETEs, refactor DELETE code #9349
Conversation
i'm having mild issues testing this right now because i can't delete bookmarks (right click menu doesn't work on toolbar or bookmarks manager) |
@ayumi same; i worked around it by going to the site and clicking on the bookmark star to edit it. doesn't work for deleting bookmark folders though. |
@ayumi fixed by rebase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an error when sync is enabled and i try to delete a bookmark toolbar bookmark or folder:
sync 1497298899503: sending record: {"action":0,"bookmark":{"hideInToolbar":false,"isFolder":false,"site":{"creationTime":0,"customTitle":"","favicon":"","lastAccessedTime":0,"location":"https://archive.org/","title":"Internet Archive: Digital Library of Free Books, Movies, Music & Wayback Machine"}},"deviceId":{"0":0},"objectId":{"0":88,"1":27,"2":9,"3":5,"4":193,"5":163,"6":63,"7":17,"8":32,"9":20,"10":19,"11":60,"12":194,"13":251,"14":248,"15":199}}
An uncaught exception occurred in the main process Uncaught Exception:
AssertionError: state must be an Immutable.Map
at validateState (/repos/github.com/brave/browser-laptop/app/common/state/windowState.js:18:10)
at Object.getActiveWindow (/repos/github.com/brave/browser-laptop/app/common/state/windowState.js:75:13)
at Object.getActiveWindowId (/repos/github.com/brave/browser-laptop/app/common/state/windowState.js:80:21)
at Object.getActiveTab (/repos/github.com/brave/browser-laptop/app/common/state/tabState.js:543:48)
at updateActiveTabBookmarked (/repos/github.com/brave/browser-laptop/app/browser/reducers/sitesReducer.js:32:24)
at sitesReducer (/repos/github.com/brave/browser-laptop/app/browser/reducers/sitesReducer.js:85:15)
STR:
- Fresh profile; enable sync.
- Add folder to bookmark toolbar
- Try to delete the folder from the toolbar
* @param {Object} action | ||
* @returns {boolean} | ||
*/ | ||
const validateAction = (action) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this no longer needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
@ayumi that error should be fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚗
https://travis-ci.org/brave/browser-laptop/jobs/242177590 shows 1 test failure but it passes locally so seems unrelated |
@diracdeltas don't forget to cherry-pick into 0.18.x now that master is pointing at 0.19.x 😄 |
@bsclifton cool, i will do that once the fixes for this PR are approved (#9407) |
handle empty objects in sync DELETEs, refactor DELETE code
this is causing weird behavior with moving sites. may have to revert. |
fix #9308
gets rid of syncActions.removeSite by DELETEing sites in the appStoreChange listener instead
test plan:
Submitter Checklist:
git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests