Repository navigation
Fix Sparkle update dialog requiring two presses - #1908
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRoutes user-initiated update checks through a new request entry with a presentation mode, probes the latest appcast item before starting updates (with coalescing and cancellation), adds debug injection for UI tests, and introduces reflection helpers, a network appcast probe, and a parser. Also adds presentation-tracking and delegation to Sparkle's standard driver. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UpdateController
participant UpdateLatestItemProbe
participant AppcastServer
participant UpdateDriver
participant SPUStandardUserDriver
User->>UpdateController: requestCheckForUpdates(presentation)
UpdateController->>UpdateController: coalesce requests / set checking state
UpdateController->>UpdateLatestItemProbe: probe latest appcast
UpdateLatestItemProbe->>AppcastServer: GET appcast feed
AppcastServer-->>UpdateLatestItemProbe: appcast XML
UpdateLatestItemProbe-->>UpdateController: latest item (version)
UpdateController->>UpdateController: clear deferred candidate if needed
UpdateController->>UpdateDriver: prepareForUserInitiatedCheck(presentation)
UpdateDriver->>UpdateDriver: choose presentation (dialog/custom)
alt dialog
UpdateDriver->>SPUStandardUserDriver: showUserInitiatedUpdateCheck()
else custom
UpdateDriver->>UpdateDriver: begin custom checking UI flow
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes issue #1906, where the standard Sparkle update dialog required two "Check for Updates" presses to appear. The root cause was that the first press fell through to the custom/unobtrusive update UI flow; this PR introduces a Key changes:
One notable gap: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant Menu as Help Menu
participant Controller as UpdateController
participant Driver as UpdateDriver
participant Standard as SPUStandardUserDriver
participant Sparkle as SPUUpdater
Note over Controller,Driver: User-initiated check (.dialog path)
User->>Menu: Click "Check for Updates"
Menu->>Controller: checkForUpdates()
Controller->>Controller: requestCheckForUpdates(.dialog)
Controller->>Controller: checkForUpdatesWhenReady(presentation: .dialog)
Controller->>Controller: performCheckForUpdates(.dialog)
Controller->>Driver: prepareForUserInitiatedCheck(.dialog)
Note right of Driver: pendingUserInitiatedCheckPresentation = .dialog
Controller->>Sparkle: updater.checkForUpdates()
Sparkle->>Driver: showUserInitiatedUpdateCheck(cancellation:)
Driver->>Driver: activateUserInitiatedCheckPresentation() → .dialog
Driver->>Standard: showUserInitiatedUpdateCheck(cancellation:)
Sparkle->>Driver: showUpdateFound(appcastItem, state, reply)
Driver->>Standard: showUpdateFound(appcastItem, state, reply)
Standard-->>User: Show standard Sparkle dialog
User->>Standard: Choose "Remind Me Later" / "Skip"
Standard->>Driver: reply(.dismiss/.skip)
Driver->>Driver: finishUserInitiatedCheckPresentation()
Note right of Driver: active/pendingPresentation = nil
Note over Controller,Driver: Background/attemptUpdate path (.custom)
Controller->>Controller: attemptUpdate()
Controller->>Controller: requestCheckForUpdates(.custom)
Controller->>Driver: prepareForUserInitiatedCheck(.custom)
Controller->>Sparkle: updater.checkForUpdates()
Sparkle->>Driver: showUserInitiatedUpdateCheck(cancellation:)
Driver->>Driver: activateUserInitiatedCheckPresentation() → .custom
Driver->>Driver: beginChecking(cancel:) [custom UI]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if state.userInitiated, choice != .install { | ||
| finishUserInitiatedCheckPresentation() | ||
| } |
There was a problem hiding this comment.
finishUserInitiatedCheckPresentation call for standard presentation
For the .dialog (standard) presentation path, finishUserInitiatedCheckPresentation() is already called inside the showUpdateFound reply closure (UpdateDriver.swift, line 78) whenever choice != .install. The delegate method userDidMake:forUpdate:state: fires immediately afterward and calls it a second time. The double-call is idempotent (both nils are already cleared), but it can be confusing. Consider guarding this call to only apply to the .custom path where showUpdateFound does not handle cleanup directly:
| if state.userInitiated, choice != .install { | |
| finishUserInitiatedCheckPresentation() | |
| } | |
| if state.userInitiated, choice != .install, activeUserInitiatedCheckPresentation == .custom { | |
| finishUserInitiatedCheckPresentation() | |
| } |
Alternatively, since finishUserInitiatedCheckPresentation is already idempotent, this can be left as-is — but worth documenting the intentional double-call to avoid future confusion.
| @@ -220,20 +225,21 @@ class UpdateController { | |||
| viewModel.state.cancel() | |||
|
|
|||
| DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(100)) { [weak self] in | |||
| self?.userDriver.prepareForUserInitiatedCheck(presentation: presentation) | |||
| self?.updater.checkForUpdates() | |||
| } | |||
There was a problem hiding this comment.
prepareForUserInitiatedCheck sets pending presentation before the previous check is cancelled
When viewModel.state != .idle, prepareForUserInitiatedCheck(presentation:) is called at line 218 before viewModel.state.cancel() at line 225. If cancelling the in-flight check causes Sparkle to call dismissUpdateInstallation synchronously (or before the 100 ms async block runs), usesStandardPresentation evaluates to true because pendingUserInitiatedCheckPresentation is already set. This causes standard.dismissUpdateInstallation() to be called on a SPUStandardUserDriver that was never activated (no showUserInitiatedUpdateCheck was forwarded to it), which could leave the standard driver in an unexpected internal state.
Moving the first prepareForUserInitiatedCheck call inside the async block alongside the second one would ensure the pending presentation is only set immediately before updater.checkForUpdates() is actually invoked:
private func performCheckForUpdates(presentation: UpdateUserInitiatedCheckPresentation) {
startUpdaterIfNeeded()
ensureSparkleInstallationCache()
if viewModel.state == .idle {
userDriver.prepareForUserInitiatedCheck(presentation: presentation)
updater.checkForUpdates()
return
}
installCancellable?.cancel()
viewModel.state.cancel()
DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(100)) { [weak self] in
self?.userDriver.prepareForUserInitiatedCheck(presentation: presentation)
self?.updater.checkForUpdates()
}
}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/SidebarHelpMenuUITests.swift`:
- Around line 51-57: The Sparkle UI tests hard-code English button labels;
update the app setup where XCUIApplication() is configured (the app variable and
its launchEnvironment before launchAndActivate(app)) to pin the test locale to
English by injecting AppleLanguages/AppleLocale (or passing -AppleLanguages and
-AppleLocale launch arguments) so the dialog labels are deterministic across
runners; ensure the same change is applied where the other block at lines 75-79
configures the app.
In `@Sources/Update/UpdateDriver.swift`:
- Around line 95-107: The custom presentation path never clears
activeUserInitiatedCheckPresentation when it reaches terminal states, so update
the custom terminal exits (e.g., in
showUpdateNotFoundWithError(_:acknowledgement:), and the other custom
.notFound/.error/.cancel/.dismiss handlers referenced near the other ranges) to
clear/reset the active presentation—call finishUserInitiatedCheckPresentation()
or otherwise clear activeUserInitiatedCheckPresentation before invoking
acknowledgement callbacks or before calling
setStateAfterMinimumCheckDelay(.notFound(...)) so that
currentUserInitiatedCheckPresentation() will no longer prefer the stale custom
presentation and future prepareForUserInitiatedCheck(.dialog) can open the
standard dialog again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98f74e3b-f48e-4acd-9f49-66c292ed1c4e
📒 Files selected for processing (4)
Sources/Update/UpdateController.swiftSources/Update/UpdateDelegate.swiftSources/Update/UpdateDriver.swiftcmuxUITests/SidebarHelpMenuUITests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Update/UpdateController.swift (1)
274-278:⚠️ Potential issue | 🟡 MinorLocalize the startup-timeout error.
This message is surfaced via the update error UI, so it should not be a bare string literal.
🌐 One possible fix
error: NSError( domain: "cmux.update", code: 1, - userInfo: [NSLocalizedDescriptionKey: "Updater is still starting. Try again in a moment."] + userInfo: [ + NSLocalizedDescriptionKey: String( + localized: "update.error.starting", + defaultValue: "Updater is still starting. Try again in a moment." + ) + ] ),As per coding guidelines: "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Update/UpdateController.swift` around lines 274 - 278, The NSError created in UpdateController.swift for the “Updater is still starting. Try again in a moment.” message must be localized: replace the hard-coded NSLocalizedDescriptionKey value with a localized string using String(localized: "update.startupTimeout", defaultValue: "Updater is still starting. Try again in a moment.") (or an appropriate key) so the NSError userInfo uses the localized text; update the creation site (the NSError(...) call in UpdateController) and add the new localization key to your .strings/stringsdict resources.
♻️ Duplicate comments (1)
Sources/Update/UpdateDriver.swift (1)
218-243:⚠️ Potential issue | 🟠 MajorFinish
.customon the remaining install/dismiss exits.The new cleanup on the not-found/error branches is good, but these custom install paths still only drive
viewModel.stateback to.idle. After a dismissed or completedattemptUpdate(),activeUserInitiatedCheckPresentationcan remain.custom, so the next Help → Check for Updates can fall back to the silent path again.🔁 One possible fix
setState(.installing(.init( retryTerminatingApplication: retryTerminatingApplication, - dismiss: { [weak viewModel] in + dismiss: { [weak self, weak viewModel] in + self?.finishUserInitiatedCheckPresentation() viewModel?.state = .idle } ))) … - setState(.idle) + finishUserInitiatedCheckPresentation() + setState(.idle) acknowledgement() … - setState(.idle) + finishUserInitiatedCheckPresentation() + setState(.idle)Also applies to: 251-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Update/UpdateDriver.swift` around lines 218 - 243, The custom presentation exits in UpdateDriver are not finishing the user-initiated presentation, leaving activeUserInitiatedCheckPresentation as .custom; in showInstallingUpdate(withApplicationTerminated:retryTerminatingApplication:) ensure the dismiss closure calls finishUserInitiatedCheckPresentation() (in addition to setting viewModel?.state = .idle), and in showUpdateInstalledAndRelaunched(_:, acknowledgement:) call finishUserInitiatedCheckPresentation() before/after setting state/acknowledgement when usesStandardPresentation is false; apply the same fix to the other custom install/dismiss paths in this file (the other methods handling install completion/failure/dismissal) so every custom-path exit invokes finishUserInitiatedCheckPresentation().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Update/UpdateController.swift`:
- Around line 433-441: The log currently assumes clearing the deferred update
always succeeds; instead check the boolean returned by
clearDeferredUpdateCandidate() (which forwards the success from
SparkleResumableUpdateReflection.clearDeferredUpdate(from:)) and only call
UpdateLogStore.shared.append(...) when that returned flag is true; apply the
same change for the other occurrences around the blocks using
shouldReplaceDeferredUpdate(...) (the similar sites at the other noted
locations) so the "cleared stale deferred update ..." message is only logged on
actual success.
- Around line 424-447: The probe callback path that waits for
UpdateLatestItemProbe (probe.start) needs a bounded fallback: when starting
UpdateLatestItemProbe in the block that sets latestItemProbe and
latestItemProbeQueuedFollowUp, schedule a short timeout (e.g., via
DispatchQueue.asyncAfter or a DispatchWorkItem) that, if fired, clears
latestItemProbe, grabs the followUp (same logic used in the success path: use
latestItemProbeQueuedFollowUp ?? completion then nil it), logs or note a
timeout, and calls followUp(); when the real probe completion occurs cancel the
timeout before performing the existing cleanup (clearing latestItemProbe,
merging follow-ups, deciding deferred update replacement, and calling
followUp()). Ensure this also mirrors the same timeout addition for the other
probe site referenced (the similar block around lines 557-600).
---
Outside diff comments:
In `@Sources/Update/UpdateController.swift`:
- Around line 274-278: The NSError created in UpdateController.swift for the
“Updater is still starting. Try again in a moment.” message must be localized:
replace the hard-coded NSLocalizedDescriptionKey value with a localized string
using String(localized: "update.startupTimeout", defaultValue: "Updater is still
starting. Try again in a moment.") (or an appropriate key) so the NSError
userInfo uses the localized text; update the creation site (the NSError(...)
call in UpdateController) and add the new localization key to your
.strings/stringsdict resources.
---
Duplicate comments:
In `@Sources/Update/UpdateDriver.swift`:
- Around line 218-243: The custom presentation exits in UpdateDriver are not
finishing the user-initiated presentation, leaving
activeUserInitiatedCheckPresentation as .custom; in
showInstallingUpdate(withApplicationTerminated:retryTerminatingApplication:)
ensure the dismiss closure calls finishUserInitiatedCheckPresentation() (in
addition to setting viewModel?.state = .idle), and in
showUpdateInstalledAndRelaunched(_:, acknowledgement:) call
finishUserInitiatedCheckPresentation() before/after setting
state/acknowledgement when usesStandardPresentation is false; apply the same fix
to the other custom install/dismiss paths in this file (the other methods
handling install completion/failure/dismissal) so every custom-path exit invokes
finishUserInitiatedCheckPresentation().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d984e9cc-6380-4f38-b191-7ed1f010a8fa
📒 Files selected for processing (3)
Sources/Update/UpdateController.swiftSources/Update/UpdateDriver.swiftcmuxUITests/SidebarHelpMenuUITests.swift
| let probe = UpdateLatestItemProbe() | ||
| latestItemProbe = probe | ||
| latestItemProbeQueuedFollowUp = nil | ||
| probe.start { [weak self] result in | ||
| guard let self else { return } | ||
| self.latestItemProbe = nil | ||
| let followUp = self.latestItemProbeQueuedFollowUp ?? completion | ||
| self.latestItemProbeQueuedFollowUp = nil | ||
|
|
||
| if let latestValidUpdate = result.latestValidUpdate, | ||
| self.shouldReplaceDeferredUpdate( | ||
| deferredUpdateCandidate, | ||
| with: latestValidUpdate | ||
| ) { | ||
| self.clearDeferredUpdateCandidate() | ||
| UpdateLogStore.shared.append( | ||
| "cleared stale deferred update \(deferredUpdateCandidate.displayVersionString) in favor of \(latestValidUpdate.displayVersionString)" | ||
| ) | ||
| } else if let error = result.error { | ||
| UpdateLogStore.shared.append("latest update probe failed: \(self.userDriver.formatErrorForLog(error))") | ||
| } | ||
|
|
||
| followUp() | ||
| } |
There was a problem hiding this comment.
Give the preflight probe a bounded fallback.
This path waits for UpdateLatestItemProbe to finish before calling performUserInitiatedCheck, and that probe has no local deadline. In the exact deferred-update case this PR targets, a slow or hung feed request can leave Help → Check for Updates stuck in .checking instead of ever opening Sparkle’s dialog. Add a short timeout and continue with followUp() when it expires.
Also applies to: 557-600
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Update/UpdateController.swift` around lines 424 - 447, The probe
callback path that waits for UpdateLatestItemProbe (probe.start) needs a bounded
fallback: when starting UpdateLatestItemProbe in the block that sets
latestItemProbe and latestItemProbeQueuedFollowUp, schedule a short timeout
(e.g., via DispatchQueue.asyncAfter or a DispatchWorkItem) that, if fired,
clears latestItemProbe, grabs the followUp (same logic used in the success path:
use latestItemProbeQueuedFollowUp ?? completion then nil it), logs or note a
timeout, and calls followUp(); when the real probe completion occurs cancel the
timeout before performing the existing cleanup (clearing latestItemProbe,
merging follow-ups, deciding deferred update replacement, and calling
followUp()). Ensure this also mirrors the same timeout addition for the other
probe site referenced (the similar block around lines 557-600).
| if let latestValidUpdate = result.latestValidUpdate, | ||
| self.shouldReplaceDeferredUpdate( | ||
| deferredUpdateCandidate, | ||
| with: latestValidUpdate | ||
| ) { | ||
| self.clearDeferredUpdateCandidate() | ||
| UpdateLogStore.shared.append( | ||
| "cleared stale deferred update \(deferredUpdateCandidate.displayVersionString) in favor of \(latestValidUpdate.displayVersionString)" | ||
| ) |
There was a problem hiding this comment.
Only log a stale-candidate clear when it actually succeeds.
SparkleResumableUpdateReflection.clearDeferredUpdate(from:) already returns a success flag, but clearDeferredUpdateCandidate() discards it and the caller logs success unconditionally. If that clear no-ops, the stale deferred update remains in place and this fix silently regresses.
🛠️ One possible fix
- private func clearDeferredUpdateCandidate() {
+ private func clearDeferredUpdateCandidate() -> Bool {
`#if` DEBUG
debugDeferredUpdateCandidate = nil
`#endif`
- _ = SparkleResumableUpdateReflection.clearDeferredUpdate(from: updater)
+ return SparkleResumableUpdateReflection.clearDeferredUpdate(from: updater)
}
…
- self.clearDeferredUpdateCandidate()
- UpdateLogStore.shared.append(
- "cleared stale deferred update \(deferredUpdateCandidate.displayVersionString) in favor of \(latestValidUpdate.displayVersionString)"
- )
+ if self.clearDeferredUpdateCandidate() {
+ UpdateLogStore.shared.append(
+ "cleared stale deferred update \(deferredUpdateCandidate.displayVersionString) in favor of \(latestValidUpdate.displayVersionString)"
+ )
+ } else {
+ UpdateLogStore.shared.append("failed to clear stale deferred update candidate")
+ }Also applies to: 476-480, 529-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Update/UpdateController.swift` around lines 433 - 441, The log
currently assumes clearing the deferred update always succeeds; instead check
the boolean returned by clearDeferredUpdateCandidate() (which forwards the
success from SparkleResumableUpdateReflection.clearDeferredUpdate(from:)) and
only call UpdateLogStore.shared.append(...) when that returned flag is true;
apply the same change for the other occurrences around the blocks using
shouldReplaceDeferredUpdate(...) (the similar sites at the other noted
locations) so the "cleared stale deferred update ..." message is only logged on
actual success.
…e-dialog-double-press Fix Sparkle update dialog requiring two presses
…low-ai#2090) * Restore inline sidebar update checks and embed appcast changelog * Revert Sparkle manual update dialog flow
This branch was successfully deployed
1 participant
Summary
Closes #1906
Testing
Summary by cubic
Fixes the double‑press bug (#1906) by showing
Sparkle’s standard update dialog on the first manual Check for Updates, while keeping our custom, unobtrusive flow for background and auto‑install checks. Also ensures the dialog always reflects the latest available update..dialogusingSPUStandardUserDriver; background stays.custom.SPUUpdaterresumable update reset).98.0.0with the latest99.0.0.Written for commit 44b4374. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests