Refactor: extract Sparkle self-update into CmuxUpdater package (WIP)#5132
Conversation
Leaf-first extraction of the Sparkle self-update domain into a new package. This commit lands the parts with no remaining app-target coupling: - Package.swift: Sparkle from 2.5.1, Swift 6 mode, ExistentialAny + InternalImportsByDefault (resolves to Sparkle 2.9.2). - UpdateTiming: display/timeout constants (DocC). - UpdateFeedResolver: redesigned from a namespace-enum into an injectable struct returning a named Resolution (+ 4 Swift Testing behavioral tests). - UpdateLogging: protocol seam so the package logs through the app's existing UpdateLogStore (which stays in the app target) instead of a .shared singleton. - UpdateState: the phase enum lifted into its own public file. - UpdateStateModel: ObservableObject -> @mainactor @observable. All state writes funnel through setState/setOverrideState which emit on a stateChanges() AsyncStream<Void> (the @Observable-native replacement for $state.sink / CombineLatest). Content/string computeds kept; SwiftUI Color computeds deferred to CmuxUpdaterUI; errorDetails takes logPath as a param. Controller redesign, driver, UI package, pbxproj wiring, and AppDelegate inversion are the remaining steps (not yet wired into the app). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors the updater into two Swift packages, replaces the observable view model with UpdateState/UpdateStateModel and AsyncStream, injects clock/logging/action seams, refactors driver/controller to drive the model, exposes UI components, updates host wiring and Xcode project, expands tests, and tightens timing guidance. ChangesUpdater modularization & core
CmuxUpdaterUI package & UI components
Host app integration & build
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…DispatchSource) Allow a bounded, cancellable, injected-Clock delay/deadline; keep banning sleep-as-synchronization (poll/settle/race) and DispatchQueue.asyncAfter; demote DispatchSource.makeTimerSource to 'only outside an async context'. This is the policy the CmuxUpdater controller redesign will follow (Clock.sleep for the min-display / auto-dismiss / timeout delays). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the runtime side of the package (builds + 12 tests green via swift test). Seams (dependency inversion; app conforms + injects): - UpdateLogging: package logs through the app's UpdateLogStore (stays in app target). - UpdateClock / SystemUpdateClock: injectable bounded-delay clock (Task.sleep behind it). - UpdateActionDelegate: retry + relaunch-prep the package can't do itself. - UpdateSettings: Sparkle defaults/migration as a value type (was a namespace-enum). UpdateDriver (SPUUserDriver + SPUUpdaterDelegate): SPU* protocols are NS_SWIFT_UI_ACTOR (@mainactor), so the driver is a clean @mainactor NSObject with zero runOnMain / DispatchQueue.main.async. The two min-display / check-timeout sites become cancellable clock.sleep Tasks. AppDelegate.checkForUpdates / AppDelegate.shared / TerminalController.shared reach-ins route through UpdateActionDelegate. UpdateController: the three Combine observers (force-install $state.sink, attempt-update $state.sink, no-update CombineLatest) collapse into one long-lived @mainactor task over model.stateChanges() plus reaction flags. The 0.25s x 20 readiness retry-poll becomes a canCheckForUpdates KVO observation with a clock.sleep timeout; the hourly background-probe Timer becomes a clock.sleep loop; the 100ms re-check settle delay becomes a next-turn Task. DEBUG test scaffolding (UpdateTestURLProtocol, UpdateTestSupport) ported to Swift 6: NSLock registration guard -> @mainactor; DispatchQueue hops -> @mainactor + Task.sleep. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cosmetic folder grouping (same module, build still green): UpdateTestSupport + UpdateTestURLProtocol move to Sources/CmuxUpdater/TestSupport/ to signal they are scaffolding, not shipping logic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…3 tests green) New UI package depending on CmuxUpdater + Sparkle. Ports UpdatePill (+ popover anchor + InstallUpdateMenuItem), UpdateBadge (+ ProgressRingView, BrowserStyleLoadingSpinner), and UpdatePopoverView (+ its subviews) off the ObservableObject view model onto the @observable UpdateStateModel. Seams/decisions: - UpdateActionsHost: the pill/popover call checkForUpdatesInCustomUI / attemptUpdate and read updateLogPath through this protocol instead of AppDelegate.shared. - UpdateAppearance(accent:): the SwiftUI Color computeds deferred from the model now live here, parameterized on the injected host accent color (cmuxAccentColor stays in the app target). Only the pill/badge need it; the popover uses standard colors. - The 'no updates' dismiss animation moved from the controller (withAnimation, which would pull SwiftUI into the domain layer) to the pill via .animation(value: showsPill). - .safeHelp inlined (was in app-target Backport.swift); popover Coordinator marked @mainactor for the AppKit NSPopover access under Swift 6. Not yet wired into the app target (pbxproj + AppDelegate inversion is the next step). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…move update god-code The app integration of the updater extraction. cmux + cmux-unit both build green. - pbxproj: link CmuxUpdater + CmuxUpdaterUI into the cmux target (mirrors the CmuxSettings/CmuxSettingsUI pattern; cmuxTests resolves them through the host app). - Delete the 10 moved files from Sources/Update/ (now in the packages); keep UpdateTitlebarAccessory, MinimalModeSidebarControls, TitlebarChromeGeometryReporting, UpdateLogStore (-2600 lines). - AppDelegate: owns the single UpdateLogStore instance and injects it (no more static .shared); constructs UpdateController(log:) + sets actionDelegate = self; conformance to UpdateActionDelegate + UpdateActionsHost lives in a dedicated extension (retry -> checkForUpdates, relaunch prep -> persist/stop/invalidate, attemptUpdate(), updateLogPath); overrideState writes go through setOverrideState; validateMenuItem returns true (the controller's old behavior). - UpdateLogStore: drop the singleton; conform to UpdateLogging; public init(). - UpdateTitlebarAccessoryController: drop the vestigial weak updateViewModel (no cycle, never read) and inject the UpdateLogStore it actually uses for its 3 log lines. - ContentView: @ObservedObject UpdateViewModel -> @observable UpdateStateModel refs; UpdatePill gets accent (cmuxAccentColor) + the actions host. cmuxApp imports the UI. - cmuxTests: migrate to the new API (UpdateStateModel + setState/setOverrideState, UpdateFeedResolver()/UpdateSettings() instances, debugSetDetectedVersion); @mainactor on the model-touching suite. Cross-toolchain build fixes (xcodebuild app build is the gate; SwiftPM resolves a newer Sparkle than the app pins): - @preconcurrency conformance to SPUUserDriver/SPUUpdaterDelegate (app toolchain doesn't treat the NS_SWIFT_UI_ACTOR protocols as @mainactor). - Drop the explicit .hardwareDoesNotSupportARM64 case (not in the app's pinned Sparkle); @unknown default covers it. - Readiness wait: KVO on the @mainactor SPUUpdater 'sends' a non-Sendable into the change handler under strict concurrency, so canCheckForUpdates is awaited with a bounded injected-clock retry (behavior-identical to the original 0.25s x 20 poll). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ShortcutAndCommandPaletteTests: UpdateFeedResolver.fallbackFeedURL was left as a static reference but it is now an instance property, so cmux-unit failed to compile (caught by Codex review; missed by my local cmux-unit 'build' which didn't recompile the test sources). Use a shared resolver instance for resolve() + fallbackFeedURL. - UpdateController: drop the stale doc line claiming readiness is 'observed via KVO'; it uses a bounded injected-clock retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI (Xcode 16.4, Sparkle 2.8.1) failed to emit the CmuxUpdater module: three SPUUserDriver witnesses (show/showUpdateFound/showReady) declared their reply closures @sendable, but Sparkle 2.8.1's protocol declares them non-@sendable, so under the package's strict Swift 6 mode the witness sendability didn't match the requirement. (Local Xcode 26 resolves a newer Sparkle where they ARE @sendable, so it matched there; the original app-target code had @sendable too but compiled because the app target isn't strict Swift 6.) Fix: @preconcurrency on the Sparkle imports across both packages, which downgrades these cross-version Sendability mismatches to warnings. Canonical fix for conforming to an Obj-C framework protocol whose closure-parameter Sendability varies by version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The package standalone swift-build had resolved Sparkle 2.9.2 (newest in the upToNextMajor-from-2.5.1 range) while the app/nightly pins 2.8.1. That skew is why my local 'swift build' passed but CI (Sparkle 2.8.1) failed on the @sendable SPUUserDriver reply mismatch. Pin both packages' Package.resolved to 2.8.1 (rev 5581748, the app's resolved revision) so local builds match nightly and would catch this class of cross-version issue. The app's own Sparkle pin is unchanged (2.8.1). With the pin, swift build now compiles against 2.8.1, validating the @preconcurrency fix against the same Sparkle CI uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UpdatePillUITests covered available/detected/notFound/checking/mock-feed/permission but not the error state. Add a CMUX_UI_TEST_UPDATE_STATE=error hook to UpdateTestSupport (+ optional CMUX_UI_TEST_UPDATE_ERROR_MESSAGE) and testUpdatePillShowsErrorStateWithRetryAndDetails, which asserts the 'Update Failed' pill and that clicking it opens the error popover with Retry + Copy Details (exercising UpdateErrorView + errorDetails + the retry seam). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Synthetic CMUX_UI_TEST_UPDATE_STATE cases (downloading/extracting/installing) +
three tests asserting the pill labels ('Downloading: 50%', 'Preparing: 50%',
'Installing…') and the installing popover's Restart Now/Later buttons. Covers the
DownloadingView/ExtractingView/InstallingView + ProgressRingView rendering without a
live install. (A true download→extract→install→relaunch needs a real signed update
and isn't reliably E2E-testable.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clicking 'Check for Updates' while the pill already shows an update dismissed it but the re-check silently dropped, so the pill just hid until a second click. The original delayed the post-dismiss re-check by 100ms to let Sparkle tear down the dismissed session; I'd replaced that with a next-turn Task that fired too soon (Sparkle coalesced the re-check). Restore the delay as a bounded, cancellable clock.sleep(100ms) on a stored recheckTask. Caught via the live update log during dogfood. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR extracts the Sparkle self-update domain from the flat app target into two new SwiftPM packages (
Confidence Score: 5/5Safe to merge; no runtime-breaking changes, and the PR explicitly does not wire the packages into any active user path yet. All state writes are main-actor isolated, all bounded delays go through the injected UpdateClock seam, and the new packages build and test green in isolation. The two findings are non-blocking style observations that don't affect correctness on any live code path in this commit. UpdateStateModel.swift — the dismissDetectedAvailableUpdate double-notification and the 485-line mixed-concern file are worth addressing before this leaves draft. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as AppDelegate
participant Ctrl as UpdateController
participant Updater as SPUUpdater
participant Driver as UpdateDriver
participant Model as UpdateStateModel
participant UI as UpdatePill/PopoverView
participant Stream as stateChanges() AsyncStream
App->>Ctrl: startUpdaterIfNeeded()
Ctrl->>Updater: updater.start()
Ctrl->>Updater: checkForUpdateInformation()
Note over Ctrl,Stream: startStateReactions() — long-lived @MainActor Task
Ctrl->>Stream: for await _ in stateChanges()
App->>Ctrl: checkForUpdates()
Ctrl->>Ctrl: checkForUpdatesWhenReady()
alt canCheckForUpdates
Ctrl->>Updater: checkForUpdates()
else not ready
Ctrl->>Model: setState(.checking)
Ctrl->>Ctrl: waitForReadinessThenCheck() (clock.sleep × 20)
Ctrl->>Updater: checkForUpdates()
end
Updater->>Driver: showUserInitiatedUpdateCheck(cancellation:)
Driver->>Model: setState(.checking)
Model->>Stream: notifyStateChanged()
Stream->>Ctrl: handleStateChange()
Updater->>Driver: showUpdateFound(appcastItem:reply:)
Driver->>Model: applyDriverState(.updateAvailable)
Model->>Stream: notifyStateChanged()
Stream->>Ctrl: handleStateChange()
UI->>Model: observe effectiveState → render UpdateAvailableView
App->>Ctrl: installUpdate()
Ctrl->>Ctrl: "isForceInstalling = true"
Ctrl->>Model: state.confirm() → reply(.install)
Updater->>Driver: showInstallingUpdate(...)
Driver->>Model: setState(.installing)
Model->>Stream: notifyStateChanged()
Stream->>Ctrl: "handleStateChange() → evaluateForceInstall → isForceInstalling = false"
Reviews (4): Last reviewed commit: "Address CodeRabbit (df968da): use inject..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return nsError.localizedDescription | ||
| } |
There was a problem hiding this comment.
userFacingErrorMessage returns nsError.localizedDescription directly. For unrecognized Sparkle or Foundation errors this can surface implementation details such as internal domain names, raw error descriptions from the vendor ("The operation couldn't be completed. (org.sparkle-project.Sparkle error 4006.)"), or similar strings. The custom rule requires that user-facing copy describe the situation in cmux terms and hold implementation details to sanitized logs only — the same pattern the userFacingErrorTitle fallback already uses correctly.
| return nsError.localizedDescription | |
| } | |
| return String(localized: "update.error.failed.message", defaultValue: "Something went wrong while checking for updates. Try again or check the update log for details.") | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed - the catch-all now returns a sanitized, cmux-phrased message ("Something went wrong while checking for updates. Try again, or check the update log for details.") via String(localized: update.error.failed.message). The raw vendor description/domain/code stay in the copyable Details block + the update log, matching the userFacingErrorTitle fallback pattern.
— Claude Code
| public func cancelActiveStateForNewCheck() { | ||
| state.cancel() | ||
| setState(.idle) | ||
| setOverrideState(nil) | ||
| } |
There was a problem hiding this comment.
cancelActiveStateForNewCheck calls setState and then setOverrideState, each of which independently calls notifyStateChanged(). This fires two separate emissions on the stateChanges() stream for a single conceptual transition, causing UpdateController.handleStateChange() — and therefore scheduleNoUpdateDismiss — to execute twice. In the current implementation this is harmless because the guard checks are idempotent, but it makes every future subscriber of stateChanges() pay for an extra notification and re-schedule the dismiss timer on each step. Consider factoring into a single combined write (update both fields, then call notifyStateChanged() once).
There was a problem hiding this comment.
Fixed - cancelActiveStateForNewCheck now updates both state and overrideState then calls notifyStateChanged() once, so a single logical reset emits one stateChanges() event instead of two.
— Claude Code
| private func transition(to state: UpdateState) { | ||
| model.applyDriverState(.checking(.init(cancel: {}))) | ||
| let model = self.model | ||
| Task { @MainActor in | ||
| try? await Task.sleep(for: .milliseconds(200)) | ||
| model.applyDriverState(state) | ||
| } | ||
| } |
There was a problem hiding this comment.
transition(to:) uses a bare Task.sleep(for: .milliseconds(200)) to create a brief "checking" visual before switching to the target state. This is a settlement/race sleep rather than a bounded, injected-clock delay driven by a real signal, which the updated CLAUDE.md and review rules now explicitly reject even in DEBUG scaffolding. Since UpdateTestSupport already receives the model and log as seams, the clock can be threaded in the same way and used here (clock.sleep rather than Task.sleep) so tests can advance virtual time with no real waiting. The same Task.sleep on line 85 inside performMockFeedCheckIfNeeded is in the same category.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Intentional, and allowed by the same CLAUDE.md rule: 'Deterministic sleeps remain fine in tests and explicit test-only scaffolding.' UpdateTestSupport is #if DEBUG, gated on CMUX_UI_TEST_MODE, and exists only to stage UI-test states - the bounded/injected-clock requirement targets shipping runtime code. The two Task.sleep calls are deterministic test-only staging delays, so I'm leaving them rather than threading a clock seam into DEBUG scaffolding.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux.xcodeproj/project.pbxproj (1)
2844-2879:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing app suffix build settings in both configs.
cmuxDebug/Release defineCMUX_SIDEBAR_EXTENSION_POINT_ID, but they still omitCMUX_BUNDLE_ID_SUFFIXandCMUX_DISPLAY_NAME_SUFFIX. That leaves the shared app-target build-setting contract incomplete.Suggested pbxproj patch
A5001082 /* Debug */ = { isa = XCBuildConfiguration; buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = "AppIcon-Debug"; CMUX_AUTH_CALLBACK_SCHEME = "cmux-dev"; + CMUX_BUNDLE_ID_SUFFIX = ""; + CMUX_DISPLAY_NAME_SUFFIX = ""; CMUX_SIDEBAR_EXTENSION_POINT_ID = com.manaflow.cmux.sidebar; CODE_SIGN_ENTITLEMENTS = ""; @@ A5001083 /* Release */ = { isa = XCBuildConfiguration; buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; CMUX_AUTH_CALLBACK_SCHEME = cmux; + CMUX_BUNDLE_ID_SUFFIX = ""; + CMUX_DISPLAY_NAME_SUFFIX = ""; CMUX_SIDEBAR_EXTENSION_POINT_ID = com.manaflow.cmux.sidebar; CODE_SIGN_ENTITLEMENTS = Resources/cmux.entitlements;As per coding guidelines,
cmux.xcodeproj/project.pbxproj: AddCMUX_SIDEBAR_EXTENSION_POINT_ID(defaultcom.manaflow.cmux.sidebar),CMUX_BUNDLE_ID_SUFFIX(default empty), andCMUX_DISPLAY_NAME_SUFFIX(default empty) build settings to the app and appex targets in all build configs.Also applies to: 2885-2920
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 2844 - 2879, The Debug/Release build configurations define CMUX_SIDEBAR_EXTENSION_POINT_ID but are missing the app-suffix settings CMUX_BUNDLE_ID_SUFFIX and CMUX_DISPLAY_NAME_SUFFIX; update the project.pbxproj entries for the app and appex targets (where CMUX_SIDEBAR_EXTENSION_POINT_ID appears) to add CMUX_BUNDLE_ID_SUFFIX = "" and CMUX_DISPLAY_NAME_SUFFIX = "" alongside the existing CMUX_SIDEBAR_EXTENSION_POINT_ID setting in every build config so the shared app-target build-setting contract is complete.
🤖 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 `@cmux.xcodeproj/project.pbxproj`:
- Around line 1898-1899: The project file currently registers the CmuxUpdater
and CmuxUpdaterUI package products only for the cmux app target; add the
corresponding package wiring for the unit-test target (cmuxTests) by creating an
XCLocalSwiftPackageReference and an XCSwiftPackageProductDependency for each
package product (CmuxUpdater and CmuxUpdaterUI) and add PBXBuildFile entries in
the Frameworks phase of the cmuxTests target so both targets (cmux and
cmuxTests) link the same local package products; ensure the new entries mirror
the ones you added for the cmux target and reference the same package/product
identifiers (CmuxUpdater, CmuxUpdaterUI) so imports in extracted modules resolve
in tests.
In `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift`:
- Around line 255-282: Replace the polling loop inside
waitForReadinessThenCheck()/readyCheckTask with a KVO observation on the
updater.canCheckForUpdates property: create and store an NSKeyValueObservation
(e.g. canCheckObservation) that watches self.updater.canCheckForUpdates and,
when the observed value becomes true, calls performCheckForUpdates() and
invalidates/removes the observation; remove the
readyRetryCount/readyRetryDelay/clock.sleep retry logic and ensure any existing
observation or readyCheckTask is cancelled/invalidated before creating a new one
and that you still append the timeout/error state via log.append and
model.setState(.error(...)) if you decide to keep a fallback; reference symbols:
waitForReadinessThenCheck, readyCheckTask, updater.canCheckForUpdates,
performCheckForUpdates, readyRetryCount/readyRetryDelay, clock, log.append,
model.setState.
In `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateSettings.swift`:
- Around line 27-32: Document the public initializer UpdateSettings.init by
adding a documentation comment above it that includes a brief summary and two `-
Parameter` callouts for `scheduledCheckInterval` and
`previousDefaultScheduledCheckInterval`, describing what each parameter controls
and their default values (60 * 60 and 60 * 60 * 24 respectively); ensure the
comment follows the package's public API doc style used in Packages/**/*.swift.
In `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateState.swift`:
- Around line 106-298: The file exposes many public nested payload types
(NotFound, PermissionRequest, Checking, UpdateAvailable, ReleaseNotes, Error,
Downloading, Extracting, Installing) under UpdateState which violates the "one
major type per file" guideline; either split each public type into its own file
named after the type (e.g., UpdateAvailable.swift containing public struct
UpdateAvailable) and keep their public API, or if they are truly internal
implementation details of UpdateState, reduce their visibility to
internal/private and keep them nested; update access levels and imports
accordingly and ensure UpdateState references (UpdateState.UpdateAvailable,
etc.) still compile after the change.
In `@Packages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View`+SafeHelp.swift:
- Around line 4-6: Add a Swift-DocC triple-slash comment for the public API func
safeHelp(_ text: String) with a one-sentence summary and a parameter callout;
specifically prepend /// A one-line summary describing that the modifier applies
a help tooltip only when the provided text is non-empty. Then add a "- Parameter
text: Description of the tooltip text shown when non-empty." callout (and add "-
Returns:" only if needed), ensuring the doc uses /// style above the
safeHelp(_:) declaration.
---
Outside diff comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 2844-2879: The Debug/Release build configurations define
CMUX_SIDEBAR_EXTENSION_POINT_ID but are missing the app-suffix settings
CMUX_BUNDLE_ID_SUFFIX and CMUX_DISPLAY_NAME_SUFFIX; update the project.pbxproj
entries for the app and appex targets (where CMUX_SIDEBAR_EXTENSION_POINT_ID
appears) to add CMUX_BUNDLE_ID_SUFFIX = "" and CMUX_DISPLAY_NAME_SUFFIX = ""
alongside the existing CMUX_SIDEBAR_EXTENSION_POINT_ID setting in every build
config so the shared app-target build-setting contract is complete.
🪄 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: 4175d1eb-7b59-4aa2-bf1a-f5f009cd265f
📒 Files selected for processing (39)
CLAUDE.mdPackages/CmuxUpdater/Package.resolvedPackages/CmuxUpdater/Package.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/TestSupport/UpdateTestSupport.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/TestSupport/UpdateTestURLProtocol.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateActionDelegate.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateClock.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver+SPUUpdaterDelegate.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateDriver.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateFeedResolver.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateLogging.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateSettings.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateState.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateStateModel.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateTiming.swiftPackages/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateFeedResolverTests.swiftPackages/CmuxUpdater/Tests/CmuxUpdaterTests/UpdateStateModelTests.swiftPackages/CmuxUpdaterUI/Package.resolvedPackages/CmuxUpdaterUI/Package.swiftPackages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateActionsHost.swiftPackages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateAppearance.swiftPackages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdateBadge.swiftPackages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePill.swiftPackages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePopoverView.swiftPackages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View+SafeHelp.swiftPackages/CmuxUpdaterUI/Tests/CmuxUpdaterUITests/UpdateAppearanceTests.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/Update/UpdateController.swiftSources/Update/UpdateLogStore.swiftSources/Update/UpdateTestSupport.swiftSources/Update/UpdateTiming.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/FileDropOverlayViewTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swiftcmuxUITests/UpdatePillUITests.swift
💤 Files with no reviewable changes (3)
- Sources/Update/UpdateController.swift
- Sources/Update/UpdateTestSupport.swift
- Sources/Update/UpdateTiming.swift
| CDFEED0200000000CDFEED02 /* CmuxUpdater */, | ||
| CDFEED0500000000CDFEED05 /* CmuxUpdaterUI */, |
There was a problem hiding this comment.
Wire the updater packages into the unit-test target too.
These additions only register CmuxUpdater/CmuxUpdaterUI on cmux. The unit-test side in this project file (cmuxTests and its Frameworks phase) still has no matching package product dependencies, so the project wiring is incomplete for the extracted modules.
As per coding guidelines, cmux.xcodeproj/project.pbxproj: Wire a new local package into cmux.xcodeproj by adding one XCLocalSwiftPackageReference, one XCSwiftPackageProductDependency, and a PBXBuildFile in the Frameworks phase of every target that imports it; link into both cmux and cmux-unit targets.
Also applies to: 2040-2041, 3301-3310
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 1898 - 1899, The project file
currently registers the CmuxUpdater and CmuxUpdaterUI package products only for
the cmux app target; add the corresponding package wiring for the unit-test
target (cmuxTests) by creating an XCLocalSwiftPackageReference and an
XCSwiftPackageProductDependency for each package product (CmuxUpdater and
CmuxUpdaterUI) and add PBXBuildFile entries in the Frameworks phase of the
cmuxTests target so both targets (cmux and cmuxTests) link the same local
package products; ensure the new entries mirror the ones you added for the cmux
target and reference the same package/product identifiers (CmuxUpdater,
CmuxUpdaterUI) so imports in extracted modules resolve in tests.
There was a problem hiding this comment.
Verified not needed for this project's structure. cmuxTests has an empty Frameworks phase and only lists CMUXAgentLaunch; it imports Sparkle/Bonsplit (and now CmuxUpdater) via '@testable import cmux' (host resolution), not direct linkage. The cmux-unit 'tests' job passed on this branch (run 26827655110) and Codex's build-for-testing compiled cmuxTests, both with cmux-only wiring. This also matches the CmuxSettings/CmuxSettingsUI precedent (cmux-only, same AppDelegate/ContentView consumers). Adding explicit test-target linkage would deviate from the working pattern, so leaving as-is.
— Claude Code
There was a problem hiding this comment.
@azooz2003-bit Thanks for verifying and for the detailed context. That makes sense given this project’s host-based test resolution and the existing CmuxSettings/CmuxSettingsUI precedent. I’ll withdraw this finding — no change needed here, and leaving CmuxUpdater/CmuxUpdaterUI wired only to cmux is consistent with the current project structure.
🐇💭
✏️ Learnings added
Learnt from: azooz2003-bit
Repo: manaflow-ai/cmux PR: 5132
File: cmux.xcodeproj/project.pbxproj:1932-1933
Timestamp: 2026-06-02T17:38:56.685Z
Learning: In `manaflow-ai/cmux`, `cmux.xcodeproj/project.pbxproj` does not need every app-target Swift package product repeated on the `cmuxTests` unit-test target. `cmuxTests` is host-based and resolves packages such as `Sparkle`, `Bonsplit`, `CmuxSettings`, `CmuxSettingsUI`, and `CmuxUpdater` through `testable import cmux`; its Frameworks phase can remain empty and its `packageProductDependencies` may list only direct test-target dependencies such as `CMUXAgentLaunch`. Do not flag missing `cmuxTests` package wiring for app-only imports unless tests directly import/link the package outside the hosted `cmux` module pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3401
File: comeup/simulator-harness/ComeupSimulatorHarness.xcodeproj/project.pbxproj:470-478
Timestamp: 2026-05-01T21:00:25.900Z
Learning: For Xcode projects where a Swift Package Manager dependency is wired as a local package through an .xcworkspace (i.e., the package is referenced in contents.xcworkspacedata with a `group:` entry pointing to the local package), do not require or flag a missing `XCRemoteSwiftPackageReference` section in the project .pbxproj. In this setup, `XCSwiftPackageProductDependency` entries should be resolved via the workspace reference to the local package directory; treat the dependency as satisfied without `XCRemoteSwiftPackageReference`, especially in multi-project workspaces.
| private func waitForReadinessThenCheck() { | ||
| readyCheckTask = Task { @MainActor [weak self] in | ||
| guard let self else { return } | ||
| var remaining = self.readyRetryCount | ||
| while remaining > 0 { | ||
| if self.updater.canCheckForUpdates { | ||
| self.performCheckForUpdates() | ||
| return | ||
| } | ||
| remaining -= 1 | ||
| // Bounded readiness wait on the injected clock (see property comment). | ||
| try? await self.clock.sleep(for: self.readyRetryDelay) | ||
| if Task.isCancelled { return } | ||
| } | ||
| self.log.append("checkForUpdatesWhenReady timed out") | ||
| if case .checking = self.model.state { | ||
| self.model.setState(.error(.init( | ||
| error: NSError( | ||
| domain: "cmux.update", | ||
| code: 1, | ||
| userInfo: [NSLocalizedDescriptionKey: "Updater is still starting. Try again in a moment."] | ||
| ), | ||
| retry: { [weak self] in self?.checkForUpdates() }, | ||
| dismiss: { [weak self] in self?.model.setState(.idle) } | ||
| ))) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Sparkle 2.5.1 provide an async or notification-based API to observe when SPUUpdater.canCheckForUpdates becomes true, compatible with Swift 6 strict concurrency?
💡 Result:
Sparkle 2.5.1 does not provide a native async/await or notification-based API specifically for observing the canCheckForUpdates property. The SPUUpdater.canCheckForUpdates property is designed for synchronous menu item validation and is Key-Value Observing (KVO) compliant [1][2][3]. To observe this property in a way that is compatible with Swift 6 strict concurrency and modern SwiftUI patterns, the recommended approach is to wrap the KVO observation into an ObservableObject using Combine's publisher API [1][4]. Compatible Implementation Pattern: You can use updater.publisher(for: \.canCheckForUpdates) to create a publisher and assign it to an @Published property within an ObservableObject [1][5][4]. This is the standard way to bridge Sparkle’s KVO-compliant property into SwiftUI or async-friendly reactive flows [1][4]. Example for SwiftUI integration: final class CheckForUpdatesViewModel: ObservableObject { @Published var canCheckForUpdates = false private var cancellables = Set init(updater: SPUUpdater) { updater.publisher(for: .canCheckForUpdates).receive(on: DispatchQueue.main).assign(to: &$canCheckForUpdates) } } Important Concurrency Note: The canCheckForUpdates property must be accessed on the main thread [2]. When working within Swift 6 strict concurrency (e.g., using MainActor isolation), ensure that any observation or updates to your UI state remain on the main actor [2]. Since Combine's assign(to: &) is effectively designed to work with UI state on the main thread, it remains a suitable bridge for modern Swift applications.
Citations:
- 1: https://sparkle-project.github.io/documentation/programmatic-setup
- 2: https://sparkle-project.github.io/documentation/api-reference/Classes/SPUUpdater.html
- 3: https://github.com/sparkle-project/sparkle-project.github.io/blob/master/documentation/api-reference/Classes/SPUUpdater.html
- 4: https://medium.com/@borto_ale/integrating-sparkle-updater-in-swiftui-for-macos-82ae4e0b4ac6
- 5: https://stackoverflow.com/questions/75718697/how-to-fix-sparkle-unable-to-check-for-updates
🌐 Web query:
Sparkle SPUUpdater canCheckForUpdates becomes true notification delegate callback
💡 Result:
In the Sparkle framework, SPUUpdater's canCheckForUpdates property is a KVO-compliant boolean that indicates whether a user-initiated update check is currently allowed [1][2][3]. It is primarily intended for menu item validation (e.g., enabling or disabling a 'Check for Updates' menu item) rather than acting as a trigger for a delegate callback [1][2][3]. There is no direct notification or delegate callback triggered automatically when the canCheckForUpdates property changes from false to true. However, because the property is KVO-compliant, you can observe changes to it programmatically [1][2][3]. To respond to changes in whether an update check is possible, you should use Key-Value Observing (KVO) on the updater instance [1][3]: 1. Register an observer for the key path canCheckForUpdates on your SPUUpdater instance. 2. In your observer's observeValue(forKeyPath:of:change:context:) method (or the Swift closure-based observer), update your UI (e.g., enabling/disabling a button or menu item) based on the new boolean value. If your goal is to control whether an update check is allowed at a specific time, you should implement the SPUUpdaterDelegate method: - (BOOL)updater:(SPUUpdater *)updater mayPerformUpdateCheck:(SPUUpdateCheck)updateCheck error:(NSError)error; This delegate method is called by Sparkle when it is considering whether to perform a check [4][5]. Returning NO from this method prevents the update check from proceeding [4][5]. If you need to prevent scheduled checks while allowing manual ones, or vice versa, you can inspect the SPUUpdateCheck enum passed to this delegate method [4][5].
Citations:
- 1: https://sparkle-project.github.io/documentation/api-reference/Classes/SPUUpdater.html
- 2: https://sparkle-project.org/documentation/programmatic-setup/
- 3: https://github.com/sparkle-project/sparkle-project.github.io/blob/master/documentation/api-reference/Classes/SPUUpdater.html
- 4: https://sparkle-project.github.io/documentation/api-reference/Protocols/SPUUpdaterDelegate.html
- 5: https://sparkle-project.org/documentation/api-reference/Protocols/SPUUpdaterDelegate.html
Replace the readiness polling loop with an NSKeyValueObservation on SPUUpdater.canCheckForUpdates (Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift, lines 255-282)
- Sparkle’s
SPUUpdater.canCheckForUpdatesis KVO-compliant but does not provide a notification/delegate callback when it flips fromfalse→true; the currentwhile+clock.sleeploop is polling. - Observe
canCheckForUpdatesviaNSKeyValueObservation(or the closure-based KVO API) and triggerperformCheckForUpdates()as soon as it becomestrue, removing the retry/sleep approach. SPUUpdaterDelegate.updater(_:mayPerformUpdateCheck:error:)is only consulted when Sparkle is already evaluating an update check, not a “become ready” signal.
🤖 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 `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift` around lines
255 - 282, Replace the polling loop inside
waitForReadinessThenCheck()/readyCheckTask with a KVO observation on the
updater.canCheckForUpdates property: create and store an NSKeyValueObservation
(e.g. canCheckObservation) that watches self.updater.canCheckForUpdates and,
when the observed value becomes true, calls performCheckForUpdates() and
invalidates/removes the observation; remove the
readyRetryCount/readyRetryDelay/clock.sleep retry logic and ensure any existing
observation or readyCheckTask is cancelled/invalidated before creating a new one
and that you still append the timeout/error state via log.append and
model.setState(.error(...)) if you decide to keep a fallback; reference symbols:
waitForReadinessThenCheck, readyCheckTask, updater.canCheckForUpdates,
performCheckForUpdates, readyRetryCount/readyRetryDelay, clock, log.append,
model.setState.
There was a problem hiding this comment.
Intentional. The suggested updater.publisher(for: \.canCheckForUpdates) is Combine, which the package's concurrency rules forbid (no @Published/Combine). Closure-based NSKeyValueObservation also doesn't work here: observing the @mainactor SPUUpdater 'sends' a non-Sendable value into the change handler under strict Swift 6 (compile error), and NSObject-subclass KVO is likewise disallowed. So readiness is awaited with a bounded, cancellable injected-clock retry - behavior-identical to the original 0.25s x 20 poll and testable with a fake clock. Documented in the property comment.
— Claude Code
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /// Payload for ``UpdateState/notFound(_:)``. | ||
| public struct NotFound { | ||
| /// Tells Sparkle the "no update" result was acknowledged/dismissed. | ||
| public let acknowledgement: () -> Void | ||
|
|
||
| /// Creates the payload. | ||
| public init(acknowledgement: @escaping () -> Void) { | ||
| self.acknowledgement = acknowledgement | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/permissionRequest(_:)``. | ||
| public struct PermissionRequest { | ||
| /// The Sparkle permission request being answered. | ||
| public let request: SPUUpdatePermissionRequest | ||
| /// Replies to Sparkle's permission prompt. | ||
| public let reply: @Sendable (SUUpdatePermissionResponse) -> Void | ||
|
|
||
| /// Creates the payload. | ||
| public init(request: SPUUpdatePermissionRequest, reply: @escaping @Sendable (SUUpdatePermissionResponse) -> Void) { | ||
| self.request = request | ||
| self.reply = reply | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/checking(_:)``. | ||
| public struct Checking { | ||
| /// Cancels the in-progress check. | ||
| public let cancel: () -> Void | ||
|
|
||
| /// Creates the payload. | ||
| public init(cancel: @escaping () -> Void) { | ||
| self.cancel = cancel | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/updateAvailable(_:)``. | ||
| public struct UpdateAvailable { | ||
| /// The appcast item describing the available update. | ||
| public let appcastItem: SUAppcastItem | ||
| /// Replies to Sparkle with the user's install/dismiss choice. | ||
| public let reply: @Sendable (SPUUserUpdateChoice) -> Void | ||
|
|
||
| /// Creates the payload. | ||
| public init(appcastItem: SUAppcastItem, reply: @escaping @Sendable (SPUUserUpdateChoice) -> Void) { | ||
| self.appcastItem = appcastItem | ||
| self.reply = reply | ||
| } | ||
|
|
||
| /// A link to the release notes for this update, derived from its version string. | ||
| public var releaseNotes: ReleaseNotes? { | ||
| ReleaseNotes(displayVersionString: appcastItem.displayVersionString) | ||
| } | ||
| } | ||
|
|
||
| /// A "view release notes" link derived from an update's display version string. | ||
| public enum ReleaseNotes { | ||
| /// The version maps to a git commit; links to the commit page. | ||
| case commit(URL) | ||
| /// The version maps to a semantic-version tag; links to the release page. | ||
| case tagged(URL) | ||
|
|
||
| /// Derives a release-notes link from a display version string, returning `nil` when | ||
| /// the string contains neither a semantic version nor a git hash. | ||
| public init?(displayVersionString: String) { | ||
| let version = displayVersionString | ||
|
|
||
| if let semver = Self.extractSemanticVersion(from: version) { | ||
| let tag = semver.hasPrefix("v") ? semver : "v\(semver)" | ||
| if let url = URL(string: "https://github.com/manaflow-ai/cmux/releases/tag/\(tag)") { | ||
| self = .tagged(url) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| guard let newHash = Self.extractGitHash(from: version) else { | ||
| return nil | ||
| } | ||
|
|
||
| if let url = URL(string: "https://github.com/manaflow-ai/cmux/commit/\(newHash)") { | ||
| self = .commit(url) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| private static func extractSemanticVersion(from version: String) -> String? { | ||
| let pattern = #"v?\d+\.\d+\.\d+"# | ||
| if let range = version.range(of: pattern, options: .regularExpression) { | ||
| return String(version[range]) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| private static func extractGitHash(from version: String) -> String? { | ||
| let pattern = #"[0-9a-f]{7,40}"# | ||
| if let range = version.range(of: pattern, options: .regularExpression) { | ||
| return String(version[range]) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| /// The destination URL of the release-notes link. | ||
| public var url: URL { | ||
| switch self { | ||
| case .commit(let url): return url | ||
| case .tagged(let url): return url | ||
| } | ||
| } | ||
|
|
||
| /// The localized label for the release-notes link. | ||
| public var label: String { | ||
| switch self { | ||
| case .commit: return String(localized: "update.viewGitHubCommit", defaultValue: "View GitHub Commit") | ||
| case .tagged: return String(localized: "update.viewReleaseNotes", defaultValue: "View Release Notes") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/error(_:)``. | ||
| public struct Error { | ||
| /// The underlying error. | ||
| public let error: any Swift.Error | ||
| /// Retries the failed operation. | ||
| public let retry: () -> Void | ||
| /// Dismisses the error. | ||
| public let dismiss: () -> Void | ||
| /// Extra technical detail captured at failure time, surfaced in the error popover. | ||
| public let technicalDetails: String? | ||
| /// The feed URL in effect when the error occurred, surfaced in the error popover. | ||
| public let feedURLString: String? | ||
|
|
||
| /// Creates the payload. | ||
| public init(error: any Swift.Error, | ||
| retry: @escaping () -> Void, | ||
| dismiss: @escaping () -> Void, | ||
| technicalDetails: String? = nil, | ||
| feedURLString: String? = nil) { | ||
| self.error = error | ||
| self.retry = retry | ||
| self.dismiss = dismiss | ||
| self.technicalDetails = technicalDetails | ||
| self.feedURLString = feedURLString | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/downloading(_:)``. | ||
| public struct Downloading { | ||
| /// Cancels the download. | ||
| public let cancel: () -> Void | ||
| /// Total expected byte count, when known. | ||
| public let expectedLength: UInt64? | ||
| /// Bytes received so far. | ||
| public let progress: UInt64 | ||
|
|
||
| /// Creates the payload. | ||
| public init(cancel: @escaping () -> Void, expectedLength: UInt64?, progress: UInt64) { | ||
| self.cancel = cancel | ||
| self.expectedLength = expectedLength | ||
| self.progress = progress | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/extracting(_:)``. | ||
| public struct Extracting { | ||
| /// Extraction progress in `0...1`. | ||
| public let progress: Double | ||
|
|
||
| /// Creates the payload. | ||
| public init(progress: Double) { | ||
| self.progress = progress | ||
| } | ||
| } | ||
|
|
||
| /// Payload for ``UpdateState/installing(_:)``. | ||
| public struct Installing { | ||
| /// Whether this install was triggered by Sparkle's automatic "install on quit" path | ||
| /// rather than an explicit user action. | ||
| public var isAutoUpdate = false | ||
| /// Retries terminating the app so the install can finish. | ||
| public let retryTerminatingApplication: () -> Void | ||
| /// Dismisses the installing state. | ||
| public let dismiss: () -> Void | ||
|
|
||
| /// Creates the payload. | ||
| public init(isAutoUpdate: Bool = false, | ||
| retryTerminatingApplication: @escaping () -> Void, | ||
| dismiss: @escaping () -> Void) { | ||
| self.isAutoUpdate = isAutoUpdate | ||
| self.retryTerminatingApplication = retryTerminatingApplication | ||
| self.dismiss = dismiss | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Consider splitting nested public payload types into separate files.
UpdateState.swift contains multiple public nested types (NotFound, PermissionRequest, Checking, UpdateAvailable, ReleaseNotes, Error, Downloading, Extracting, Installing). As per coding guidelines, "One major type per file; each struct, class, enum, actor, or protocol that is part of a public API lives in its own file named after the type."
However, since these are tightly coupled payload types that exist solely to serve UpdateState cases and are accessed via the enum (e.g., UpdateState.UpdateAvailable), keeping them nested may be more maintainable than 9 separate files. This is a judgment call between strict guideline adherence and pragmatic cohesion.
🤖 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 `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateState.swift` around lines 106
- 298, The file exposes many public nested payload types (NotFound,
PermissionRequest, Checking, UpdateAvailable, ReleaseNotes, Error, Downloading,
Extracting, Installing) under UpdateState which violates the "one major type per
file" guideline; either split each public type into its own file named after the
type (e.g., UpdateAvailable.swift containing public struct UpdateAvailable) and
keep their public API, or if they are truly internal implementation details of
UpdateState, reduce their visibility to internal/private and keep them nested;
update access levels and imports accordingly and ensure UpdateState references
(UpdateState.UpdateAvailable, etc.) still compile after the change.
There was a problem hiding this comment.
Keeping nested (judgment call you noted). These payloads exist solely to carry each UpdateState case's data and are always referenced through the enum (UpdateState.UpdateAvailable, etc.); the file-org rule explicitly allows small, closely-bound nested types to stay with the parent. Splitting into 9 files would scatter tightly-coupled state with no discoverability gain.
— Claude Code
There was a problem hiding this comment.
@azooz2003-bit That's a sound call. Tightly-coupled enum-case payloads that are only ever reachable through UpdateState.X are exactly the scenario where the cohesion argument wins over the per-file rule — the enum is effectively the namespace and the file boundary. Happy to leave it as-is.
🧠 Learnings used
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: pgbezerra
Repo: manaflow-ai/cmux PR: 3307
File: Sources/cmuxApp.swift:6413-6417
Timestamp: 2026-04-30T11:55:31.575Z
Learning: In this repo (manaflow-ai/cmux), when adding a new Settings section in SwiftUI (e.g., in Sources/cmuxApp.swift or related Views), don’t wire navigation/search with a raw anchor string alone. Instead: (1) create a corresponding SettingsNavigationTarget enum case (e.g., .workspaces); (2) provide the localized title, symbol, search text, and aliases for that case; (3) add/update the matching entry in SettingsSearchIndex so the sidebar/search can navigate to it; and (4) apply .settingsSearchAnchor(SettingsSearchIndex.sectionID(for: <target>)) to the section header. This prevents broken jump-to behavior by ensuring the navigation anchor and the search index stay consistent.
Learnt from: ryouka0731
Repo: manaflow-ai/cmux PR: 3415
File: Sources/Worktree/WorktreeManager.swift:1-10
Timestamp: 2026-05-06T19:24:46.399Z
Learning: Do not recommend extracting Foundation-only Swift files into a new SwiftPM package target in this repo. The established pattern is to keep app-internal helper modules under Sources/ unless the repository introduces a dedicated package (e.g., CMUXAuthCore, CMUXDebugLog, CMUXWorkstream) with a clear architectural rationale. Speculative SwiftPM extractions for individual Foundation-only modules expand PR scope without documented justification. Apply this guideline when reviewing changes to Swift files under Sources/; require a documented package architecture justification before considering packaging changes.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 3626
File: Sources/TabManager.swift:1817-1818
Timestamp: 2026-05-07T08:37:03.967Z
Learning: In this Swift repo (manafow-ai/cmux), when cleaning up stale agent process entries, use `Workspace.clearAgentPID(key:panelId:)` as the single cleanup path. Do not directly mutate `Workspace.statusEntries` or `Workspace.agentPIDs` from outside the dedicated helpers; for example, `TabManager.sweepStaleAgentPIDs` should only call `clearAgentPID` rather than performing its own mutations. This ensures panel-scoped side effects and port/refresh logic run consistently.
Learnt from: psh4607
Repo: manaflow-ai/cmux PR: 3696
File: cmuxTests/ShortcutAndCommandPaletteTests.swift:1716-1771
Timestamp: 2026-05-07T10:56:50.266Z
Learning: In the manaflow-ai/cmux repo, SwiftLint does not enforce a `required_deinit` rule (no project `.swiftlint.yml` in cmux itself, no `required_deinit` in `.github/review-bot-rules/`, and no SwiftLint CI run in `.github/workflows/`). During code reviews, do not raise findings for missing `deinit` on `XCTestCase` subclasses or other Swift classes based on a `required_deinit` rule.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3784
File: README.md:161-161
Timestamp: 2026-05-09T04:48:35.413Z
Learning: In the cmux project, the right-sidebar keyboard shortcut labels were intentionally swapped (per PR `#3784`). Reviewers should NOT flag the ⌘⇧E (Cmd+Shift+E) label as “Open file explorer.” Use these mappings consistently: ⌘⇧E → `focusRightSidebar` with the user-facing label “Toggle right sidebar focus”; ⌘⌥B (Cmd+Option+B) → `toggleFileExplorer` with the user-facing label “Open file explorer.”
…ge emission - userFacingErrorMessage catch-all no longer returns raw nsError.localizedDescription (which can leak vendor strings like 'org.sparkle-project.Sparkle error 4006'); returns a cmux-phrased message instead. Raw details stay in errorDetails + the log. - cancelActiveStateForNewCheck updates state + overrideState then emits notifyStateChanged() once, instead of two redundant stateChanges() emissions for one logical reset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/UpdatePillUITests.swift">
<violation number="1" location="cmuxUITests/UpdatePillUITests.swift:252">
P3: Inconsistent popover-button assertion pattern after `pill.click()`: the second button uses `.exists` instead of `waitForExistence`, differing from the existing test convention in this file. Use `waitForExistence` for both popover buttons to stay consistent and avoid a potential source of flakiness under heavy CI load.</violation>
<violation number="2" location="cmuxUITests/UpdatePillUITests.swift:282">
P3: Inconsistent popover-button assertion pattern: `Copy Details` uses `.exists` while `Retry` uses `waitForExistence`. Use `waitForExistence` for both for consistency and to reduce flakiness risk.</violation>
</file>
<file name="Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift">
<violation number="1" location="Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift:94">
P2: `recheckTask?.cancel()` is only called in the non-idle branch; the idle-path early return leaves a prior `recheckTask` alive, potentially causing a duplicate `checkForUpdates()` call when the task fires.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Add - Parameter callouts for scheduledCheckInterval / previousDefaultScheduledCheckInterval per the package public-API doc rule. (Other CodeRabbit findings replied to inline: the cmux-unit wiring is host-resolved like Sparkle/Bonsplit and the tests job passes cmux-only; canCheckForUpdates can't use Combine/KVO under the rules so it's a bounded clock retry; the nested UpdateState payloads stay per the closely-bound-types allowance; safeHelp is internal.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Re: CodeRabbit's outside-diff note on |
There was a problem hiding this comment.
6 issues found
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:13167">
P1: Passing cmuxAccentColor() to UpdatePill(accent:) risks NSException crash if UpdatePill extracts NSColor components without first converting to sRGB color space. Per repository convention, any getRed/getGreen/getBlue call on an NSColor must go through usingColorSpace(.sRGB) with a safe fallback.</violation>
</file>
<file name="Sources/Update/UpdateLogStore.swift">
<violation number="1" location="Sources/Update/UpdateLogStore.swift:15">
P2: Public `init()` lacks injectable `FileManager` parameter, forcing tests to touch the real filesystem</violation>
</file>
<file name="Packages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View+SafeHelp.swift">
<violation number="1" location="Packages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View+SafeHelp.swift:6">
P3: `safeHelp` only accepts `String`, missing a `LocalizedStringKey` overload that would enable automatic localization of help tooltips via SwiftUI's string-table lookup.</violation>
</file>
<file name="Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift">
<violation number="1" location="Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift:368">
P2: `ensureSparkleInstallationCache` uses `FileManager.default` directly instead of accepting an injected `FileManager` instance, reducing testability and deviating from the established injection pattern (`log`, `clock`, `defaults` are already injected). The feedback guidance for this file explicitly calls this out — use injection for consistency and to allow test code to control the filesystem.</violation>
</file>
<file name="Packages/CmuxUpdater/Package.swift">
<violation number="1" location="Packages/CmuxUpdater/Package.swift:17">
P2: Sparkle version constraint `from: "2.5.1"` contradicts PR description's stated intent to pin Sparkle to 2.8.1. The loose range allows SPM to resolve to versions below 2.8.1 that lack the `@preconcurrency` compatibility fixes the PR claims were added, which could cause build failures on fresh checkouts or CI environments without a Package.resolved.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:14965">
P2: `validateMenuItem` returns `true` unconditionally, removing the previous delegation to `UpdateController.validateMenuItem`. This could re-enable update menu items during states where they should be disabled (e.g., allowing "Check for Updates" while already checking).</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| } | ||
| UpdatePill(model: updateViewModel) | ||
| if let updateActionsHost = AppDelegate.shared { | ||
| UpdatePill(model: updateViewModel, accent: cmuxAccentColor(), actions: updateActionsHost) |
There was a problem hiding this comment.
P1: Passing cmuxAccentColor() to UpdatePill(accent:) risks NSException crash if UpdatePill extracts NSColor components without first converting to sRGB color space. Per repository convention, any getRed/getGreen/getBlue call on an NSColor must go through usingColorSpace(.sRGB) with a safe fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 13167:
<comment>Passing cmuxAccentColor() to UpdatePill(accent:) risks NSException crash if UpdatePill extracts NSColor components without first converting to sRGB color space. Per repository convention, any getRed/getGreen/getBlue call on an NSColor must go through usingColorSpace(.sRGB) with a safe fallback.</comment>
<file context>
@@ -13161,7 +13163,9 @@ private struct SidebarFooterButtons: View {
}
- UpdatePill(model: updateViewModel)
+ if let updateActionsHost = AppDelegate.shared {
+ UpdatePill(model: updateViewModel, accent: cmuxAccentColor(), actions: updateActionsHost)
+ }
}
</file context>
There was a problem hiding this comment.
Not applicable. UpdatePill/UpdateAppearance never extract NSColor components from the accent - the accent Color is used directly as a SwiftUI fill/foreground, and the only NSColor work is NSColor.systemBlue.blended(withFraction:of:). There's no getRed/getGreen/getBlue (or usingColorSpace) call on the passed accent, so there's no sRGB-conversion crash path here.
— Claude Code
| private let formatter: ISO8601DateFormatter | ||
|
|
||
| private init() { | ||
| init() { |
There was a problem hiding this comment.
P2: Public init() lacks injectable FileManager parameter, forcing tests to touch the real filesystem
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Update/UpdateLogStore.swift, line 15:
<comment>Public `init()` lacks injectable `FileManager` parameter, forcing tests to touch the real filesystem</comment>
<file context>
@@ -1,16 +1,18 @@
private let formatter: ISO8601DateFormatter
- private init() {
+ init() {
formatter = ISO8601DateFormatter()
formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
</file context>
There was a problem hiding this comment.
Out of scope here. UpdateLogStore is app-target infrastructure (not a Packages/ type), and it's a general file logger (its sibling FocusLogStore logs focus events). The inject-FileManager testability rule targets package public types. This extraction de-singletoned it (composition-root owned, injected as any UpdateLogging); a FileManager seam on the app logger is a reasonable separate follow-up, not part of the updater extraction.
— Claude Code
| ), | ||
| ], | ||
| dependencies: [ | ||
| .package(url: "https://github.com/sparkle-project/Sparkle", from: "2.5.1"), |
There was a problem hiding this comment.
P2: Sparkle version constraint from: "2.5.1" contradicts PR description's stated intent to pin Sparkle to 2.8.1. The loose range allows SPM to resolve to versions below 2.8.1 that lack the @preconcurrency compatibility fixes the PR claims were added, which could cause build failures on fresh checkouts or CI environments without a Package.resolved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxUpdater/Package.swift, line 17:
<comment>Sparkle version constraint `from: "2.5.1"` contradicts PR description's stated intent to pin Sparkle to 2.8.1. The loose range allows SPM to resolve to versions below 2.8.1 that lack the `@preconcurrency` compatibility fixes the PR claims were added, which could cause build failures on fresh checkouts or CI environments without a Package.resolved.</comment>
<file context>
@@ -0,0 +1,41 @@
+ ),
+ ],
+ dependencies: [
+ .package(url: "https://github.com/sparkle-project/Sparkle", from: "2.5.1"),
+ ],
+ targets: [
</file context>
There was a problem hiding this comment.
Mitigated. The @preconcurrency Sparkle import makes the code compile against any 2.x (2.8.1 or 2.9.x), so a fresh-checkout resolution difference won't break the build; the committed Package.resolved pins 2.8.1 to match nightly; and keeping the requirement at 'from: 2.5.1' matches the app target's own Sparkle requirement so the combined Xcode resolution unifies cleanly without this leaf package imposing a tighter cap on the whole app.
— Claude Code
There was a problem hiding this comment.
Mitigated. The @preconcurrency Sparkle import lets the code compile against any 2.x (2.8.1 or 2.9.x), so a fresh-checkout resolution difference won't break the build; the committed Package.resolved pins 2.8.1 (matching nightly); and 'from: 2.5.1' matches the app target's own Sparkle requirement so the combined Xcode resolution unifies without this leaf package capping the whole app.
— Claude Code
| updateController.validateMenuItem(item) | ||
| // User-initiated update checks are always allowed; other items are unconditionally valid | ||
| // (this preserves the prior UpdateController.validateMenuItem behavior). | ||
| true |
There was a problem hiding this comment.
P2: validateMenuItem returns true unconditionally, removing the previous delegation to UpdateController.validateMenuItem. This could re-enable update menu items during states where they should be disabled (e.g., allowing "Check for Updates" while already checking).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 14965:
<comment>`validateMenuItem` returns `true` unconditionally, removing the previous delegation to `UpdateController.validateMenuItem`. This could re-enable update menu items during states where they should be disabled (e.g., allowing "Check for Updates" while already checking).</comment>
<file context>
@@ -14956,7 +14960,9 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
- updateController.validateMenuItem(item)
+ // User-initiated update checks are always allowed; other items are unconditionally valid
+ // (this preserves the prior UpdateController.validateMenuItem behavior).
+ true
}
</file context>
There was a problem hiding this comment.
Behavior-preserving: the original UpdateController.validateMenuItem returned true for all items (both branches were 'return true'). Nothing was disabled before, so 'return true' is identical.
— Claude Code
| extension View { | ||
| /// Applies a help tooltip, skipping it when `text` is empty so an empty tooltip never shows. | ||
| @ViewBuilder | ||
| func safeHelp(_ text: String) -> some View { |
There was a problem hiding this comment.
P3: safeHelp only accepts String, missing a LocalizedStringKey overload that would enable automatic localization of help tooltips via SwiftUI's string-table lookup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View+SafeHelp.swift, line 6:
<comment>`safeHelp` only accepts `String`, missing a `LocalizedStringKey` overload that would enable automatic localization of help tooltips via SwiftUI's string-table lookup.</comment>
<file context>
@@ -0,0 +1,13 @@
+extension View {
+ /// Applies a help tooltip, skipping it when `text` is empty so an empty tooltip never shows.
+ @ViewBuilder
+ func safeHelp(_ text: String) -> some View {
+ if text.isEmpty {
+ self
</file context>
There was a problem hiding this comment.
Not needed. safeHelp is an internal package helper invoked only with already-resolved runtime strings (model.text), never a literal key, so a LocalizedStringKey overload would have no caller and add unused API surface.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux.xcodeproj/project.pbxproj (1)
2055-2079: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winNormalize the pbxproj before merge.
These high-churn package sections were edited directly. Please run the repo’s pbxproj normalization step before landing this so we don’t commit nondeterministic Xcode reordering alongside the real wiring changes.
Based on learnings,
scripts/normalize-pbxproj.pymust be run on any stagedcmux.xcodeproj/project.pbxprojto sort high-churn sections and prevent nondeterministic Xcode reordering from reaching commits.Also applies to: 3172-3244, 3291-3375
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 2055 - 2079, The pbxproj contains nondeterministic reordering in the packageReferences section; run the repository’s pbxproj normalization script (normalize-pbxproj.py) on the staged project file to sort high‑churn sections (notably the packageReferences array), then re-stage the normalized file and amend the commit (or create a new commit) so only the intended wiring changes remain; ensure you target the project’s packageReferences entries (the list shown in the diff) before pushing.
🤖 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 `@cmux.xcodeproj/project.pbxproj`:
- Around line 2814-2817: Revert the accidental version bumps by removing the
changes to MARKETING_VERSION and CURRENT_PROJECT_VERSION introduced in this PR:
restore the previous values for all occurrences where these keys were modified
(e.g., the blocks containing MARKETING_VERSION and CURRENT_PROJECT_VERSION
around the shown diff and the other occurrences referenced), leaving
GENERATE_INFOPLIST_FILE and MACOSX_DEPLOYMENT_TARGET changes untouched; ensure
no version changes remain in project.pbxproj so only the intended
updater/package extraction changes are in this branch and leave version updates
to the release workflow (./scripts/bump-version.sh).
In `@Sources/AppDelegate.swift`:
- Around line 5953-5969: The code currently creates a ProcessOutputCollector
that buffers the child process stdout/stderr (outputCollector.start() /
outputCollector.finish()), which can retain arbitrarily large diffs; change the
launch code so the child’s standardOutput/standardError are not fully buffered:
either set process.standardOutput and process.standardError to
FileHandle.nullDevice (or Pipe() that forwards to .nullDevice) when only a byte
count is needed, or modify ProcessOutputCollector to incrementally count bytes
(expose a byteCount property and avoid storing full Data/String) and call that
instead of finish()—apply the same change for the second occurrence referenced
in the comment.
In `@Sources/ContentView.swift`:
- Around line 11148-11155: Precompute a lookup dictionary from workspace id to
workspace once (e.g., let workspaceById: [UUID: Workspace] =
Dictionary(uniqueKeysWithValues: tabManager.tabs.map { ($0.id, $0) })) and use
it instead of calling tabManager.tabs.first(where:) inside cmuxSidebarSurfaces
and the sibling helper that builds grid/row surfaces; replace the repeated
liveWorkspace lookup with workspaceById[workspace.id], then early-return [] if
nil and continue using liveWorkspace.sidebarOrderedPanelIds(),
liveWorkspace.panels[panelId], and liveWorkspace.panelTitle(panelId:) as before
to eliminate the O(n²) nested scans.
- Around line 11052-11057: Replace the Combine NotificationCenter publisher
usage with Swift concurrency: stop using
NotificationCenter.default.publisher(for:
BrowserStackSidebar.stateDidLoadNotification) inside .onReceive and instead
consume NotificationCenter.default.notifications(named:) as an async sequence
(e.g., spawn a Task in the SwiftUI view lifecycle such as .task or .onAppear)
and call refreshExtensionSidebarSnapshot() on the main actor when you receive
the notification; ensure you reference
BrowserStackSidebar.stateDidLoadNotification and invoke the existing
refreshExtensionSidebarSnapshot function, and cancel the Task appropriately when
the view disappears to avoid leaks.
---
Outside diff comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 2055-2079: The pbxproj contains nondeterministic reordering in the
packageReferences section; run the repository’s pbxproj normalization script
(normalize-pbxproj.py) on the staged project file to sort high‑churn sections
(notably the packageReferences array), then re-stage the normalized file and
amend the commit (or create a new commit) so only the intended wiring changes
remain; ensure you target the project’s packageReferences entries (the list
shown in the diff) before pushing.
🪄 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: 018425a7-2cce-4383-9aef-5e3272bdd66c
📒 Files selected for processing (7)
CLAUDE.mdPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateSettings.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateStateModel.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux.xcodeproj/project.pbxproj (1)
2055-2079: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winNormalize the pbxproj before merge.
These high-churn package sections were edited directly. Please run the repo’s pbxproj normalization step before landing this so we don’t commit nondeterministic Xcode reordering alongside the real wiring changes.
Based on learnings,
scripts/normalize-pbxproj.pymust be run on any stagedcmux.xcodeproj/project.pbxprojto sort high-churn sections and prevent nondeterministic Xcode reordering from reaching commits.Also applies to: 3172-3244, 3291-3375
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 2055 - 2079, The pbxproj contains nondeterministic reordering in the packageReferences section; run the repository’s pbxproj normalization script (normalize-pbxproj.py) on the staged project file to sort high‑churn sections (notably the packageReferences array), then re-stage the normalized file and amend the commit (or create a new commit) so only the intended wiring changes remain; ensure you target the project’s packageReferences entries (the list shown in the diff) before pushing.
🤖 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 `@cmux.xcodeproj/project.pbxproj`:
- Around line 2814-2817: Revert the accidental version bumps by removing the
changes to MARKETING_VERSION and CURRENT_PROJECT_VERSION introduced in this PR:
restore the previous values for all occurrences where these keys were modified
(e.g., the blocks containing MARKETING_VERSION and CURRENT_PROJECT_VERSION
around the shown diff and the other occurrences referenced), leaving
GENERATE_INFOPLIST_FILE and MACOSX_DEPLOYMENT_TARGET changes untouched; ensure
no version changes remain in project.pbxproj so only the intended
updater/package extraction changes are in this branch and leave version updates
to the release workflow (./scripts/bump-version.sh).
In `@Sources/AppDelegate.swift`:
- Around line 5953-5969: The code currently creates a ProcessOutputCollector
that buffers the child process stdout/stderr (outputCollector.start() /
outputCollector.finish()), which can retain arbitrarily large diffs; change the
launch code so the child’s standardOutput/standardError are not fully buffered:
either set process.standardOutput and process.standardError to
FileHandle.nullDevice (or Pipe() that forwards to .nullDevice) when only a byte
count is needed, or modify ProcessOutputCollector to incrementally count bytes
(expose a byteCount property and avoid storing full Data/String) and call that
instead of finish()—apply the same change for the second occurrence referenced
in the comment.
In `@Sources/ContentView.swift`:
- Around line 11148-11155: Precompute a lookup dictionary from workspace id to
workspace once (e.g., let workspaceById: [UUID: Workspace] =
Dictionary(uniqueKeysWithValues: tabManager.tabs.map { ($0.id, $0) })) and use
it instead of calling tabManager.tabs.first(where:) inside cmuxSidebarSurfaces
and the sibling helper that builds grid/row surfaces; replace the repeated
liveWorkspace lookup with workspaceById[workspace.id], then early-return [] if
nil and continue using liveWorkspace.sidebarOrderedPanelIds(),
liveWorkspace.panels[panelId], and liveWorkspace.panelTitle(panelId:) as before
to eliminate the O(n²) nested scans.
- Around line 11052-11057: Replace the Combine NotificationCenter publisher
usage with Swift concurrency: stop using
NotificationCenter.default.publisher(for:
BrowserStackSidebar.stateDidLoadNotification) inside .onReceive and instead
consume NotificationCenter.default.notifications(named:) as an async sequence
(e.g., spawn a Task in the SwiftUI view lifecycle such as .task or .onAppear)
and call refreshExtensionSidebarSnapshot() on the main actor when you receive
the notification; ensure you reference
BrowserStackSidebar.stateDidLoadNotification and invoke the existing
refreshExtensionSidebarSnapshot function, and cancel the Task appropriately when
the view disappears to avoid leaks.
---
Outside diff comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 2055-2079: The pbxproj contains nondeterministic reordering in the
packageReferences section; run the repository’s pbxproj normalization script
(normalize-pbxproj.py) on the staged project file to sort high‑churn sections
(notably the packageReferences array), then re-stage the normalized file and
amend the commit (or create a new commit) so only the intended wiring changes
remain; ensure you target the project’s packageReferences entries (the list
shown in the diff) before pushing.
🪄 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: 018425a7-2cce-4383-9aef-5e3272bdd66c
📒 Files selected for processing (7)
CLAUDE.mdPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateSettings.swiftPackages/CmuxUpdater/Sources/CmuxUpdater/UpdateStateModel.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxproj
🛑 Comments failed to post (4)
cmux.xcodeproj/project.pbxproj (1)
2814-2817: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Drop the unrelated version bump from this feature PR.
MARKETING_VERSIONandCURRENT_PROJECT_VERSIONchanged for the app, tests, and plugin even though this draft is about updater/package extraction. That adds avoidable merge churn and release noise; keep version bumps in the dedicated release flow unless this branch is actually cutting0.64.12.Based on learnings,
/releaseprepares releases and./scripts/bump-version.shis the intended path for updatingMARKETING_VERSIONandCURRENT_PROJECT_VERSION.Also applies to: 2897-2906, 2938-2947, 3014-3017, 3032-3041, 3057-3066, 3081-3084, 3100-3103
🤖 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 `@cmux.xcodeproj/project.pbxproj` around lines 2814 - 2817, Revert the accidental version bumps by removing the changes to MARKETING_VERSION and CURRENT_PROJECT_VERSION introduced in this PR: restore the previous values for all occurrences where these keys were modified (e.g., the blocks containing MARKETING_VERSION and CURRENT_PROJECT_VERSION around the shown diff and the other occurrences referenced), leaving GENERATE_INFOPLIST_FILE and MACOSX_DEPLOYMENT_TARGET changes untouched; ensure no version changes remain in project.pbxproj so only the intended updater/package extraction changes are in this branch and leave version updates to the release workflow (./scripts/bump-version.sh).Sources/AppDelegate.swift (1)
5953-5969:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid buffering the diff subprocess output.
This path launches
cmux diff --unstaged, then stores the full stdout/stderr inProcessOutputCollectoreven though the handler only uses a DEBUG byte count. That makes a user-triggered shortcut retain arbitrarily large patch text and file contents in memory until the child exits. Route these streams tonullDevice, or switch the collector to incremental byte counting so the payload is never buffered.Possible fix
- let stdoutPipe = Pipe() - let stderrPipe = Pipe() - process.standardOutput = stdoutPipe - process.standardError = stderrPipe - let outputCollector = ProcessOutputCollector(stdout: stdoutPipe, stderr: stderrPipe) - outputCollector.start() + process.standardOutput = FileHandle.nullDevice + process.standardError = FileHandle.nullDevice process.terminationHandler = { terminatedProcess in - let output = outputCollector.finish() let processIdentifier = terminatedProcess.processIdentifier let terminationStatus = terminatedProcess.terminationStatus Task { `@MainActor` in AppDelegate.shared?.diffViewerProcesses.removeValue(forKey: processIdentifier) guard terminationStatus != 0 else { return } `#if` DEBUG - cmuxDebugLog("openDiffViewer exited status=\(terminationStatus) outputBytes=\(output.utf8.count)") + cmuxDebugLog("openDiffViewer exited status=\(terminationStatus)") `#endif` NSSound.beep() } } ... } catch { - outputCollector.cancel() `#if` DEBUG cmuxDebugLog("openDiffViewer failed errorType=\(type(of: error))") `#endif` return false }Also applies to: 5987-5989
🤖 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/AppDelegate.swift` around lines 5953 - 5969, The code currently creates a ProcessOutputCollector that buffers the child process stdout/stderr (outputCollector.start() / outputCollector.finish()), which can retain arbitrarily large diffs; change the launch code so the child’s standardOutput/standardError are not fully buffered: either set process.standardOutput and process.standardError to FileHandle.nullDevice (or Pipe() that forwards to .nullDevice) when only a byte count is needed, or modify ProcessOutputCollector to incrementally count bytes (expose a byteCount property and avoid storing full Data/String) and call that instead of finish()—apply the same change for the second occurrence referenced in the comment.Sources/ContentView.swift (2)
11052-11057:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Inspect the exact code around the referenced lines in Sources/ContentView.swift sed -n '11030,11090p' Sources/ContentView.swift | cat -n # 2) Find any occurrences of the specific notification + Combine publisher usage rg -n "BrowserStackSidebar\.stateDidLoadNotification" Sources/ContentView.swift Sources -S || true rg -n "NotificationCenter\.default\.publisher\(for:" Sources -S || true rg -n "NotificationCenter\.default\.notifications\(named:" Sources -S || true # 3) Check repository lint/rules for NotificationCenter/Combine guidance ls -la .github/review-bot-rules || true find .github/review-bot-rules -maxdepth 2 -type f -name "*.md" -print || true rg -n "NotificationCenter\.default\.notifications|notifications\\(named\\)|AsyncSequence|Combine|\\.onReceive" .github/review-bot-rules -S || trueRepository: manaflow-ai/cmux
Length of output: 13466
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant lint rule(s) sed -n '1,200p' .github/review-bot-rules/swift-concurrency-modernization.md | cat -n # Search for explicit NotificationCenter async sequence guidance in all rule files rg -n "NotificationCenter\.default\.notifications|notifications\\(named\\)" .github/review-bot-rules -S || true rg -n "Combine usage|New Combine usage|publisher\\(for:" .github/review-bot-rules -S || true # Check whether the repo already uses NotificationCenter.default.notifications(named:) anywhere rg -n "NotificationCenter\\.default\\.notifications\\(named:" Sources -S || true rg -n "\\.notifications\\(named:" Sources -S || trueRepository: manaflow-ai/cmux
Length of output: 2070
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Look for any rule text that mandates NotificationCenter AsyncSequence usage rg -n "NotificationCenter\.default\.notifications|notifications\\(named:" .github/review-bot-rules -S || true rg -n "publisher\\(for:.*NotificationCenter|publisher\\(for:|\\.onReceive\\(" .github/review-bot-rules -S || true rg -n "AsyncSequence.*NotificationCenter|NotificationCenter.*AsyncSequence" .github/review-bot-rules -S || true # 2) See if the repo already uses NotificationCenter.default.notifications(named:) anywhere rg -n "NotificationCenter\\.default\\.notifications\\(named:" Sources -S || true rg -n "\\.notifications\\(named:" Sources -S || trueRepository: manaflow-ai/cmux
Length of output: 42
Avoid introducing a new Combine
NotificationCenter.publisherin SwiftUI; switch to an asyncNotificationCenter.default.notifications(named:)flow
swift-concurrency-modernization.mdflags new Combine usage (publishers/subscribers) for app state/async flow when async/await is available. This adds aNotificationCenter.default.publisher(for: BrowserStackSidebar.stateDidLoadNotification)pipeline inSources/ContentView.swift..onReceive( NotificationCenter.default.publisher(for: BrowserStackSidebar.stateDidLoadNotification) .receive(on: RunLoop.main) ) { _ in refreshExtensionSidebarSnapshot() }🤖 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/ContentView.swift` around lines 11052 - 11057, Replace the Combine NotificationCenter publisher usage with Swift concurrency: stop using NotificationCenter.default.publisher(for: BrowserStackSidebar.stateDidLoadNotification) inside .onReceive and instead consume NotificationCenter.default.notifications(named:) as an async sequence (e.g., spawn a Task in the SwiftUI view lifecycle such as .task or .onAppear) and call refreshExtensionSidebarSnapshot() on the main actor when you receive the notification; ensure you reference BrowserStackSidebar.stateDidLoadNotification and invoke the existing refreshExtensionSidebarSnapshot function, and cancel the Task appropriately when the view disappears to avoid leaks.
11148-11155:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winIndex workspaces once before building these sidebar models.
Line 11149 and Line 11902 both do
tabManager.tabs.first(where:)lookups inside loops over already-collected workspaces/rows, so this render path becomes O(n²) as workspace count grows. Precompute a[UUID: Workspace]map once and reuse it in both helpers.As per coding guidelines,
**/*.{swift,ts,tsx,js,jsx,sh}production paths must fail nested full-collection scans over scalable user data.Also applies to: 11762-11768, 11901-11903
🤖 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/ContentView.swift` around lines 11148 - 11155, Precompute a lookup dictionary from workspace id to workspace once (e.g., let workspaceById: [UUID: Workspace] = Dictionary(uniqueKeysWithValues: tabManager.tabs.map { ($0.id, $0) })) and use it instead of calling tabManager.tabs.first(where:) inside cmuxSidebarSurfaces and the sibling helper that builds grid/row surfaces; replace the repeated liveWorkspace lookup with workspaceById[workspace.id], then early-return [] if nil and continue using liveWorkspace.sidebarOrderedPanelIds(), liveWorkspace.panels[panelId], and liveWorkspace.panelTitle(panelId:) as before to eliminate the O(n²) nested scans.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:13167">
P1: Passing cmuxAccentColor() to UpdatePill(accent:) risks NSException crash if UpdatePill extracts NSColor components without first converting to sRGB color space. Per repository convention, any getRed/getGreen/getBlue call on an NSColor must go through usingColorSpace(.sRGB) with a safe fallback.</violation>
</file>
<file name="Sources/Update/UpdateLogStore.swift">
<violation number="1" location="Sources/Update/UpdateLogStore.swift:15">
P2: Public `init()` lacks injectable `FileManager` parameter, forcing tests to touch the real filesystem</violation>
</file>
<file name="Packages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View+SafeHelp.swift">
<violation number="1" location="Packages/CmuxUpdaterUI/Sources/CmuxUpdaterUI/View+SafeHelp.swift:6">
P3: `safeHelp` only accepts `String`, missing a `LocalizedStringKey` overload that would enable automatic localization of help tooltips via SwiftUI's string-table lookup.</violation>
</file>
<file name="Packages/CmuxUpdater/Package.swift">
<violation number="1" location="Packages/CmuxUpdater/Package.swift:17">
P2: Sparkle version constraint `from: "2.5.1"` contradicts PR description's stated intent to pin Sparkle to 2.8.1. The loose range allows SPM to resolve to versions below 2.8.1 that lack the `@preconcurrency` compatibility fixes the PR claims were added, which could cause build failures on fresh checkouts or CI environments without a Package.resolved.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:14965">
P2: `validateMenuItem` returns `true` unconditionally, removing the previous delegation to `UpdateController.validateMenuItem`. This could re-enable update menu items during states where they should be disabled (e.g., allowing "Check for Updates" while already checking).</violation>
</file>
<file name="Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateStateModel.swift">
<violation number="1" location="Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateStateModel.swift:396">
P2: New user-facing localization key is used in code but missing from `Resources/Localizable.xcstrings` (`en`/`ja` translations are not defined).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| static func errorDetails(for error: Swift.Error, technicalDetails: String?, feedURLString: String?) -> String { | ||
| // Catch-all: keep user-facing copy in cmux terms; raw vendor descriptions, domains, and | ||
| // codes stay in `errorDetails` (the copyable Details block + the update log), not here. | ||
| return String(localized: "update.error.failed.message", defaultValue: "Something went wrong while checking for updates. Try again, or check the update log for details.") |
There was a problem hiding this comment.
P2: New user-facing localization key is used in code but missing from Resources/Localizable.xcstrings (en/ja translations are not defined).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateStateModel.swift, line 396:
<comment>New user-facing localization key is used in code but missing from `Resources/Localizable.xcstrings` (`en`/`ja` translations are not defined).</comment>
<file context>
@@ -388,7 +391,9 @@ public final class UpdateStateModel {
- return nsError.localizedDescription
+ // Catch-all: keep user-facing copy in cmux terms; raw vendor descriptions, domains, and
+ // codes stay in `errorDetails` (the copyable Details block + the update log), not here.
+ return String(localized: "update.error.failed.message", defaultValue: "Something went wrong while checking for updates. Try again, or check the update log for details.")
}
</file context>
There was a problem hiding this comment.
Fixed - added update.error.failed.message to Resources/Localizable.xcstrings with en + ja.
— Claude Code
…test assertion consistency - performCheckForUpdates cancels recheckTask on every path (incl. the idle early-return), so a stale deferred re-check can't fire a duplicate checkForUpdates(). (cubic P2) - Add update.error.failed.message to Resources/Localizable.xcstrings (en + ja) for the new sanitized error-message fallback. (cubic P2) - UpdatePillUITests: both popover-button assertions use waitForExistence (was mixing .exists), for consistency + less CI flakiness. (cubic P3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ensureSparkleInstallationCache now uses an injected FileManager (default .default), matching the existing log/clock/defaults injection and the package testability rule (no hardcoded FileManager.default in package public types). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift (2)
278-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded English error message violates localization requirements.
The error description is hardcoded as
"Updater is still starting. Try again in a moment.". Per full-internationalization.md, production user-facing text must use localized APIs with matching translated string-catalog entries.🌐 Proposed fix
self.model.setState(.error(.init( error: NSError( domain: "cmux.update", code: 1, - userInfo: [NSLocalizedDescriptionKey: "Updater is still starting. Try again in a moment."] + userInfo: [NSLocalizedDescriptionKey: String(localized: "updater.readiness.timeout", defaultValue: "Updater is still starting. Try again in a moment.")] ),Then add the corresponding entry to
Resources/Localizable.xcstringswith translations for all supported locales. As per coding guidelines, placeholder or English-only translations must not be used to satisfy a locale slot.🤖 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 `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift` around lines 278 - 286, Replace the hardcoded English message used in the NSError userInfo at the model.setState(.error(...)) call in UpdateController (the NSError with domain "cmux.update" and NSLocalizedDescriptionKey) with a localized lookup (e.g., use NSLocalizedString or String(localized:) with a key like "updater_still_starting_message"); update the .error initializer to pass that localized string instead of the literal; then add the matching key "updater_still_starting_message" (and translations for all supported locales) to Resources/Localizable.xcstrings per full-internationalization.md so no locale slots are left as placeholders.
133-139: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd DocC comments to public methods.
installUpdate(),attemptUpdate(), andcheckForUpdatesInCustomUI()are public API but lack the triple-slash documentation required for public symbols inPackages/. Other public methods in this file are documented—these should follow the same pattern.📝 Proposed documentation
+ /// Forces installation of the current update by auto-confirming all installable states. + /// + /// Does nothing if no installable update is available or if a force-install is already in progress. public func installUpdate() {+ /// Checks for updates and automatically confirms installation if one is found. + /// + /// Combines a check with auto-confirm behavior—useful for "update now" flows. public func attemptUpdate() {+ /// Checks for updates using the custom popover-based UI. + /// + /// Equivalent to ``checkForUpdates()`` but intended for the in-app update pill flow. public func checkForUpdatesInCustomUI() {Also applies to: 151-158, 214-217
🤖 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 `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift` around lines 133 - 139, The public methods installUpdate(), attemptUpdate(), and checkForUpdatesInCustomUI() are missing DocC triple-slash comments; add concise triple-slash (///) documentation above each public func explaining its purpose, parameters (if any), return behavior, and any side effects (e.g., sets isForceInstalling, triggers evaluateForceInstall, or updates UI state), matching the style used for other documented public methods in this file so the package passes Packages/ public-symbol doc rules.
🤖 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 `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift`:
- Around line 383-386: ensureSparkleInstallationCache currently reads
Bundle.main.bundleIdentifier which ignores the injected hostBundle; make
hostBundle a stored property set in the initializer (where hostBundle is
accepted) and change ensureSparkleInstallationCache to use
hostBundle.bundleIdentifier (e.g., guard let bundleIdentifier =
hostBundle.bundleIdentifier) and any other places that reference Bundle.main for
this purpose so the injected bundle is honored for cache path calculation.
---
Outside diff comments:
In `@Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swift`:
- Around line 278-286: Replace the hardcoded English message used in the NSError
userInfo at the model.setState(.error(...)) call in UpdateController (the
NSError with domain "cmux.update" and NSLocalizedDescriptionKey) with a
localized lookup (e.g., use NSLocalizedString or String(localized:) with a key
like "updater_still_starting_message"); update the .error initializer to pass
that localized string instead of the literal; then add the matching key
"updater_still_starting_message" (and translations for all supported locales) to
Resources/Localizable.xcstrings per full-internationalization.md so no locale
slots are left as placeholders.
- Around line 133-139: The public methods installUpdate(), attemptUpdate(), and
checkForUpdatesInCustomUI() are missing DocC triple-slash comments; add concise
triple-slash (///) documentation above each public func explaining its purpose,
parameters (if any), return behavior, and any side effects (e.g., sets
isForceInstalling, triggers evaluateForceInstall, or updates UI state), matching
the style used for other documented public methods in this file so the package
passes Packages/ public-symbol doc rules.
🪄 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: 0e81ea3f-17fa-484c-bee6-8797758a34d2
📒 Files selected for processing (3)
Packages/CmuxUpdater/Sources/CmuxUpdater/UpdateController.swiftResources/Localizable.xcstringscmuxUITests/UpdatePillUITests.swift
…iness-timeout error - ensureSparkleInstallationCache uses the injected hostBundle.bundleIdentifier instead of Bundle.main (consistency with the hostBundle param already used for SPUUpdater; the Sparkle install cache is keyed by the host app's bundle id). - The 'Updater is still starting' NSError description is now String(localized: update.error.notReady) with en + ja in Localizable.xcstrings (was hardcoded English). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
azooz2003-bit
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
CodeRabbit findings resolved: real ones fixed (init docs, sanitized error message, injected hostBundle, localized readiness error) and the rest acknowledged/withdrawn by CodeRabbit inline. CI verified green on 296dbbb (18 checks pass, 0 failing: tests, tests-build-and-lag, ui-regressions, activation-session, release-build, remote-daemon-tests, workflow-guard-tests, web-typecheck), Codex review clean, Greptile pass, dogfooded across all pill/popover states. Dismissing stale review states to merge.
1 participant
Wave 3 of the modular refactor: extract the Sparkle self-update domain out of the flat app target into a new
CmuxUpdaterpackage (a laterCmuxUpdaterUIpackage will follow for the views).Draft / not ready. This is a checkpoint so the shape is visible. So far it lands only the parts with no remaining app-target coupling; the package builds and tests green standalone but is not yet wired into the app, so it changes no runtime behavior.
This is a redesign, not a lift: the existing state model is
ObservableObject/@Publishedand the controller drives it through Combine, both forbidden in packages. The extraction rebuilds state propagation on@Observable+ the sanctioned concurrency primitives while preserving behavior exactly (critical user path).Landed in this commit (
swift build+swift testgreen):Package.swift— Sparkle from 2.5.1 (resolves 2.9.2), Swift 6 mode.UpdateTiming— display/timeout constants.UpdateFeedResolver— redesigned from a namespace-enum into an injectable struct returning a namedResolution(+ 4 Swift Testing behavioral tests).UpdateLogging— protocol seam so the package logs through the app's existingUpdateLogStore(which stays in the app target) instead of a.sharedsingleton.UpdateState— the phase enum lifted into its own public file.UpdateStateModel—ObservableObject→@MainActor @Observable. All state writes funnel throughsetState/setOverrideState, which emit on astateChanges()AsyncStream<Void>(the@Observable-native replacement for$state.sink/CombineLatest). SwiftUIColorcomputeds are deferred toCmuxUpdaterUI.Still to do before this leaves draft:
UpdateController— replace the three Combine observers (force-install, attempt-update, no-update auto-dismiss) with one long-lived@MainActor TaskoverstateChanges(); relocate theasyncAftertiming sites behind one-shotDispatchSourcetimers.UpdateDriver(SPUUserDriver) +UpdateSettings+ test support; invert theAppDelegate/TerminalControllerreach-ins behind seams.CmuxUpdaterUIpackage (UpdatePill,UpdatePopoverView,UpdateBadge+ the Color computeds + anUpdateActionsHostseam).cmux.xcodeproj(cmux+cmux-unit);AppDelegatebecomes the composition root + one-line forwards; splitFocusLogStoreinto the app target.cmux+cmux-unitgreen; dogfood the update pill/popover.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted
Sparkleself-update intoCmuxUpdaterandCmuxUpdaterUI, fully wired into the app with an@MainActorcontroller/driver. Added UI tests and localized error copy; builds/tests are green locally and in CI withSparkle2.8.1.Refactors
CmuxUpdater+CmuxUpdaterUIintocmux.xcodeproj; removed legacySources/Update/*and the Combine-based controller.AppDelegate: ownsUpdateLogStore; constructsUpdateController(log:); conforms toUpdateActionDelegateandUpdateActionsHost.ContentViewusesUpdateStateModel;UpdatePill/UpdatePopoverViewcall intoUpdateActionsHostand useUpdateAppearance(accent:); titlebar accessory now takesUpdateLogStore.UpdateLogging,UpdateClock/SystemUpdateClock,UpdateActionDelegate,UpdateSettings; bounded, cancellable delays for min-display, no-update auto-dismiss, and readiness.CMUX_UI_TEST_*.FileManagerand hostBundleintoUpdateControllerfor testability;ensureSparkleInstallationCachekeys by bundle id.Bug Fixes
Sparkle2.8.1 by adding@preconcurrencyimports and pinning versions in both packages.update.error.failed.messageandupdate.error.notReady(en/ja); full details stay in the log and “Copy Details.”cmux-unitby using an instanceUpdateFeedResolverand updated readiness docs.waitForExistenceassertions.Written for commit 296dbbb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Documentation