Repository navigation
Persist app icon mode on the app bundle - #2884
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCmuxDockTilePlugin now observes Changes
Sequence Diagram(s)sequenceDiagram
participant NSApp
participant CmuxPlugin as CmuxDockTilePlugin
participant ImageResolver
participant NSWorkspace
participant DockTile
NSApp->>CmuxPlugin: effectiveAppearance change (observed)
CmuxPlugin->>CmuxPlugin: schedule updateDockTile on main queue
CmuxPlugin->>ImageResolver: request imageName(isDarkAppearance:)
ImageResolver-->>CmuxPlugin: icon or nil
alt icon available
CmuxPlugin->>NSWorkspace: setIcon(icon, forFile: appBundle.path)
NSWorkspace-->>CmuxPlugin: ok
CmuxPlugin->>NSWorkspace: noteFileSystemChanged(appBundle.path)
CmuxPlugin->>DockTile: show custom icon
else icon missing
CmuxPlugin->>NSWorkspace: setIcon(nil, forFile: appBundle.path)
NSWorkspace-->>CmuxPlugin: ok
CmuxPlugin->>NSWorkspace: noteFileSystemChanged(appBundle.path)
CmuxPlugin->>DockTile: revert to default icon
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryPersists the user's chosen app icon by writing it directly to the app bundle via Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App (launch / settings change)
participant AI as applyIcon()
participant NW as NSWorkspace
participant Finder as Finder
participant LS as LaunchServices (bg)
App->>AI: applyIcon(.dark / .light)
AI->>AI: stopAppearanceObservation()
AI->>AI: setApplicationIconImage(icon)
AI->>NW: setIcon(icon, forFile: bundlePath)
NW-->>AI: true
AI->>Finder: noteFileSystemChanged(bundlePath)
AI->>LS: LSRegisterURL(bundleURL) [async]
AI->>App: notifyDockTilePlugin()
App->>AI: applyIcon(.automatic)
AI->>NW: setIcon(nil, forFile: bundlePath)
NW-->>AI: true
AI->>Finder: noteFileSystemChanged(bundlePath)
AI->>LS: LSRegisterURL(bundleURL) [async]
AI->>AI: startAppearanceObservation()
Note over AI: applyIconForCurrentAppearance()<br/>sets runtime icon for current theme
AI->>App: notifyDockTilePlugin()
Reviews (1): Last reviewed commit: "Persist app icon mode on the app bundle" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/cmuxApp.swift (1)
3853-3855: Optional dedupe: extract shared fixed-mode icon apply path.The
.lightand.darkbranches now share identical bundle-update logic; a helper would reduce drift risk.♻️ Proposed refactor
static func applyIcon(_ mode: AppIconMode, environment: Environment = .live()) { + func applyFixedModeIcon(_ mode: AppIconMode) { + guard let icon = environment.imageForMode(mode) else { return } + environment.setApplicationIconImage(icon) + if environment.setBundleIcon(icon) { + environment.notifyBundleIconSystem() + } + } + switch mode { case .automatic: if environment.setBundleIcon(nil) { environment.notifyBundleIconSystem() } environment.startAppearanceObservation() case .light: environment.stopAppearanceObservation() - guard let icon = environment.imageForMode(.light) else { return } - environment.setApplicationIconImage(icon) - if environment.setBundleIcon(icon) { - environment.notifyBundleIconSystem() - } + applyFixedModeIcon(.light) case .dark: environment.stopAppearanceObservation() - guard let icon = environment.imageForMode(.dark) else { return } - environment.setApplicationIconImage(icon) - if environment.setBundleIcon(icon) { - environment.notifyBundleIconSystem() - } + applyFixedModeIcon(.dark) } environment.notifyDockTilePlugin() }Also applies to: 3860-3862
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3853 - 3855, The .light and .dark branches duplicate the same fixed-mode bundle update calls (environment.setBundleIcon(icon) and environment.notifyBundleIconSystem()); extract that shared logic into a small helper function (e.g., applyFixedModeBundleIcon(icon) or a private method on the same scope) and call it from both branches to avoid duplication and drift, ensuring you move the environment.setBundleIcon(icon) + environment.notifyBundleIconSystem() sequence into that helper and replace the duplicated lines in the .light and .dark cases with a single helper call.cmuxTests/NotificationAndMenuBarTests.swift (1)
33-34: Assert bundle-icon side effects, not just wire stubs.These tests now satisfy the new
Environmentsignature, but they don’t verify the new behavior this PR introduces (setBundleIconargument andnotifyBundleIconSysteminvocation). Please capture/assert these side effects in both dark and automatic paths to protect persistence/clear regressions.✅ Suggested test assertions
func testApplyDarkSetsRuntimeIconAndNotifiesDockTilePlugin() { let expectedIcon = NSImage(size: NSSize(width: 16, height: 16)) var receivedRuntimeIcon: NSImage? + var receivedBundleIcon: NSImage? + var bundleNotifyCallCount = 0 var dockTileNotificationCount = 0 var startObservationCallCount = 0 var stopObservationCallCount = 0 let environment = AppIconSettings.Environment( @@ setApplicationIconImage: { icon in receivedRuntimeIcon = icon }, - setBundleIcon: { _ in true }, - notifyBundleIconSystem: {}, + setBundleIcon: { icon in + receivedBundleIcon = icon + return true + }, + notifyBundleIconSystem: { + bundleNotifyCallCount += 1 + }, @@ AppIconSettings.applyIcon(.dark, environment: environment) XCTAssertTrue(receivedRuntimeIcon === expectedIcon) + XCTAssertTrue(receivedBundleIcon === expectedIcon) + XCTAssertEqual(bundleNotifyCallCount, 1) XCTAssertEqual(dockTileNotificationCount, 1) XCTAssertEqual(startObservationCallCount, 0) XCTAssertEqual(stopObservationCallCount, 1) } func testApplyAutomaticStartsObservationAndNotifiesDockTilePlugin() { + var bundleIconUpdates: [NSImage?] = [] + var bundleNotifyCallCount = 0 var dockTileNotificationCount = 0 var startObservationCallCount = 0 var stopObservationCallCount = 0 let environment = AppIconSettings.Environment( @@ - setBundleIcon: { _ in true }, - notifyBundleIconSystem: {}, + setBundleIcon: { icon in + bundleIconUpdates.append(icon) + return true + }, + notifyBundleIconSystem: { + bundleNotifyCallCount += 1 + }, @@ AppIconSettings.applyIcon(.automatic, environment: environment) + XCTAssertEqual(bundleIconUpdates.count, 1) + if let firstUpdate = bundleIconUpdates.first { + XCTAssertNil(firstUpdate) + } + XCTAssertEqual(bundleNotifyCallCount, 1) XCTAssertEqual(dockTileNotificationCount, 1) XCTAssertEqual(startObservationCallCount, 1) XCTAssertEqual(stopObservationCallCount, 0) }Based on learnings: In
Sources/cmuxApp.swift,AppIconSettings.applyIcon(.automatic)and dock-tile behavior depend on bundle-icon state transitions; tests should verify those transitions, not only runtime icon/delegate calls.Also applies to: 67-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/NotificationAndMenuBarTests.swift` around lines 33 - 34, Update the tests to assert the new bundle-icon side effects rather than only stubbing the environment: capture calls to the Environment.setBundleIcon closure and the notifyBundleIconSystem invocation in NotificationAndMenuBarTests (both dark and automatic icon-path tests) and assert the expected state transitions (e.g., that setBundleIcon is called with .dark when applying .dark and called with .automatic/cleared when applying .automatic) and that notifyBundleIconSystem is invoked each time; use spies/flags or a mutable capture to record arguments and invocation counts and add assertions in both the dark-path and automatic-path test cases to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/NotificationAndMenuBarTests.swift`:
- Around line 33-34: Update the tests to assert the new bundle-icon side effects
rather than only stubbing the environment: capture calls to the
Environment.setBundleIcon closure and the notifyBundleIconSystem invocation in
NotificationAndMenuBarTests (both dark and automatic icon-path tests) and assert
the expected state transitions (e.g., that setBundleIcon is called with .dark
when applying .dark and called with .automatic/cleared when applying .automatic)
and that notifyBundleIconSystem is invoked each time; use spies/flags or a
mutable capture to record arguments and invocation counts and add assertions in
both the dark-path and automatic-path test cases to prevent regressions.
In `@Sources/cmuxApp.swift`:
- Around line 3853-3855: The .light and .dark branches duplicate the same
fixed-mode bundle update calls (environment.setBundleIcon(icon) and
environment.notifyBundleIconSystem()); extract that shared logic into a small
helper function (e.g., applyFixedModeBundleIcon(icon) or a private method on the
same scope) and call it from both branches to avoid duplication and drift,
ensuring you move the environment.setBundleIcon(icon) +
environment.notifyBundleIconSystem() sequence into that helper and replace the
duplicated lines in the .light and .dark cases with a single helper call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aef090f8-92c2-424d-8cab-1960d847626b
📒 Files selected for processing (2)
Sources/cmuxApp.swiftcmuxTests/NotificationAndMenuBarTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
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="cmuxTests/NotificationAndMenuBarTests.swift">
<violation number="1" location="cmuxTests/NotificationAndMenuBarTests.swift:33">
P3: Add call-count assertions for `setBundleIcon` and `notifyBundleIconSystem` in these tests so they fail if the persistence/notification path is removed or no longer executed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppIconDockTilePlugin.swift`:
- Around line 81-82: The icon-update paths that call
NSWorkspace.shared.setIcon(...) should also re-register the app bundle with
LaunchServices: after each NSWorkspace change (the branches containing
NSWorkspace.shared.setIcon(nil, forFile: appBundleURL.path, ...) and the other
icon-mutation branch), call LSRegisterURL(appBundleURL as CFURL, true) (or
mirror the AppDelegate.scheduleLaunchServicesBundleRegistration() behavior) to
force LaunchServices metadata refresh, and ensure the call is invoked on the
same execution context as the existing NSWorkspace calls.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84e3c7a2-3c3f-47e6-8d76-62ab3b57db1d
📒 Files selected for processing (1)
Sources/AppIconDockTilePlugin.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 718052e054
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppIconDockTilePlugin.swift`:
- Around line 16-19: DockTileAppIconMode.imageName(isDarkAppearance:) currently
returns concrete NSImage names for the .automatic case which causes the dock
plugin to persist custom icons; change the .automatic branch in
DockTileAppIconMode.imageName(isDarkAppearance:) to return nil (matching
cmuxApp.swift's AppIconMode.automatic) so the guard that checks imageName fails
and the code path that calls setIcon(nil, ...) is executed to clear bundle
overrides instead of persisting icons; update any other imageName
implementations in DockTileAppIconMode (e.g., the similar branches referenced
around lines 62-69 and 88-105) to follow the same nil-return behavior for
.automatic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79ed1159-6247-459c-9de6-1791b2dc4ad9
📒 Files selected for processing (1)
Sources/AppIconDockTilePlugin.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4b92e80e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced May 6, 2026
rodchristiansen
pushed a commit
to rodchristiansen/cmux
that referenced
this pull request
Sep 2, 2026
* Persist app icon mode on the app bundle * Move app icon persistence into the dock tile plugin * Make automatic app icon follow Dock appearance * Refresh LaunchServices after dock icon changes * Skip app icon sync during test launches * Skip bundle icon persistence for untagged debug app --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CmuxDockTilePlugin, which is the Dock-side source of truthVerification
./scripts/setup.sh./scripts/reload.sh --tag task-fix-app-icon-persistencekillall Dockonce to make Dock load the updated plugin binarydefaults write com.cmuxterm.app.debug.task.fix.app.icon.persistence appIconMode -string dark.appgainsIcondefaults write com.cmuxterm.app.debug.task.fix.app.icon.persistence appIconMode -string automatic.appno longer hasIconNotes
NSWorkspace.setIconpath was removed. The plugin now owns persistence because Dock icon state after force-kill is resolved from the Dock side, not the app process.Summary by CodeRabbit