Repository navigation
Keep update pill polling current after first update - #3833
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRoute ChangesUpdate Polling and Version Tracking
Sequence Diagram(s)sequenceDiagram
participant UpdateDriver
participant UpdateViewModel
participant UpdateState
UpdateDriver->>UpdateViewModel: recordAvailableUpdate(update)
UpdateViewModel->>UpdateState: recordDetectedUpdateMetadata(update.appcastItem)
UpdateViewModel->>UpdateState: set state to .updateAvailable(update)
UpdateState-->>UpdateViewModel: publish effectiveState
UpdateViewModel-->>Clients: update visible pill / replies
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
Contributor
Greptile SummaryThis PR fixes #3829 by ensuring the update pill always reflects the latest Sparkle appcast emission rather than staying locked to the first-seen value. The key mechanism is routing all
Confidence Score: 5/5Safe to merge; all new state-transition methods are correct for every current production call path. All three new methods behave correctly for every production call path; the new test suite covers the key emission, rollback, override-refresh, and dismissal scenarios. No files require special attention; the only nuance is the implicit invariant in Important Files Changed
Sequence DiagramsequenceDiagram
participant Sparkle
participant UpdateDriver
participant UpdateViewModel
participant UI
Note over Sparkle,UI: Repeated background poll scenario (bug fix)
Sparkle->>UpdateDriver: didFindValidUpdate(item v9.9.0)
UpdateDriver->>UpdateViewModel: recordDetectedUpdate(v9.9.0)
UpdateViewModel-->>UI: "detectedUpdateVersion = "9.9.0""
Sparkle->>UpdateDriver: showUpdateFound(v9.9.0, reply1)
UpdateDriver->>UpdateViewModel: recordAvailableUpdate(v9.9.0, reply1)
UpdateViewModel-->>UI: "state = .updateAvailable(v9.9.0)"
Note over Sparkle,UI: Second poll - newer version
Sparkle->>UpdateDriver: showUpdateFound(v9.9.1, reply2)
UpdateDriver->>UpdateViewModel: recordAvailableUpdate(v9.9.1, reply2)
Note over UpdateViewModel: state and overrideState both updated to reply2
UpdateViewModel-->>UI: pill shows v9.9.1
Note over Sparkle,UI: No-update path (clears stale state)
Sparkle->>UpdateDriver: updaterDidNotFindUpdate
UpdateDriver->>UpdateViewModel: dismissDetectedAvailableUpdate()
UpdateViewModel->>Sparkle: reply2(.dismiss) [once]
UpdateViewModel-->>UI: "state = .idle, overrideState = nil"
Reviews (4): Last reviewed commit: "Prevent duplicate update dismiss replies..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Update/UpdateViewModel.swift`:
- Around line 38-40: When updating the available update payload in
recordAvailableUpdate(_:) you update state with the new reply closure but leave
overrideState’s UpdateAvailable case pointing at the old reply; change the
overrideState when it’s .updateAvailable so it not only replaces the item via
replacingAvailableUpdateItem(with:) but also updates the reply/handler to match
the newly assigned state's reply (i.e., construct a new UpdateAvailable value or
add a helper like replacingReply(...) so overrideState =
overrideState.replacingAvailableUpdateItem(with: item).replacingReply(with:
state.reply)). Apply the same synchronous replacement logic in the other
occurrence noted (lines 43–45) so overrideState’s callback is always kept in
sync with state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f19038fc-fcc5-4105-98e3-beef089ab2c2
📒 Files selected for processing (3)
Sources/Update/UpdateDriver.swiftSources/Update/UpdateViewModel.swiftcmuxTests/UpdatePillReleaseVisibilityTests.swift
The update pill can be backed by an interactive update state and a background-detected update cache at the same time. This regression test models repeated appcast emissions so CI proves that the visible pill version is not allowed to remain pinned to the first update. Constraint: Local tests are intentionally not run for this repo; verification is CI-only. Rejected: XCUITest repro | the model-level ownership bug is lower-risk and faster to validate in the unit target. Confidence: high Scope-risk: narrow Directive: Keep this test focused on visible model behavior rather than source-shape assertions. Tested: git diff --check Not-tested: Swift unit suite, per repo instruction to avoid local tests
austinywang
force-pushed
the
issue-3829-update-pill-keep-polling
branch
from
May 11, 2026 00:04
8d16cf6 to
3d5ac2c
Compare
The visible update state and the background-detected update cache could diverge after the first available update. Route update-available transitions through the view model and refresh any active updateAvailable state whenever Sparkle reports a valid update item, so later polls replace the item the pill renders instead of being hidden behind the first state. Constraint: The existing Sparkle probe cadence stays in UpdateController; the fix only changes update item ownership in the view model. Rejected: Add a second polling loop | would duplicate Sparkle probing and leave the stale state owner split intact. Rejected: Only remove an early return | no polling early return existed in cmux; the stale item lived in duplicated model state. Confidence: high Scope-risk: narrow Directive: Future update UI should route valid appcast items through UpdateViewModel rather than assigning updateAvailable state directly. Tested: git diff --check Not-tested: Swift unit suite, per repo instruction to avoid local tests
austinywang
force-pushed
the
issue-3829-update-pill-keep-polling
branch
from
May 11, 2026 00:06
3d5ac2c to
d99b4eb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Update/UpdateViewModel.swift (1)
43-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSynchronize
overrideStatewith the new reply closure.When
overrideStateis already.updateAvailable,recordAvailableUpdateupdates only the appcast item (viarecordDetectedUpdateat lines 38-40) but leaves the old reply closure in place. This meanseffectiveStatecan route user actions to a stale Sparkle reply handler.Suggested fix
func recordAvailableUpdate(_ update: UpdateState.UpdateAvailable) { recordDetectedUpdate(update.appcastItem) state = .updateAvailable(update) + if let overrideState, case .updateAvailable = overrideState { + self.overrideState = .updateAvailable(update) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Update/UpdateViewModel.swift` around lines 43 - 45, recordAvailableUpdate(_:) currently updates the appcast item but doesn't update overrideState's reply closure when overrideState is already .updateAvailable, causing effectiveState to use a stale reply; modify recordAvailableUpdate(_ update: UpdateState.UpdateAvailable) so that after calling recordDetectedUpdate(update.appcastItem) it also sets overrideState = .updateAvailable(update) (or updates the existing overrideState's reply closure) to ensure the new reply closure is used; reference overrideState, recordAvailableUpdate(_:), recordDetectedUpdate(_:) and effectiveState when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/Update/UpdateViewModel.swift`:
- Around line 43-45: recordAvailableUpdate(_:) currently updates the appcast
item but doesn't update overrideState's reply closure when overrideState is
already .updateAvailable, causing effectiveState to use a stale reply; modify
recordAvailableUpdate(_ update: UpdateState.UpdateAvailable) so that after
calling recordDetectedUpdate(update.appcastItem) it also sets overrideState =
.updateAvailable(update) (or updates the existing overrideState's reply closure)
to ensure the new reply closure is used; reference overrideState,
recordAvailableUpdate(_:), recordDetectedUpdate(_:) and effectiveState when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 496233dd-1efa-48a9-94b6-2b9104d28dda
📒 Files selected for processing (3)
Sources/Update/UpdateDriver.swiftSources/Update/UpdateViewModel.swiftcmuxTests/UpdatePillReleaseVisibilityTests.swift
Passive appcast detection now records metadata only, while installable update transitions carry the full SUAppcastItem and reply closure through recordAvailableUpdate. This keeps visible update actions tied to the Sparkle callback that produced the installable state, including when an override pill is already visible. Constraint: Do not run local tests; CI owns test execution for this repo. Rejected: Replace only the appcast item inside existing updateAvailable state | it preserves stale reply closures and can route Install/Dismiss to the wrong Sparkle callback. Confidence: high Scope-risk: narrow Directive: Do not refresh UpdateAvailable display data without refreshing the matching reply closure from the same Sparkle emission. Tested: git diff --check Not-tested: Local XCTest execution per repository policy
The PR branch needs the latest mainline changes before CI can prove the update-pill fix against the current tree. The merge was conflict-free, so the update-state changes remain isolated while CI reruns on the actual merge candidate. Constraint: Required PR iteration against origin/main before CI handoff. Confidence: high Scope-risk: moderate Tested: conflict-free git merge origin/main Not-tested: Local tests per repository policy
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5aff258. Configure here.
A user-triggered update check can cancel the current Sparkle update reply while a queued no-update callback is still waiting on the main queue. The view model now provides one cancellation path that replies once, clears the interactive state, and removes any visible override before the next check starts. Constraint: Sparkle reply callbacks must be answered exactly once per update interaction Rejected: Only set state to idle inline in UpdateController | would keep the cancellation semantics split across controller and view model paths Confidence: high Scope-risk: narrow Tested: ./scripts/reload.sh --tag issue-3829-update-pill-keep-polling Not-tested: Local test execution per repository policy; CI will run the regression test
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3829.
Verification
Note
Medium Risk
Moderate risk because it changes update state transitions and dismissal/cancel behavior (including Sparkle reply callbacks), which could affect install/dismiss flows if mis-handled.
Overview
Ensures the update pill reflects the latest valid Sparkle appcast emission (including rollbacks) by recording update metadata separately and routing
.updateAvailabletransitions throughUpdateViewModel.recordAvailableUpdateso the visible version and associated reply stay current.Tightens lifecycle handling around re-checks and “no update found”:
checkForUpdatesnow cancels/reset state viacancelActiveStateForNewCheck(), andupdaterDidNotFindUpdatecallsdismissDetectedAvailableUpdate()to clear cached detection, reset UI back to.idle, and send a single.dismissreply for any active/override.updateAvailable.Adds
UpdateViewModelLatestEmissionTestsregression coverage for repeated emissions, rollback behavior, reply freshness when overrides are visible, and avoiding double-reply on cancel+dismiss.Reviewed by Cursor Bugbot for commit 0ace395. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keeps the update pill tied to the latest
Sparkleappcast emission so newer polls update the visible version (when idle), rollbacks revert it, and rechecks avoid duplicate dismiss replies. Fixes #3829..updateAvailabletransitions throughUpdateViewModel.recordAvailableUpdate(viaUpdateDriver.applyState) to refresh visible and override state with the latestSUAppcastItemand reply.recordDetectedUpdateupdates the pill when idle and never swaps an active installable update, keeping actions tied to the matchingSparklereply.dismissDetectedAvailableUpdatesends.dismissonce and resets detected/visible state to.idle.cancelActiveStateForNewCheckcancels the interactive/override state to prevent duplicate dismiss replies.Written for commit 0ace395. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests