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

Refactor automatic update flow to use custom Sparkle user driver #3274

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

quanganhdo
Copy link
Member

@quanganhdo quanganhdo commented Sep 13, 2024

Task/Issue URL: https://app.asana.com/0/1201037661562251/1208183525704010/f
Tech Design URL:
CC:

Description:

Refactors automatic update flow to use a custom Sparkle user driver.

Steps to test this PR:

  1. Refer to https://app.asana.com/0/1201037661562251/1208230039908704/f

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@quanganhdo
Copy link
Member Author

quanganhdo commented Sep 13, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @quanganhdo and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Sep 13, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against bdc7a75

@quanganhdo quanganhdo changed the title Revert "Resolving automatic update edge cases (#3142)" Refactor automatic update flow to use custom Sparkle user driver Sep 13, 2024
@quanganhdo quanganhdo marked this pull request as ready for review September 13, 2024 19:10
This was referenced Sep 13, 2024
@quanganhdo quanganhdo force-pushed the anh/refactor/sparkle-user-driver branch 2 times, most recently from 1a1d4e6 to ccfe27b Compare September 16, 2024 15:34
Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

The code looks really good. 💯 I'm going to test the solution now

func checkForUpdate() {
func checkForUpdateIfNeeded() {
guard let updater, !updater.sessionInProgress else { return }

Logger.updates.debug("Checking for updates")
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a mistake when refactoring pixels and all update pixels are just debug pixels. Do you think it would be beneficial to make them available in release builds for potential debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be beneficial to have pixels at different states to see if our users make it to the end of the update cycle. Have we defined those, or do they have to be created?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are couple of pixels and the error pixel in case something goes wrong. Monitoring various states as you suggested may be a good idea too 👍

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

@quanganhdo, after installation of the build number 261, it says the app is up to date which is not expected. Am I missing something? I'm currently at the coffee shop with a low connection or maybe the phased rollout is the reason?
Screenshot 2024-09-18 at 09 53 10

Another minor thing is the empty box in the release notes which I suppose is caused by empty release notes in the appcast. Just flagging it in case not.

@quanganhdo
Copy link
Member Author

quanganhdo commented Sep 18, 2024

The test appcast pointed to internal-channel so internal user state was needed https://github.com/quanganhdo/playground/blob/main/appcast2.xml

I've removed that channel from the file. Update should show up now (along with the release note).

Copy link
Contributor

@tomasstrba tomasstrba left a comment

Choose a reason for hiding this comment

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

LGMT! 👍 Works as expected including edge cases

Copy link

github-actions bot commented Oct 3, 2024

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

quanganhdo and others added 29 commits October 18, 2024 10:55
…ere UI stuck loading when switching between browser update modes
Task/Issue URL:
https://app.asana.com/0/1201037661562251/1208183525704010/f
Tech Design URL:
CC:
Figma:
https://www.figma.com/design/vDBpSFDzW0eN7nwpS95gju/macOS-update-process?node-id=2359-52971&t=KnvIdm2f7PRjNwfK-4

**Description**:

**Steps to test this PR**:
1. Change the appcast URL to an invalid URL to test the error state
2. Play with different update options to see if update works

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

**Definition of Done**:

* [ ] Does this PR satisfy our [Definition of
Done](https://app.asana.com/0/1202500774821704/1207634633537039/f)?

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
[Pull Request
Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f)
@quanganhdo quanganhdo force-pushed the anh/refactor/sparkle-user-driver branch from 4130eff to bdc7a75 Compare October 18, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants