Skip to content

Fix NIGHTLY update bundle icon metadata - #4353

Merged
austinywang merged 15 commits into
mainfrom
issue-4350-nightly-update-quit-dialog
May 19, 2026
Merged

austinywang merged 15 commits into
mainfrom
issue-4350-nightly-update-quit-dialog

Conversation

@austinywang

@austinywang austinywang commented May 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • stop app-icon persistence from mutating NIGHTLY/debug channel bundles; only stable com.cmuxterm.app may persist a user-selected bundle icon
  • pass a smoke-launch disable argument and seed the shared app defaults key so release/nightly packaging smokes cannot mutate signed app bundles before DMG creation
  • add final artifact verification for stable/nightly bundle name, icon plist keys, icon file, Finder custom-icon detritus, and strict codesign after smoke launch

Fixes #4350.

Investigation

  • /Applications/cmux NIGHTLY.app/Contents/Info.plist currently has CFBundleName=cmux NIGHTLY, CFBundleDisplayName=cmux NIGHTLY, CFBundleIconFile=AppIcon-Nightly, and CFBundleIconName=AppIcon-Nightly. Stable differs as expected with cmux and AppIcon.
  • Published NIGHTLY DMGs immediately before, at, and after 19d70ef4f all have the correct plist keys and AppIcon-Nightly.icns, so the regression was not a checked-in plist patch removal.
  • The latest NIGHTLY DMG does contain an app-root Icon\r file plus com.apple.FinderInfo, and codesign --verify --deep --strict fails with resource fork, Finder information, or similar detritus not allowed. That metadata makes LaunchServices prefer stale/custom Finder icon state over CFBundleIconFile / CFBundleIconName, which matches the generic-folder prompt.
  • Exact breaking commit: 0de573e33 (Persist app icon mode on the app bundle (#2884)). It introduced NSWorkspace.shared.setIcon in Sources/AppIconDockTilePlugin.swift; the nightly/release workflows then smoke-launch the signed app before DMG packaging, so the runtime Dock tile path mutates the artifact.

Architecture

Bundle channel identity now has one owner: build-time bundle metadata. The Dock tile plugin can still persist alternate icons for the installed stable app, but NIGHTLY/debug variants and CI smoke launches cannot write Finder custom-icon metadata into app bundles. CI smoke launches cross the Dock plug-in process boundary through the same app defaults domain the plug-in already uses for icon mode. The packaging workflows verify this invariant after the launch smoke and before DMG creation.

Testing

  • Added regression coverage for the bundle icon persistence policy and smoke-launch defaults mirroring.
  • Verified scripts/verify-app-bundle-channel-metadata.sh /tmp/cmux4350-mnt/cmux\ NIGHTLY.app nightly fails on the current published NIGHTLY DMG because it contains Icon\r.
  • Verified scripts/verify-app-bundle-channel-metadata.sh /Applications/cmux.app stable passes on the installed stable app.
  • Did not run local tests; repo policy says tests run in CI.

Note

Medium Risk
Touches macOS packaging/smoke-launch and managed-default side-effect application order, which can affect release pipelines and startup behavior, but is largely additive with targeted tests/guards.

Overview
Stops app-bundle icon persistence from mutating non-stable channel bundles. Adds AppBundleIconPersistencePolicy and wires the Dock tile plugin to only persist Finder icons for the stable com.cmuxterm.app/cmux.app, with a launch-arg/defaults kill switch.

Hardens CI packaging against icon/Finder metadata drift. Smoke-launch now seeds a shared defaults flag and passes --cmux-disable-bundle-icon-persistence, and both nightly.yml and release.yml run a new verify-app-bundle-channel-metadata.sh check after smoke launch to assert expected plist name/id/icon keys, required .icns, absence of Icon\r/com.apple.FinderInfo, and strict codesign verification.

Refines startup side effects and quit-warning gating. Managed settings import now defers live default side effects until cmuxApp applies language/appearance, and quit confirmation logic is centralized via QuitWarningSettings.shouldShowConfirmation to avoid double prompts; tests expand coverage for the new policy, deferred side effects, and socket autodiscovery robustness.

Reviewed by Cursor Bugbot for commit 5e4418a. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Locks bundle icon persistence to the stable app and verifies app-bundle icon metadata during packaging; also respects the quit warning in tagged DEV builds. NIGHTLY/debug builds stay codesign-clean and avoid the generic-folder icon prompt. Fixes #4350.

  • Bug Fixes
    • Only persist the bundle icon for stable com.cmuxterm.app / cmux.app via AppBundleIconPersistencePolicy; mirror --cmux-disable-bundle-icon-persistence to shared defaults at startup, seed it in scripts/smoke-launch-macos-app.sh, and initialize this flag before replaying deferred settings side effects. nightly.yml and release.yml run scripts/verify-app-bundle-channel-metadata.sh to validate plist name/id/icon keys, required .icns, absence of Icon\r/com.apple.FinderInfo, and strict codesign.
    • Respect “Warn Before Quit” in tagged DEV builds and avoid double prompts using QuitWarningSettings.shouldShowConfirmation.
    • Defer managed-default side effects during initial settings import, batch and order them, then apply after appearance setup. Tests cover the policy, defaults mirroring, deferred replay and ordering, and initial-import notifications.

Written for commit 5e4418a. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Icon persistence now respects release channel and an explicit disable flag; quit-before-exit confirmation follows user preference.
  • New Features

    • Smoke-launch can signal the app to disable icon persistence at launch.
    • Release and nightly pipelines now verify app-bundle channel metadata before packaging.
  • Refactor

    • Startup defers managed-default side effects during initial load and applies them later.
  • Tests

    • Added tests for icon persistence, quit-warning logic, deferred startup side effects, and socket autodiscovery.

Review Change Stack

@vercel

vercel Bot commented May 19, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment May 19, 2026 10:19am
cmux-staging Building Building Preview, Comment May 19, 2026 10:19am

@coderabbitai

coderabbitai Bot commented May 19, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3394c142-fd2e-4cb0-a856-c21346e54b6d

📥 Commits

Reviewing files that changed from the base of the PR and between 28b29b6 and 5e4418a.

📒 Files selected for processing (1)
  • Sources/cmuxApp.swift

📝 Walkthrough

Walkthrough

Adds a channel-aware bundle metadata verifier and integrates it into nightly/release post‑notarization steps; introduces AppBundleIconPersistencePolicy with app/plugin and smoke-launch wiring to control dock icon persistence; refactors keyboard-settings startup side-effect handling and updates tests including a CLI socket test refactor.

Changes

Bundle metadata validation and icon persistence

Layer / File(s) Summary
Bundle metadata verification script & CI hooks
scripts/verify-app-bundle-channel-metadata.sh, .github/workflows/nightly.yml, .github/workflows/release.yml
Adds a Bash verifier that checks bundle path, Contents/Info.plist CFBundle* keys, channel-specific expected values, icon existence, rejects Finder custom icon/FinderInfo xattr, conditionally runs codesign; nightly/release workflows call the verifier post-notarization.
Icon persistence policy and unit tests
Sources/App/AppBundleIconPersistencePolicy.swift, cmuxTests/AppearanceSettingsTests.swift
New AppBundleIconPersistencePolicy centralizes stable bundle identity and disable flag, provides shouldPersist and updateDisableDefault; tests cover stable/nightly/debug branches and disable-flag behavior.
Xcode project wiring for new file
cmux.xcodeproj/project.pbxproj
Adds AppBundleIconPersistencePolicy.swift to project file references and to cmux and CmuxDockTilePlugin target sources and the Sources group.
App/plugin integration and smoke-launch
Sources/AppIconDockTilePlugin.swift, Sources/cmuxApp.swift, scripts/smoke-launch-macos-app.sh
Dock tile plugin delegates persistence to the new policy; app init syncs disable-argument to UserDefaults and applies deferred keyboard settings side effects; smoke-launch writes the disable defaults key and passes --cmux-disable-bundle-icon-persistence when opening the app.
Quit warning gating and tests
Sources/AppDelegate.swift, Sources/cmuxApp.swift, cmuxTests/ShortcutAndCommandPaletteTests.swift
Adds QuitWarningSettings.shouldShowConfirmation(...), AppDelegate uses it to decide quit confirmation and quit-shortcut behavior; unit test verifies enabled/disabled preference outcomes.
KeyboardShortcutSettingsFileStore: suppress live side effects on startup
Sources/KeyboardShortcutSettingsFileStore.swift, cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
reload gains applyLiveDefaultSideEffects flag; apply/restore helpers thread the flag to avoid running live default side effects during initial load; startup tests ensure deferred side effects are applied only after explicit invocation.
CLI socket autodiscovery test refactor
tests/test_cli_socket_autodiscovery.py
Refactors PingServer to use a monotonic deadline-driven accept loop, a shutdown event, per-connection handler threads, and a new _handle_connection method; adds time import.

Sequence Diagram(s)

sequenceDiagram
  participant Workflow as Nightly/Release Workflow
  participant Verifier as verify-app-bundle-channel-metadata.sh
  participant Plist as Info.plist reader
  participant Icon as .icns file check
  participant Codesign as codesign verifier
  Workflow->>Verifier: invoke (app-path, channel)
  Verifier->>Plist: read & validate CFBundle* keys
  Verifier->>Icon: check .icns exists & non-empty
  Verifier->>Codesign: if _CodeSignature present -> codesign --verify
  Verifier->>Workflow: exit 0 on success (continue packaging)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • manaflow-ai/cmux#4318: Both PRs touch the macOS notarization/release flow and the smoke-launch validation path.
  • manaflow-ai/cmux#4345: Related changes to KeyboardShortcutSettingsFileStore startup timing and tests.

Poem

"I'm a rabbit in the build farm, hopping through the night —
I checked each plist and icon to make the channel right.
Stable keeps its badge, nightly shows its name,
Tests tap once more, and CI closed the chain.
🐇✨ All icons aligned, the builds sleep tight."


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Cmux Swiftui State Layout ❌ Error New ObservableObject with @Published violates SwiftUI state layout rule. PR introduces KeyboardShortcutSettingsObserver(ObservableObject, @Published) where @Observable is modern shape. Refactor KeyboardShortcutSettingsObserver from ObservableObject/@published to @Observable macro pattern for modern SwiftUI state.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (15 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix NIGHTLY update bundle icon metadata' is directly related to the main objective of the PR, which is to fix the issue where NIGHTLY bundle icon metadata was being incorrectly mutated and displayed with generic folder icon.
Description check ✅ Passed The PR description is comprehensive and follows the template structure with Summary, Testing, Checklist sections. It clearly explains what changed, why, investigation findings, and architecture decisions.
Linked Issues check ✅ Passed All coding requirements from issue #4350 are met: AppBundleIconPersistencePolicy restricts icon persistence to stable bundles, smoke-launch passes disable flag, scripts/verify-app-bundle-channel-metadata.sh validates bundle metadata, and regression tests cover the policy and defaults mirroring.
Out of Scope Changes check ✅ Passed All changes are scoped to the PR objectives: app icon persistence policy, smoke-launch hardening, bundle verification, managed-default side effects deferral for proper initialization order, and quit-warning logic centralization are all necessary to fix the NIGHTLY bundle icon issue.
Cmux Swift Actor Isolation ✅ Passed No actor isolation issues found. New pure enums are safe. Thread-safety properly maintained with NSLock and Thread.isMainThread checks with DispatchQueue.main.async dispatch.
Cmux Swift Blocking Runtime ✅ Passed No new blocking patterns introduced. Pre-existing NSLock usage unchanged. New AppBundleIconPersistencePolicy has no blocking primitives.
Cmux No Hacky Sleeps ✅ Passed No violations detected. New verification script has no sleep/poll patterns. Smoke-launch changes add configuration only. Python test uses proper cancellation-aware timeout logic.
Cmux Swift Concurrency ✅ Passed No new legacy async patterns introduced. New code uses synchronous patterns and existing infrastructure. Pre-existing DispatchQueue usage is AppKit-related only.
Cmux Swift @Concurrent ✅ Passed All Swift changes are synchronous. No new async functions introduced. New functions properly call synchronous methods and isolated functions. Tests marked @MainActor as required.
Cmux Swift File And Package Boundaries ✅ Passed AppBundleIconPersistencePolicy is 31 lines, single responsibility. Oversized file additions well below 250-line threshold (cmuxApp +14, FileStore +72). No responsibility mixing or boundaries violated.
Cmux Swift Logging ✅ Passed No new print/debugPrint/dump/NSLog in app code. New files have no logging. Pre-existing NSLog counts unchanged. No MainActor Logger issues. Shell scripts use echo for CLI output.
Cmux User-Facing Error Privacy ✅ Passed No violations detected. Error messages are CI/build-only scripts, developer-only context, or safe product terms. No sensitive data, vendor names, credentials, or implementation details exposed.
Cmux Full Internationalization ✅ Passed PR adds no new user-facing strings. Changes are technical constants, CI scripts, internal logic, and tests. All user-visible text uses existing localization. Complies with i18n requirements.
Cmux Architecture Rethink ✅ Passed Fixes with clear owners: PolicyEnum centralizes icon persistence, QuitWarning extracts logic, deferred side-effects bridges singleton init. No sleeps, locks, observers.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR does not modify auxiliary window code. Changes are limited to icon persistence policy, quit-warning logic, and managed settings.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-4350-nightly-update-quit-dialog

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented May 19, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes NIGHTLY/debug bundle icon corruption during CI packaging by confining NSWorkspace.shared.setIcon calls to the stable com.cmuxterm.app / cmux.app bundle only, and seeding a shared UserDefaults key before the smoke launch so the Dock-hosted tile plugin honours the disable flag without reading process arguments from the wrong process. A post-smoke verification script checks plist keys, icon file presence, absence of Icon\r/com.apple.FinderInfo, and strict codesign before DMG creation.

  • AppBundleIconPersistencePolicy: new policy enum gates persistence on both stable bundle identifier and stable app name; the smoke script seeds the disable key in the app's shared defaults domain before open, and the main app mirrors it from launch arguments at startup; the Dock plugin reads the shared key rather than process arguments.
  • KeyboardShortcutSettingsFileStore: initial settings load now defers live side-effects into an ordered, deduplicating ManagedDefaultBatchSideEffects queue; applyDeferredManagedDefaultSideEffects drains and replays them after language/appearance setup in cmuxApp.init.
  • AppDelegate / QuitWarningSettings: consolidates quit-confirmation logic into shouldShowConfirmation; intentionally removes the tagged-DEV skip so "Warn Before Quit" is respected across all build variants.

Confidence Score: 5/5

Safe to merge — the fix correctly owns the invariant at three layers (policy, shared defaults, post-smoke verification) with no regressions found.

The Dock tile plugin now reads from the shared defaults domain rather than the wrong process's launch arguments, the smoke script seeds and cleans up the key around the launch window, and the verify script gates DMG creation on all plist/codesign invariants. The deferred side-effects path is well-tested and the ordering guarantees are preserved by the deduplicating batch structure. No correctness issues were found across the Swift, shell, or workflow changes.

No files require special attention.

Important Files Changed

Filename Overview
Sources/App/AppBundleIconPersistencePolicy.swift New policy enum gates icon persistence on stable bundle id + name, and mirrors the launch arg to a shared UserDefaults key readable by the Dock tile plugin.
Sources/AppIconDockTilePlugin.swift shouldPersistBundleIcon now delegates to AppBundleIconPersistencePolicy, reading the shared-defaults key instead of launch args (Dock runs in a different process).
Sources/KeyboardShortcutSettingsFileStore.swift Initial settings load defers live side-effects into an ordered ManagedDefaultBatchSideEffects queue; applyDeferredManagedDefaultSideEffects drains and applies them after appearance setup in cmuxApp.init.
scripts/smoke-launch-macos-app.sh Seeds cmuxDisableBundleIconPersistence to YES before open, passes the matching CLI flag, and deletes the key in the EXIT trap.
scripts/verify-app-bundle-channel-metadata.sh New script validates plist name/id/icon keys, icon .icns presence, absence of Icon\r and com.apple.FinderInfo, and strict codesign for stable and nightly channels.

Sequence Diagram

sequenceDiagram
    participant CI as CI Workflow
    participant Script as smoke-launch-macos-app.sh
    participant Defaults as UserDefaults bundle domain
    participant App as cmuxApp.init MainActor
    participant Plugin as CmuxDockTilePlugin Dock.app
    participant Verify as verify-app-bundle-channel-metadata.sh

    CI->>Script: run app_path
    Script->>Defaults: defaults write BUNDLE_ID cmuxDisableBundleIconPersistence YES
    Script->>App: open --args --cmux-disable-bundle-icon-persistence
    App->>Defaults: updateDisableDefault true via UserDefaults.standard
    Plugin->>Defaults: read cmuxDisableBundleIconPersistence
    Defaults-->>Plugin: true so shouldPersist is false
    Note over Plugin: setIcon skipped no Icon file written
    Script->>Script: wait STABLE_SECONDS
    Script->>App: kill APP_PID
    Script->>Defaults: defaults delete key in EXIT trap
    CI->>Verify: run app_path stable or nightly
    Verify->>Verify: check CFBundleName CFBundleIdentifier CFBundleIconFile CFBundleIconName
    Verify->>Verify: check icns present no custom icon no FinderInfo
    Verify->>Verify: codesign verify deep strict if signed
    Verify-->>CI: exit 0 proceed to DMG
Loading

Reviews (10): Last reviewed commit: "fix: initialize icon persistence flag be..." | Re-trigger Greptile

Comment thread Sources/AppIconDockTilePlugin.swift Outdated
Comment thread Sources/AppIconDockTilePlugin.swift Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 6 files

Re-trigger cubic

coderabbitai[bot]
coderabbitai Bot previously requested changes May 19, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/AppBundleIconPersistencePolicy.swift`:
- Around line 1-23: AppBundleIconPersistencePolicy (including static properties
stableReleaseBundleIdentifier, stableReleaseAppBundleName,
disablePersistenceArgument and the method shouldPersist) is
lifecycle-independent and must be relocated out of the app target root Sources/
into a shared/helper module; move the entire enum file into an existing non-root
shared or helper directory (e.g., a utilities or Foundation-only target) so it
compiles and tests without app lifecycle dependencies, update the module
import/visibility if needed, and adjust any call sites to import the new module
or target where AppBundleIconPersistencePolicy now lives.

In `@Sources/AppIconDockTilePlugin.swift`:
- Around line 91-94: AppIconDockTilePlugin's use of
ProcessInfo.processInfo.arguments (inside NSDockTilePlugIn) cannot read the
target app's launch args so the current gate around
AppBundleIconPersistencePolicy.shouldPersist will not work; change the gating to
read a shared preference or IPC instead (e.g., a UserDefaults(suiteName:)
app-group key or DistributedNotification/CFPreferences) that both the main app
and the Dock plug-in can access. Update AppIconDockTilePlugin to check the
shared UserDefaults key (set by the main app when handling
--cmux-disable-bundle-icon-persistence) before calling
AppBundleIconPersistencePolicy.shouldPersist, and ensure the main app writes
that key at launch; alternatively use a distributed notification or other IPC to
propagate the flag from the app to the plug-in.
🪄 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: ae209b14-4170-47be-9acb-008616ea6d6a

📥 Commits

Reviewing files that changed from the base of the PR and between d1e98aa and 3e2cdf7.

📒 Files selected for processing (4)
  • Sources/AppBundleIconPersistencePolicy.swift
  • Sources/AppIconDockTilePlugin.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/AppearanceSettingsTests.swift

Comment thread Sources/App/AppBundleIconPersistencePolicy.swift
Comment thread Sources/AppIconDockTilePlugin.swift
Comment thread Sources/App/AppBundleIconPersistencePolicy.swift
@lawrencecchen
lawrencecchen dismissed coderabbitai[bot]’s stale review May 19, 2026 07:51

Dismissed as outdated after the requested changes were addressed in 69c6208: the policy moved under Sources/App, the Dock tile plugin now reads shared app defaults instead of process arguments, and CodeRabbit marked the inline threads addressed.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 6 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread cmuxTests/AppearanceSettingsTests.swift Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Sources/KeyboardShortcutSettingsFileStore.swift (1)

207-230: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Deferred side effects are dropped instead of replayed.

reload(applyLiveDefaultSideEffects: false) writes the managed defaults, but the later reapplyManagedSettingsIfNeeded() pass only emits side effects when a value is mutated. By then the stored value already matches the imported default, so didMutateStoredValue is false and notifications/runtime updates like TerminalScrollBarSettings and AgentSessionAutoResumeSettings never fire. The removed-key path is worse: once startup restores a backup with side effects suppressed, activeManagedUserDefaults can be empty and the later reapply path never runs at all. Please track suppressed side effects and flush them after initialization, or let the first post-startup replay apply side effects even when no storage mutation is needed.

Also applies to: 865-925, 1066-1072, 1152-1158

🤖 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 `@cmuxTests/ShortcutAndCommandPaletteTests.swift`:
- Around line 1589-1605: Add the missing fourth assertion to fully cover all
branches: in the testShouldShowConfirmationFollowsEnabledPreference function,
after setting defaults.set(false, forKey:
QuitWarningSettings.warnBeforeQuitKey), add an assertion that
QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: true,
defaults: defaults) returns false; this uses the existing defaults variable and
exercises QuitWarningSettings.shouldShowConfirmation and the
QuitWarningSettings.warnBeforeQuitKey path for the (false, true) case.

In `@tests/test_cli_socket_autodiscovery.py`:
- Around line 83-84: Replace the use of the aliased socket.timeout exception
with the builtin TimeoutError in the exception handler to be idiomatic for
Python 3.10+; locate the try/except block in
tests/test_cli_socket_autodiscovery.py (the loop that currently has "except
socket.timeout: continue") and change it to "except TimeoutError: continue" so
the test catches the canonical exception type (no new imports required).
- Around line 101-116: In _handle_connection, replace the socket-specific
exception in the except clause with the modern built-in TimeoutError to match
the earlier change: update the except tuple (currently except
(ConnectionResetError, socket.timeout):) to use TimeoutError instead of
socket.timeout so the handler catches ConnectionResetError and TimeoutError
consistently.
🪄 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: e2dedcf2-fe39-478c-8f31-c6da205acf61

📥 Commits

Reviewing files that changed from the base of the PR and between f3af2f7 and 5d95a65.

📒 Files selected for processing (6)
  • Sources/AppDelegate.swift
  • Sources/KeyboardShortcutSettingsFileStore.swift
  • Sources/cmuxApp.swift
  • cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • tests/test_cli_socket_autodiscovery.py

Comment thread cmuxTests/ShortcutAndCommandPaletteTests.swift
Comment thread tests/test_cli_socket_autodiscovery.py Outdated
Comment thread tests/test_cli_socket_autodiscovery.py

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 3 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/AppDelegate.swift">

<violation number="1" location="Sources/AppDelegate.swift:1545">
P2: Tagged DEV builds no longer bypass quit confirmation in `applicationShouldTerminate`, causing an unintended warning dialog regression.</violation>

<violation number="2" location="Sources/AppDelegate.swift:11193">
P2: `handleQuitShortcutWarning` lost the tagged DEV bypass, so Cmd+Q can now prompt instead of exiting immediately on tagged DEV builds.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread Sources/AppDelegate.swift
}

if !QuitWarningSettings.isEnabled() {
if !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: false) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: handleQuitShortcutWarning lost the tagged DEV bypass, so Cmd+Q can now prompt instead of exiting immediately on tagged DEV builds.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 11193:

<comment>`handleQuitShortcutWarning` lost the tagged DEV bypass, so Cmd+Q can now prompt instead of exiting immediately on tagged DEV builds.</comment>

<file context>
@@ -11204,13 +11190,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
-        }
-
-        if !QuitWarningSettings.isEnabled() {
+        if !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: false) {
             NSApp.terminate(nil)
             return true
</file context>
Suggested change
if !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: false) {
if SocketControlSettings.isTaggedDevBuild() || !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: false) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional for this PR. The latest dogfood report was that tagged DEV had Warn Before Quit enabled but skipped the dialog; tagged DEV now honors the same quit-warning preference so this branch can verify that path.

— Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it—thanks for the context. I’ll treat the tagged DEV bypass removal as intentional for this PR.

Comment thread Sources/AppDelegate.swift
Comment on lines +1545 to 1549
if !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: isQuitWarningConfirmed) {
closeAllWebInspectorsBeforeAppTeardown()
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "taggedDev"])
return .terminateNow
}

// If the user already confirmed via the Cmd+Q shortcut warning dialog
// (handleQuitShortcutWarning), skip the check to avoid a second alert.
if isQuitWarningConfirmed {
closeAllWebInspectorsBeforeAppTeardown()
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "confirmed"])
return .terminateNow
}

// Respect the "Warn Before Quit" setting even when Cmd+Q arrives via
// the Cmd+Tab app switcher, bypassing handleCustomShortcut.
guard QuitWarningSettings.isEnabled() else {
closeAllWebInspectorsBeforeAppTeardown()
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "warningDisabled"])
let reason = isQuitWarningConfirmed ? "confirmed" : "warningDisabled"
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": reason])
return .terminateNow

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Tagged DEV builds no longer bypass quit confirmation in applicationShouldTerminate, causing an unintended warning dialog regression.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 1545:

<comment>Tagged DEV builds no longer bypass quit confirmation in `applicationShouldTerminate`, causing an unintended warning dialog regression.</comment>

<file context>
@@ -1540,26 +1540,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
-        if SocketControlSettings.isTaggedDevBuild() {
+        // If the user already confirmed via the Cmd+Q shortcut warning dialog,
+        // or disabled the setting, skip the check to avoid a second alert.
+        if !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: isQuitWarningConfirmed) {
             closeAllWebInspectorsBeforeAppTeardown()
-            StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "taggedDev"])
</file context>
Suggested change
if !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: isQuitWarningConfirmed) {
closeAllWebInspectorsBeforeAppTeardown()
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "taggedDev"])
return .terminateNow
}
// If the user already confirmed via the Cmd+Q shortcut warning dialog
// (handleQuitShortcutWarning), skip the check to avoid a second alert.
if isQuitWarningConfirmed {
closeAllWebInspectorsBeforeAppTeardown()
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "confirmed"])
return .terminateNow
}
// Respect the "Warn Before Quit" setting even when Cmd+Q arrives via
// the Cmd+Tab app switcher, bypassing handleCustomShortcut.
guard QuitWarningSettings.isEnabled() else {
closeAllWebInspectorsBeforeAppTeardown()
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": "warningDisabled"])
let reason = isQuitWarningConfirmed ? "confirmed" : "warningDisabled"
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": reason])
return .terminateNow
if SocketControlSettings.isTaggedDevBuild() || !QuitWarningSettings.shouldShowConfirmation(isQuitWarningConfirmed: isQuitWarningConfirmed) {
closeAllWebInspectorsBeforeAppTeardown()
let reason = SocketControlSettings.isTaggedDevBuild()
? "taggedDev"
: (isQuitWarningConfirmed ? "confirmed" : "warningDisabled")
StartupBreadcrumbLog.append("appDelegate.shouldTerminate.terminateNow", fields: ["reason": reason])
return .terminateNow
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional for this PR. The tagged DEV bypass caused the reported regression where Warn Before Quit was enabled but no confirmation appeared, so applicationShouldTerminate now uses the shared quit-warning policy.

— Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood—thanks for the clarification. Noted that the change is intentional to use the shared quit-warning policy in this PR.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit aaec9d5. Configure here.

Comment thread Sources/KeyboardShortcutSettingsFileStore.swift Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 949-954: Drain deferred side effects before applying the live
batch: call drainDeferredManagedDefaultSideEffects() first, then merge the
current sideEffects into the drained batch (so the live sideEffects
overwrite/coalesce stale deferred entries), and pass that merged batch to
applyManagedDefaultBatchSideEffects(sideEffects) (leave
deferManagedDefaultSideEffects(sideEffects) in the else branch unchanged);
update the logic around drainDeferredManagedDefaultSideEffects(),
sideEffects.merge(...), and applyManagedDefaultBatchSideEffects(...) so the
drained batch is the base and the live sideEffects take precedence.
🪄 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: 443d4988-d98f-4f93-abf6-ef905248a408

📥 Commits

Reviewing files that changed from the base of the PR and between 5d95a65 and aaec9d5.

📒 Files selected for processing (5)
  • Sources/KeyboardShortcutSettingsFileStore.swift
  • Sources/cmuxApp.swift
  • cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • tests/test_cli_socket_autodiscovery.py

Comment thread Sources/KeyboardShortcutSettingsFileStore.swift
@austinywang

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 47-57: Move the bundle-icon disable flag setup so it runs before
deferred side effects: call
AppBundleIconPersistencePolicy.updateDisableDefault(defaults:
UserDefaults.standard, launchArguments: ProcessInfo.processInfo.arguments)
before
KeyboardShortcutSettings.settingsFileStore.applyDeferredManagedDefaultSideEffects()
(so the shared disable flag is present when AppIconSettings.applyIcon()
notifications fire), effectively relocating the updateDisableDefault()
invocation above applyDeferredManagedDefaultSideEffects() and preserving the
existing defaults/launchArguments usage.
🪄 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: 22438fe6-fa6c-4a37-991f-4002525d1283

📥 Commits

Reviewing files that changed from the base of the PR and between 5d95a65 and 28b29b6.

📒 Files selected for processing (5)
  • Sources/KeyboardShortcutSettingsFileStore.swift
  • Sources/cmuxApp.swift
  • cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • tests/test_cli_socket_autodiscovery.py

Comment thread Sources/cmuxApp.swift Outdated
@austinywang

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

This branch was successfully deployed

1 active deployment
Preview – cmux — 5e4418ab Deployed May 19, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cmux NIGHTLY update quit dialog shows "cmux" + generic folder icon (regression)

1 participant