-
Notifications
You must be signed in to change notification settings - Fork 0
Implement LaunchAgent scheduler for reliable background updates #4
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
base: main
Are you sure you want to change the base?
Conversation
- Add Anchoring + Decoy pricing in Pro tab ($3.99 → $2.99, decoy option) - Implement Loss Aversion for security warnings ("Don't risk unpatched apps") - Add Social Proof elements (user count, testimonials) - Create Zeigarnik Effect progress indicators with completion tension - Set smart defaults (auto-update ON) using Default Effect - Enhance error states with Actor-Observer bias (situational framing) - Add Authority signals (industry-standard verification) - Improve Privacy tab with Reciprocity bias ("helps improve reliability") 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add SecurityChecks for codesign verification and quarantine handling - Create Installer for ZIP/DMG/PKG with automatic backup before install - Implement Rollback system for one-click version restoration - Include cleanup utilities for managing backup storage - All installs verify signatures before replacing apps 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Implement SparkleHints for SUFeedURL discovery from app bundles - Add GitHub API client for releases with rate limiting and auth - Create RepoHints mapping for 30+ popular apps to GitHub repos - Add Brew cask integration for Homebrew-managed apps - Include repository validation and fallback discovery patterns 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace BGTaskScheduler with macOS LaunchAgent for better reliability - Add install/uninstall functionality with proper plist generation - Include status checking and schedule updating capabilities - Configure for low-priority background execution - Handle loading/unloading with proper error handling 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @adityash8, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a fundamental shift in how background updates are managed within the AutoUp application, transitioning to a more robust macOS LaunchAgent system. This change not only enhances the reliability of scheduled updates but also lays the groundwork for a more comprehensive update management platform. New core functionalities have been added to interact with various update sources and installation methods, alongside improved security checks and a user-friendly rollback feature. The user interface has also received significant attention, incorporating psychological design principles to provide a more engaging and informative experience, complemented by anonymous telemetry for continuous improvement. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a significant amount of new functionality by replacing BGTaskScheduler
with a more reliable LaunchAgent
for background updates. The new core logic for handling Homebrew, GitHub releases, installations, and rollbacks is well-structured. I'm particularly impressed with the thoughtful UX improvements in the UI, which leverage psychological principles to enhance user engagement. My review focuses on increasing the robustness and maintainability of the new backend code, with suggestions to improve error handling, reduce code duplication, and adhere to Swift best practices.
|
||
if let appPath = currentAppPath { | ||
let fullPath = "/Applications/\(appPath)" | ||
_ = try? SecurityChecks.backup(appPath: fullPath, bundleID: bundleID, version: currentVersion) |
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.
Similar to the ZIP installation, backup failures here are silently ignored using try?
. This is risky as a failed update could leave the app in a broken state with no backup. Consider handling the error from SecurityChecks.backup
to ensure a rollback is possible if the update fails.
_ = try? SecurityChecks.backup(appPath: fullPath, bundleID: bundleID, version: currentVersion) | |
_ = try SecurityChecks.backup(appPath: fullPath, bundleID: bundleID, version: currentVersion) |
let dateA = (try? a.resourceValues(forKeys: [.creationDateKey]).creationDate) ?? .distantPast | ||
let dateB = (try? b.resourceValues(forKeys: [.creationDateKey]).creationDate) ?? .distantPast | ||
return dateA > dateB | ||
}.first?.appendingPathComponent("\(bundleID).app") |
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.
The logic here assumes the backed-up application's filename is \(bundleID).app
. However, the SecurityChecks.backup
function preserves the original application name (e.g., AppName.app
). This will cause latestBackup
to return an incorrect URL, and rollbacks will fail. Instead of assuming the name, you should list the contents of the latest backup directory to find the .app
bundle within it.
}.first?.appendingPathComponent("\(bundleID).app") | |
}.first.flatMap { backupDirURL -> URL? in | |
(try? FileManager.default.contentsOfDirectory(at: backupDirURL, includingPropertiesForKeys: nil, options: []))? | |
.first(where: { $0.pathExtension == "app" }) | |
} |
} | ||
|
||
static func latest(owner: String, repo: String, token: String? = nil) async throws -> Release { | ||
var request = URLRequest(url: URL(string: "https://api.github.com/repos/\(owner)/\(repo)/releases/latest")!) |
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.
Force-unwrapping the URL with !
can lead to a runtime crash if the owner
or repo
strings contain characters that result in an invalid URL. It's safer to use guard let
to validate the URL and throw a specific error if it's invalid.
guard let url = URL(string: "https://api.github.com/repos/\(owner)/\(repo)/releases/latest") else {
throw GitHubError.invalidRepo
}
var request = URLRequest(url: url)
} | ||
|
||
static func releases(owner: String, repo: String, count: Int = 10, token: String? = nil) async throws -> [Release] { | ||
var request = URLRequest(url: URL(string: "https://api.github.com/repos/\(owner)/\(repo)/releases?per_page=\(count)")!) |
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.
Force-unwrapping the URL with !
can lead to a runtime crash. As with the latest
function, it's safer to use guard let
to validate the URL and handle the potential for an invalid URL gracefully.
guard let url = URL(string: "https://api.github.com/repos/\(owner)/\(repo)/releases?per_page=\(count)") else {
throw GitHubError.invalidRepo
}
var request = URLRequest(url: url)
private static func moveToApplications(_ src: URL) throws { | ||
let dst = URL(fileURLWithPath: "/Applications").appendingPathComponent(src.lastPathComponent) |
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.
The moveToApplications
function hardcodes the destination to the system's /Applications
directory. This doesn't account for apps installed in the user's ~/Applications
directory. The installation process should respect the original location of the app. Consider passing the original application's path to this function to determine the correct destination directory.
throw InstallerError.dmgAttachFailed(code) | ||
} | ||
|
||
guard let mount = out.split(separator: "\t").last.map(String.init) else { |
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.
Parsing the plain text output of hdiutil
by splitting the string is brittle and might fail with different system languages or future hdiutil
versions. hdiutil
supports a -plist
argument which provides the output in a structured XML format. Parsing this plist would be a much more robust way to get the mount point.
try? task.run() | ||
task.waitUntilExit() | ||
return task.terminationStatus == 0 |
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.
Using try? task.run()
silently ignores errors that can occur when launching the process (e.g., if the codesign
executable doesn't exist). This can make debugging difficult. It would be more robust to use a do-catch
block to handle potential errors from task.run()
or let the function throw. This same issue applies to getQuarantineStatus
and removeQuarantine
.
try? task.run() | |
task.waitUntilExit() | |
return task.terminationStatus == 0 | |
do { | |
try task.run() | |
task.waitUntilExit() | |
return task.terminationStatus == 0 | |
} catch { | |
// It's good practice to log this error. | |
return false | |
} |
for key in sparkleKeys { | ||
if let value = bundle.object(forInfoDictionaryKey: key) { | ||
print("Found Sparkle key \(key) for \(bundle.bundleIdentifier ?? "unknown"): \(value)") | ||
} | ||
} |
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.
"StandardOutPath": NSHomeDirectory() + "/Library/Logs/AutoUp.log", | ||
"StandardErrorPath": NSHomeDirectory() + "/Library/Logs/AutoUp.err", |
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.
Constructing file paths by concatenating strings with +
is not safe and can lead to errors. It's better to use URL
's appendingPathComponent
method to build paths robustly, which correctly handles path separators and other file system conventions.
"StandardOutPath": NSHomeDirectory() + "/Library/Logs/AutoUp.log", | |
"StandardErrorPath": NSHomeDirectory() + "/Library/Logs/AutoUp.err", | |
"StandardOutPath": URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent("Library/Logs/AutoUp.log").path, | |
"StandardErrorPath": URL(fileURLWithPath: NSHomeDirectory()).appendingPathComponent("Library/Logs/AutoUp.err").path, |
enum Telemetry { | ||
static func configure(enabled: Bool, apiKey: String) { | ||
if enabled { | ||
PostHogSDK.shared.setup(apiKey: apiKey, host: URL(string:"https://app.posthog.com")!) |
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.
Force-unwrapping a URL with !
is generally discouraged, even for a hardcoded string. While it's unlikely to fail here, a safer pattern is to use guard let
and handle the nil case, for example by logging a fatal error. This makes the code more robust against unexpected issues.
guard let posthogHost = URL(string:"https://app.posthog.com") else {
// This should not happen with a hardcoded URL, but it's safer to handle.
fatalError("Invalid PostHog host URL")
}
PostHogSDK.shared.setup(apiKey: apiKey, host: posthogHost)
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.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on October 16
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
let dateA = (try? a.resourceValues(forKeys: [.creationDateKey]).creationDate) ?? .distantPast | ||
let dateB = (try? b.resourceValues(forKeys: [.creationDateKey]).creationDate) ?? .distantPast | ||
return dateA > dateB | ||
}.first?.appendingPathComponent("\(bundleID).app") |
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.
Bug: Backup Filename Mismatch Causes Rollback Failures
The latestBackup
function constructs the path to backed-up application bundles by appending bundleID.app
to the version folder path. However, the actual backup process stores the application bundle using its display name within that folder. This filename mismatch prevents latestBackup
from finding the correct backup, causing rollback operations to fail.
} | ||
|
||
static func track(_ name: String, props: [String: Any] = [:]) { | ||
guard UserDefaults.standard.bool(forKey: "telemetry_enabled") else { return } |
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.
Bug: Telemetry Toggle Key Mismatch
The Telemetry.track
function checks for the UserDefaults
key telemetry_enabled
, but the SettingsView
uses telemetryEnabled
for the telemetry toggle. This key mismatch prevents the UI setting from controlling telemetry tracking, making the privacy toggle non-functional.
Summary
Test plan
🤖 Generated with Claude Code