Fix unexpected menu-bar-only activation policy#6068
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a two-key explicit-enable scheme ( ChangesMenu-bar-only confirmation guard
Sequence Diagram(s)sequenceDiagram
participant User
participant AppSection
participant HostSettingsActions
participant MenuBarOnlySettings
participant DefaultsValueModel
User->>AppSection: toggles Menu Bar Only
AppSection->>HostSettingsActions: setMenuBarOnly(enabled)
HostSettingsActions->>MenuBarOnlySettings: setEnabled(enabled, defaults)
MenuBarOnlySettings-->>MenuBarOnlySettings: write menuBarOnly key
MenuBarOnlySettings-->>MenuBarOnlySettings: write explicitEnableKey
HostSettingsActions-->>AppSection: returns true
AppSection->>DefaultsValueModel: acceptCommittedValue(enabled)
DefaultsValueModel-->>DefaultsValueModel: update current (no UserDefaults write)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 1228-1229: The startup code in AppDelegate.swift at the specified
location duplicates the repair and sync pattern that already exists in
syncApplicationPresentationPreferences(). Replace the two separate calls to
MenuBarOnlySettings.repairUnconfirmedStoredPreference() and
syncActivationPolicy() with a single call to
syncApplicationPresentationPreferences() to centralize the repair-sync logic and
ensure all presentation preferences are synced consistently at startup.
🪄 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: c296962a-388a-42b9-8023-e6a3e77280ad
📒 Files selected for processing (10)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Environment/SettingsHostActions.swiftPackages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftSources/App/MenuBarExtraController.swiftSources/AppDelegate.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/HostSettingsActions.swiftSources/KeyboardShortcutSettingsFileStore.swiftcmuxTests/CommandPaletteSettingsToggleTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftcmuxTests/NotificationAndMenuBarTests.swift
💤 Files with no reviewable changes (1)
- Sources/CommandPalette/CommandPaletteSettingsToggle.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/App/MenuBarExtraController.swift`:
- Around line 524-526: The isEnabled(defaults:) method in
MenuBarExtraController.swift returns a computed value based on legacy keys when
the explicit marker is missing, but it does not persist any repair to the
defaults dictionary. This causes the Settings UI in AppSection.swift to read
stale key values and display outdated state. When the function takes the legacy
fallback path and returns !legacyCommandPaletteToggleWasUsed(defaults:), you
must also backfill or clear the menuBarOnlyKey and explicitEnableKey in the
defaults object to match the computed result. This ensures that all subsequent
reads of these keys by Settings bindings and other readers see the same
authoritative, up-to-date state instead of persisting stale opt-in signals.
In `@Sources/CommandPalette/CommandPaletteSettingsToggle.swift`:
- Around line 8-10: The legacyCommandPaletteToggleWasUsed function returns false
when the history data exists but fails to deserialize, which causes
MenuBarOnlySettings.isEnabled to incorrectly treat unreadable or corrupted
history as an implicit opt-out. This causes the app to silently fall back to
.accessory mode. Modify the function to only return true when the history data
is successfully parsed and explicitly contains the command palette usage marker;
when deserialization fails or data is unreadable, the function should not
implicitly signal that the toggle was used, ensuring the app stays in .regular
mode by default unless an explicit opt-in marker is confirmed.
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Line 424: Split the compound statement at line 424 in the
KeyboardShortcutSettingsFileStore.swift file that assigns values to both
MenuBarOnlySettings.menuBarOnlyKey and MenuBarOnlySettings.explicitEnableKey
into two separate lines. Currently, the two assignments are joined by a
semicolon on a single line. Separate them onto consecutive lines to match the
idiomatic Swift style and improve readability consistent with similar operations
elsewhere in the file.
🪄 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: be8061b1-e8ec-456e-981b-a663af5df456
📒 Files selected for processing (7)
Packages/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/AppSection.swiftSources/App/MenuBarExtraController.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/KeyboardShortcutSettingsFileStore.swiftcmuxTests/CommandPaletteSettingsToggleTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreMigrationTests.swiftcmuxTests/NotificationAndMenuBarTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/NotificationAndMenuBarTests.swift
Contributor
Greptile SummaryThis PR fixes an activation-policy regression (#6065) where a stale or accidentally-persisted
Confidence Score: 5/5The PR is safe to merge: the migration is idempotent, the heuristic is conservative, and the two-key write protocol is atomic. normalizeLegacyStoredPreference runs before the first syncActivationPolicy call, writes explicitEnableKey so all subsequent reads skip the heuristic, and the guard makes every re-entry a no-op. Tests cover the full heuristic decision tree including corrupt JSON, mixed history, and settings-file snapshot paths. No files require special attention. Important Files Changed
Reviews (12): Last reviewed commit: "merge: refresh file length budget after ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.accessorymenuBarOnly=truedefaults back to regular foreground mode only when the legacy command-palette history matches the isolated one-shot instant-toggle trapFixes #6065
Root cause / repro
Code inspection found a single production activation-policy writer:
MenuBarOnlySettings.applyActivationPolicy, called at launch and onUserDefaultschanges. PR #3181 introduced menu-bar-only mode by mappingmenuBarOnly=trueto.accessory; a later command-palette settings toggle exposed that same raw default as an instant command. A raw or accidental persistedmenuBarOnly=truetherefore leaves cmux in.accessory, which removes both the Dock icon and main menu bar.Repro confirmed on
cmux-aws-m4pro/aws-m4pro-2(macOS 15.7.4) with real tagged cmux builds:2a2d87bf81d81191005582e66e2f548c0aa406cf, seeded withmenuBarOnly=trueandcommandPalette.commandUsage.v1containingpalette.toggleSetting.menuBarOnly, launched ascom.cmuxterm.app.debug.repro6065beforeand settled toactivationPolicy=accessory.dff725d2bc2519b63d36345d66ce7465074c3aba, seeded with the same isolated legacy defaults anduseCount=1, staysactivationPolicy=regular, and normalizes defaults tomenuBarOnly=0/menuBarOnlyExplicitlyEnabled.v1=0.activationPolicy=accessory,menuBarOnly=1,menuBarOnlyExplicitlyEnabled.v1=1.Cloud-mac video was attempted but blocked by infrastructure: three default cloud-mac leases never surfaced a Tailscale host, the alternate
macos-15lease stayed queued, and the available AWS Macs denied full-screen recording via Screen Recording/TCC while raw VNC exposed Apple-only auth types unsupported bycua-vnc. The repro above is therefore confirmed by live AppKit activation-policy probing rather than video.Testing
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag repro6065before,--tag repro6065after, and--tag repro6065current2.python3 scripts/swift_file_length_budget.pypasses after merging latestorigin/main; the generated budget was refreshed for the current base.xcodebuild, no localreload.sh; CI is the gate.Localization
No new user-facing strings were added. Existing Settings labels/localized strings are reused; new literals are a hidden defaults marker and test JSON.