Show bookmarks toolbar when bookmarks are synced for the first time #7300
Conversation
app/sync.js
Outdated
| let showBookmarksToolbar = false | ||
| if (!initialState.seed) { | ||
| // syncing for the first time | ||
| const bookmarks = (AppStore.getState().get('sites') || new Immutable.List()).filter((site) => { |
There was a problem hiding this comment.
i think siteUtil.getBookmarks() does this
app/sync.js
Outdated
| ipcMain.on(messages.SYNC_READY, module.exports.onSyncReady.bind(null, | ||
| isFirstRun)) | ||
| initialized ? false : !initialState.seed && !initialState.deviceId)) |
There was a problem hiding this comment.
previously we extracted this out for clarity – is moving it into the function call needed?
There was a problem hiding this comment.
@ayumi yes, the previous code was incorrect because initialized gets changed later
There was a problem hiding this comment.
actually this still seems incorrect (assuming bind argument expressions are not re-evaluated)
app/sync.js
Outdated
| syncUtil.applySyncRecords(records) | ||
| let showBookmarksToolbar = false | ||
| if (!initialState.seed) { | ||
| // syncing for the first time |
There was a problem hiding this comment.
on the first sync, i feel like we should just check records for any bookmarks and enable the toolbar here
There was a problem hiding this comment.
i didn't do that because the bookmarks toolbar could show up noticeably earlier than when any bookmarks are actually created (for instance when syncing a large set of records). but it does make the code simpler.
app/sync.js
Outdated
| showBookmarksToolbar = true | ||
| } | ||
| } | ||
| syncUtil.applySyncRecords(records, showBookmarksToolbar) |
There was a problem hiding this comment.
it seems unnecessary to pass showBookmarksToolbar here and 8 more times in syncUtil.js
There was a problem hiding this comment.
do you mean unnecessary with #7300 (comment) or something else?
diracdeltas
force-pushed
the
fix/7282
branch
from
cce94a0 to
b5c7228
Compare
|
@ayumi addressed review comments |
diracdeltas
force-pushed
the
fix/7282
branch
from
b5c7228 to
bfe9fca
Compare
Fix #7282
Auditors: @ayumi
Test Plan:
git rebase -ito squash commits (if needed).