Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix FXIOS-11702 [v105] Fix syncClientsThenTabs function #11692

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

lougeniaC64
Copy link
Contributor

@lougeniaC64 lougeniaC64 commented Aug 23, 2022

This fixes #11702 which is a regression in the syncClientsThenTabs function which is triggered by clicking the "Sync Now" button on the Remote Tabs Panel.

@lougeniaC64 lougeniaC64 changed the title [WIP] Fix syncClientsThenTabs function Bugfix FXIOS-11702 [v105] Fix syncClientsThenTabs function Aug 24, 2022
@lougeniaC64
Copy link
Contributor Author

@nbhasin2 This is the tabs syncing bug we discussed. Just to reiterate for posterity, the bug that was introduced in v103 prevented tabs from being synced after the "Sync Now" button was clicked in the remote tabs panel. Because full syncs were unaffected (timed syncs, clicking "Sync Now" from the settings menu, etc.) this issue was masked and might seem like a delay in tabs syncing to the user.

@lougeniaC64 lougeniaC64 marked this pull request as ready for review August 24, 2022 14:53
Comment on lines 1433 to 1448
return self.syncSeveral(
why: .user,
synchronizers:
("clients", self.syncClientsWithDelegate),
("tabs", self.syncTabsWithDelegate)) >>== { statuses in
let status = statuses.find { "tabs" == $0.0 }
return deferMaybe(status!.1)
}
Copy link
Contributor

@nbhasin2 nbhasin2 Aug 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Indentation

It will look odd on github but its more readable on the XCode side

Suggested change
return self.syncSeveral(
why: .user,
synchronizers:
("clients", self.syncClientsWithDelegate),
("tabs", self.syncTabsWithDelegate)) >>== { statuses in
let status = statuses.find { "tabs" == $0.0 }
return deferMaybe(status!.1)
}
return self.syncSeveral(why: .user, synchronizers: ("clients", self.syncClientsWithDelegate),
("tabs", self.syncTabsWithDelegate)) >>== {
statuses in
let status = statuses.find { "tabs" == $0.0 }
return deferMaybe(status!.1)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be something like this because your first line will be too long.

            return self.syncSeveral(
                why: .user,
                synchronizers:
                ("clients", self.syncClientsWithDelegate),
                ("tabs", self.syncTabsWithDelegate)
            ) >>== { statuses in
                let status = statuses.find { "tabs" == $0.0 }
                return deferMaybe(status!.1)
            }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that makes sense I just quickly typed on github and likely made it worse ha

Comment on lines +1438 to +1447
let status = statuses.find { "tabs" == $0.0 }
return deferMaybe(status!.1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to write a comment to further explain what this does.

@lougeniaC64 lougeniaC64 force-pushed the fix-remote-panel-sync branch 2 times, most recently from 9bbbbf9 to 8290da7 Compare August 24, 2022 16:33
@lougeniaC64 lougeniaC64 force-pushed the fix-remote-panel-sync branch from 8290da7 to 7975984 Compare August 24, 2022 16:34
@lmarceau
Copy link
Contributor

lmarceau commented Aug 24, 2022

Bitrise build failed due to the following error

Screen Shot 2022-08-24 at 1 11 42 PM

@nbhasin2
Copy link
Contributor

Bitrise build failed due to the following error

Screen Shot 2022-08-24 at 1 11 42 PM

Seems like a BR issue, a quick re-run should fix this 🤞🏼

@lmarceau
Copy link
Contributor

All green now 🎉
Screen Shot 2022-08-24 at 1 50 57 PM

@nbhasin2 nbhasin2 merged commit 0daad91 into mozilla-mobile:main Aug 24, 2022
dnarcese pushed a commit to dnarcese/firefox-ios that referenced this pull request Sep 6, 2022
@lougeniaC64 lougeniaC64 deleted the fix-remote-panel-sync branch January 4, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix "Sync Now" tabs syncing in the Remote Tab Panel
3 participants