-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add SYNC-3478 [v109] Poll for missing send tabs on interval #12377
Add SYNC-3478 [v109] Poll for missing send tabs on interval #12377
Conversation
d24eec2
to
4a4246b
Compare
4a4246b
to
e7755ba
Compare
Going to add a test for |
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.
A small nit if you feel like changing it, otherwise LGTM.
83478a6
to
764b520
Compare
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.
Thanks a ton for the review @OrlaM 🙏 I added one more commit after doing testing on my physical device
Unfortunately couldn't find any testing infra for BrowserProfile
so testing pollCommands
properly seemed more trouble than its worth
If it looks good, I would love to see this land (I don't have permission to land PRs 😞)
// Reconfig has to happen on the main thread, since it calls `startup` | ||
// and `startup` asserts that we are on the main thread. Otherwise the notification | ||
// service will crash. | ||
DispatchQueue.main.async { |
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'm mildly surprised send tab even works in prod correctly. I had to add this because my device couldn't receive send tabs properly, and after debugging I found that the culprit is that the NotificationService on my device is running a background thread when receiving messages
This causes it to crash and deliver a default notification instead of the actual tab
I'm assuming in production builds (and in beta I tested to make sure) the notification service runs on the main thread somehow? regardless dispatching this onto the main thread fixed local push notification for me on my iPhone
Let’s hold off on merging this while I confirm one more thing! Tapping on a push notification while the app is backgrounded is not launching the tab - I’ll flag once I figure out what’s going there |
Might be worth checking if that actually works on main. We made the switch to use SceneDelegates recently which has caused a few deeplinking bugs to pop up, so might not be caused by your PR. |
Rebased on main the problem fixed itself! This should be good to land, with an enhancement coming soon in the next application-services release. Currently, if we get a push notification and we poll soon after, it's possible that the poll would reopen the same tab again This is possible if the account manager didn't refresh its state before the next poll, since the notification service has its own instance of the account manager. I'll have a PR next week with a new AS release to make it impossible |
On desktop and android, we poll for missed send tabs occasionally. For iOS, we only poll for send tabs when we receive a notification (and even then, we only poll for exactly one tab). This PR changes it so we poll for missing commands whenever we sync the clients collection.
Happy to take any feedback here. Creating the PR as a starting point. Keeping as a draft as I do more testing on a physical device
Questions:
What's the user experience:
Would this increase load to the FxA servers
Now that we poll, what happens to users who didn't get their tabs in the past?