Potential improvements for macos update feature

1. TL;DR
   Tighten state updates and UI behavior, clamp progress values, fix a retain cycle in the error retry path, and make the “No Updates” auto-dismiss task cancellable. Add focused unit tests around ReleaseNotes parsing, UpdateState equality, progress math, and notFound timing. Consider marking UI-updating types @MainActor and handling updater start errors.

2. Recommended approach (simple path)

• Safety and correctness

  1. Mark UI-facing types/methods @MainActor to guarantee main-thread state mutations.
     • Add @MainActor to UpdateViewModel and to UpdateDriver (or at least to methods mutating viewModel.state).
     • Effort: S
  2. Clamp progress to [0, 1] in all UI render paths.
     • UpdateBadge.swift: when computing progress from bytes and in extracting.
       Example:
       let progress = min(1, max(0, Double(download.progress) / Double(expectedLength)))
     • UpdatePopoverView.DownloadingView and ExtractingView too.
     • Effort: S
  3. Fix retain cycle in UpdateDriver.showUpdaterError retry closure.
     • The retry closure captures self strongly via delegate.checkForUpdates(self).
       Change to capture self weakly or avoid passing self:
       retry: { [weak self, weak viewModel] in
       viewModel?.state = .idle
       DispatchQueue.main.async { [weak self] in
       guard let delegate = NSApp.delegate as? AppDelegate else { return }
       delegate.checkForUpdates(self) // or pass nil if supported
       }
       }
     • Effort: S
  4. Make the “No Updates Available” auto-reset task cancellable to avoid stale tasks firing.
     • UpdatePill.swift:
       @State private var resetTask: Task<Void, Never>?
       .onChange(of: model.state) { newState in
       resetTask?.cancel()
       if case .notFound = newState {
       resetTask = Task { [weak model] in
       try? await Task.sleep(for: .seconds(5))
       guard !Task.isCancelled, case .notFound? = model?.state else { return }
       model?.state = .idle
       }
       } else {
       resetTask = nil
       }
       }
     • Effort: S
  5. Handle startUpdater errors explicitly rather than swallowing with try?.
     • UpdateController.startUpdater():
       do { try updater.start() } catch {
       userDriver.viewModel.state = .error(.init(
       error: error,
       retry: { [weak self] in self?.startUpdater() },
       dismiss: { [weak self] in self?.viewModel.state = .idle }
       ))
       }
     • Effort: S

• UX polish and accessibility

  1. Add accessibility labels to badge/pill so assistive tech always announces state.
     • Example: in UpdatePill button content: .accessibilityLabel(model.text)
     • Effort: S
  2. Consider clamping/abbreviating error text in UI to avoid very long localizedDescription overflowing the pill. Optionally provide tooltip with full text (help(model.text) already present).
     • Effort: S

• Tests to add (high-leverage)

  1. ReleaseNotes parsing
     • Cases:
       • "1.2.3" -> .tagged with url https://ghostty.org/docs/install/release-notes/1-2-3
       • "v1.2.3" and "1.2.3-beta.1" (if you extend regex; see “Advanced path”)
       • Git hash present "abc1234" with and without current commit -> .compareTip vs .commit, correct URLs
       • No semver or hash -> nil
     • Effort: S
  2. UpdateState Equatable behavior
     • .downloading equality differs with progress or expectedLength; equal when both same
     • .error equality for same localizedDescription but different underlying error types (document current behavior)
     • .updateAvailable equality uses displayVersionString only
     • Effort: S
  3. ViewModel formatting
     • text for each state (including downloading with known and unknown length; extracting)
     • iconName/iconColor/backgroundColor/foregroundColor mappings per state
     • Effort: S
  4. Progress math
     • Division with expectedLength=0 or nil -> indeterminate path, no crash, clamped where used
     • Overshoot (progress > expectedLength) -> UI clamps at 100%
     • Effort: S
  5. “No Updates” auto-dismiss timing
     • Ensure state transitions to idle ~5s after .notFound if unchanged; ensure task cancels if state changes
     • Effort: M

1. Rationale and trade-offs

• Clamping progress prevents visual glitches if Sparkle reports unexpected values or overshoot.
• @MainActor avoids subtle threading issues with ObservableObject and SwiftUI.
• Cancelling the notFound timer prevents sporadic, out-of-context state flips to idle.
• Fixing the retry retain cycle prevents a potential long-lived reference cycle via viewModel.state.error.retry.
• Handling updater start errors surfaces failures early and reuses the existing error UI.
• Tests focus on non-UI logic (cheap to maintain, no new deps), covering the parts most likely to regress.

1. Risks and guardrails

• Changing Equatable semantics could affect SwiftUI’s change detection. Keep current semantics, just test and document them. If you change them, do so deliberately with tests.
• Clamping progress affects visuals; ensure you don’t hide legitimate >100% sequences you want shown (unlikely).
• Ensure passing nil (instead of self) to checkForUpdates is supported; otherwise keep [weak self] and pass self if non-nil.

1. When to consider the advanced path

• If you need broader version string support for ReleaseNotes (v-prefix, pre-release/build metadata).
• If you want deterministic tests for time-based or window-presence behavior (inject schedulers and window detectors).
• If UI behavior becomes more complex or multiple sources can mutate state concurrently (move to a small reducer/state machine with explicit transitions).

1. Optional advanced path (only if relevant)

• Extend ReleaseNotes semantic version detection to accept more real-world formats:
  • Regex: ^v?\d+.\d+.\d+(?:[-+][0-9A-Za-z.-]+)?$
  • Keep slugging to x-y-z from the numeric core.
• Inject dependencies for testability:
  • In UpdatePill, inject a scheduler for the 5s delay.
  • In UpdateDriver, inject a hasUnobtrusiveTarget closure to test fallback logic.
• Differentiate errors in Equatable with error type + description:
  • .error(let l, let r): String(describing: type(of: l.error))) + l.error.localizedDescription == same for r

Summary of specific changes (copy/paste)

• UpdateDriver.showUpdaterError retain fix:
  func showUpdaterError(_ error: any Error, acknowledgement: @escaping () -> Void) {
  viewModel.state = .error(.init(
  error: error,
  retry: { [weak self, weak viewModel] in
  viewModel?.state = .idle
  DispatchQueue.main.async { [weak self] in
  guard let delegate = NSApp.delegate as? AppDelegate else { return }
  delegate.checkForUpdates(self)
  }
  },
  dismiss: { [weak viewModel] in viewModel?.state = .idle }
  ))
  if !hasUnobtrusiveTarget { standard.showUpdaterError(error, acknowledgement: acknowledgement) }
  else { acknowledgement() }
  }

• Clamp progress (example in UpdateBadge):
  let progress = min(1, max(0, Double(download.progress) / Double(expectedLength)))

• Make NotFound auto-dismiss cancellable (UpdatePill):
  @State private var resetTask: Task<Void, Never>?
  .onChange(of: model.state) { newState in
  resetTask?.cancel()
  if case .notFound = newState {
  resetTask = Task { [weak model] in
  try? await Task.sleep(for: .seconds(5))
  guard !Task.isCancelled, case .notFound? = model?.state else { return }
  model?.state = .idle
  }
  } else {
  resetTask = nil
  }
  }

• Add @MainActor:
  @MainActor final class UpdateViewModel: ObservableObject { ... }
  @MainActor final class UpdateDriver: NSObject, SPUUserDriver { ... }

Key areas needing unit tests

• ReleaseNotes init logic (semver, git hash, URL formation).
• UpdateState Equatable for all cases (esp. .error and .downloading).
• ViewModel text/icon/background/foreground mappings.
• Progress clamping behavior (downloading/extracting).
• NotFound auto-dismiss timing and cancellation.

Effort estimate

• Safety fixes (@MainActor, clamping, retain fix, startUpdater error): S (<1h)
• NotFound cancellable task + accessibility labels: S (<1h)
• Unit tests (ReleaseNotes, ViewModel mappings, Equatable, progress, notFound): M (1–3h)
• Optional regex/DI improvements: M (1–3h)

Signals to revisit with a more complex approach

• If Sparkle callback threading isn’t guaranteed, or multiple updaters exist, move to a reducer/state-machine with explicit transitions and effects.
• If UI needs richer progress info (ETA, speed), introduce a small, testable progress model rather than raw bytes.