Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfix syncing error when lastAccessedTime is empty #8556
Conversation
| @@ -607,7 +607,7 @@ module.exports.isHistoryEntry = function (siteDetail) { | |||
| // This is a Brave default newtab site | |||
| return false | |||
| } | |||
| return !!siteDetail.get('lastAccessedTime') && !isBookmarkFolder(siteDetail.get('tags')) | |||
| return !isBookmarkFolder(siteDetail.get('tags')) | |||
This comment has been minimized.
This comment has been minimized.
ayumi
Apr 28, 2017
Contributor
I think this change causes all bookmarks to show up as History items.
It might be that https://github.com/brave/browser-laptop/blob/master/app/common/lib/historyUtil.js#L19 is now returning every site not a bookmark folder.
Repro:
- Start pyramid 0, enable Sync and visit archive.org and wikipedia.org
- Start pyramid 1, join pyramid 0's sync group
- Examine pyramid 1's history – it has the 2 sites.
Expected: Pyramid 1's history should be empty because history doesn't sync by default.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
diracdeltas
Apr 28, 2017
Author
Member
Hm, actually I can't repro this. Did you start from fresh pyramids?
This comment has been minimized.
This comment has been minimized.
Test Plan: unittests should pass
|
@ayumi i may have fixed it |
| return false | ||
| } | ||
| return !!siteDetail.get('lastAccessedTime') && !isBookmarkFolder(siteDetail.get('tags')) | ||
| return !!siteDetail.get('lastAccessedTime') || !tags || tags.size === 0 |
This comment has been minimized.
This comment has been minimized.
ayumi
Apr 29, 2017
Contributor
@diracdeltas hmm that fixed half of it
try syncing to the forsaken lindy ... group. i see a lot of history items.
should this line be
return !!siteDetail.get('lastAccessedTime') && (!tags || tags.size === 0)
This comment has been minimized.
This comment has been minimized.
diracdeltas
Apr 29, 2017
•
Author
Member
@ayumi actually the crash was caused by a history entry which did not have lastAccessedTime, so changing it to !!siteDetail.get('lastAccessedTime') && (!tags || tags.size === 0) would cause it to crash again.
when i sync to forsaken..., the synced bookmarks appear in about:history. this seems like the correct behavior to me because every bookmark is by definition also a history entry.
are you seeing history entries in about:history that are not bookmarks?
This comment has been minimized.
This comment has been minimized.
ayumi
Apr 29, 2017
Contributor
i don't think there'd be a crash any more due to the change to downgrade the throw into console.log.
On bookmarks == history, I think that's not obvious to a user; so if I synced my bookmarks I'd expect history to be empty. It would only make sense if I understood the sites data model. However, this is how it currently happens in master so this is probably ok.
This comment has been minimized.
This comment has been minimized.
diracdeltas
Apr 29, 2017
Author
Member
@ayumi right, it would fix the crash, but I think the user's data was a valid history entry and should not be ignored by sync.
i think "make synced bookmarks not part of history" is a separate issue
diracdeltas commentedApr 28, 2017
•
edited
fix #8552
Test Plan: unittests and syncing tests should pass
git rebase -ito squash commits (if needed).Test Plan: