Repository navigation
feat(sidebar): revive workspace icons - #4335
Closed
lawrencecchen wants to merge 6 commits into
Closed
lawrencecchen wants to merge 6 commits into
lawrencecchen wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds comprehensive workspace icon support to cmux, enabling users to assign custom emoji or image icons to workspaces and optionally auto-detect icons from workspace directories. The feature spans icon storage and detection, state management, UI rendering, settings integration, CLI parsing, terminal API actions, and tests. ChangesWorkspace Icon Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ Passed checks (11 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR revives workspace icons in the sidebar as circular iMessage-style avatars, adding support for emoji and image-file icons via context menu, socket actions, CLI, and an off-by-default auto-detect setting. Icons are persisted in session snapshots and included in autosave fingerprints.
Confidence Score: 5/5Safe to merge; all changes are additive and well-isolated behind the off-by-default auto-detect flag. The new icon pipeline is purely additive: custom icons default to nil, auto-detect is off by default, and session snapshots use optional Codable fields that decode gracefully on older sessions. The socket/CLI additions follow the same guard-and-finish pattern as the existing set_color/clear_color actions. All new user-facing strings are fully catalogued across every supported locale. The only observations are about background I/O not being interrupted on cancellation and file size, neither of which affects correctness. Sources/Sidebar/SidebarAppearanceSupport.swift — mixed responsibilities and the inner Task.detached image-load pattern are worth a follow-up refactor. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ContextMenu
participant TabItemView
participant TabManager
participant Workspace
participant WorkspaceIconDetector
participant WorkspaceIconView
User->>ContextMenu: Set Emoji… / Set Icon from File…
ContextMenu->>TabItemView: promptEmojiIcon / promptIconFromFile
TabItemView->>TabManager: applyWorkspaceIcon(iconPath, targetIds)
TabManager->>Workspace: setCustomIcon(iconPath)
Workspace->>Workspace: normalise + bump customIconRevision
Workspace-->>TabItemView: "@Published triggers snapshot rebuild"
TabItemView->>WorkspaceIconView: iconPath + reloadToken (via snapshot)
WorkspaceIconView->>WorkspaceIconView: .task(id: iconLoadKey) → Task.detached → Data(contentsOf:)
WorkspaceIconView-->>User: Circular avatar rendered
User->>Workspace: currentDirectory changes
Workspace->>Workspace: didSet → refreshDetectedWorkspaceIcon
Workspace->>WorkspaceIconDetector: Task.detached → detectedIconPath(in:)
WorkspaceIconDetector-->>Workspace: MainActor.run → detectedIconPath + detectedIconRevision
User->>SocketAPI: workspace.action set_icon / clear_icon
SocketAPI->>TabManager: setTabIcon(tabId:iconPath:)
TabManager->>Workspace: setCustomIcon
User->>CLI: cmux workspace-action set-icon --icon emoji:🚀
CLI->>SocketAPI: "params[icon] = icon"
Reviews (5): Last reviewed commit: "fix(sidebar): address workspace icon rev..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@CLI/cmux.swift`:
- Line 4661: Replace the raw user-facing literal passed to CLIError in
cmux.swift (the throw CLIError(message: "...") instance) with a localized string
using String(localized:defaultValue:) and ensure the same change is applied to
the other occurrences referenced (around lines noted: 9526–9534, 9547–9553,
24222). Update the localization catalog: add matching keys/entries and
translated strings for every locale already supported, and wire the new keys
into the existing localization lookup so all supported locales have the
translated message for the workspace-action set-icon error.
In `@Resources/Localizable.xcstrings`:
- Around line 112820-112832: The new localization keys (e.g.,
"alert.emojiIcon.apply", "alert.emojiIcon.cancel", "alert.emojiIcon.message",
"alert.emojiIcon.placeholder", "alert.emojiIcon.title", "contextMenu.clearIcon",
"contextMenu.setEmojiIcon", "contextMenu.setIconFromFile",
"contextMenu.workspaceIcon", "openPanel.workspaceIcon.title",
"settings.app.autoDetectWorkspaceIcon",
"settings.app.autoDetectWorkspaceIcon.subtitle",
"settings.search.alias.setting.app.auto-detect-workspace-icon") are only
populated for en and ja; add matching entries for every other locale already
present in Resources/Localizable.xcstrings so the catalog is complete. For each
key, add a localization object for each existing locale with "stringUnit" {
"state": "translated", "value": "<localized string>" } (use the correct
translated text from your localization team or a verified placeholder flagged
for review), keep "extractionState": "manual", and ensure formatting matches the
surrounding entries so no partial-localization rule is violated.
In `@Sources/ContentView.swift`:
- Around line 13225-13231: The visibility check for the “Clear Icon” menu
currently uses tab.customIconPath and should instead consider the
multi-selection state in targetIds; update the conditional that wraps the Button
(which calls applyTabIcon(nil, targetIds: targetIds)) to be true when any of the
selected targets in targetIds have a customIconPath (not just the single tab
variable). Locate the block referencing tab.customIconPath and targetIds and
replace the boolean with a check that inspects the selected items (via the same
model or collection you use elsewhere to resolve targetIds to tabs/workspaces)
and shows the Button if any selected item has a non-nil customIconPath. Ensure
the applyTabIcon call and the Label("contextMenu.clearIcon", ...) remain
unchanged.
In `@Sources/Sidebar/SidebarAppearanceSupport.swift`:
- Around line 65-69: The auto-detection list standardIconFilenames in
SidebarAppearanceSupport.swift contains .svg and .ico entries which AppKit's
NSImage(contentsOfFile:) cannot reliably decode; remove SVG and ICO entries and
limit the array to raster formats (e.g., "favicon.png", "icon.png", "logo.png")
so only PNG (or other supported raster extensions) are auto-detected, or
alternatively implement and call a dedicated SVG/ICO decoding pipeline before
images reach NSImage — update the static let standardIconFilenames definition
(and any code that iterates it) to use only supported raster filenames like the
PNG variants.
In `@Sources/TerminalController.swift`:
- Line 5653: Replace the hard-coded error string assigned to result = .err(...)
with a localized message using the app's localization API (e.g.,
NSLocalizedString or the project's localization helper) and a catalog key (e.g.,
"icon.invalid_or_missing"); update the .err call to include a user-actionable
message that suggests recovery steps such as "provide an emoji or a file path
for icon, or use the clear_icon flag" and include those suggestions in the
localized string or appended recovery field; locate the assignment to result
(the .err(code: "invalid_params", ... ) usage in TerminalController) and swap
the literal message for the localized catalog key and a concise recovery action
to meet the user-facing text guidelines.
In `@Sources/Workspace.swift`:
- Around line 7178-7184: When currentDirectory changes you must cancel the
actual detached detector Task (not only its wrapper) so scans don't continue
running; update the logic around refreshDetectedWorkspaceIcon and the
workspaceIconDetectionTask handling to store and reuse the direct Task handle
returned by Task.detached (e.g., a new property like workspaceIconDetectorTask
or replace workspaceIconDetectionTask with the detached Task itself), cancel
that Task explicitly before starting a new detection or when auto-detect is
disabled, and ensure the new Task handle is replaced with the running detector
so subsequent cancellations target the real detached scan.
🪄 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: 872c6574-3092-40ed-a16d-e0c42cf35d9f
📒 Files selected for processing (17)
CLI/cmux.swiftResources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/ContentView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SessionPersistence.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/Sidebar/SidebarAppearanceSupport.swiftSources/Sidebar/SidebarWorkspaceSnapshotRefreshPolicy.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swiftcmuxTests/WorkspaceUnitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 17 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="Sources/Sidebar/SidebarAppearanceSupport.swift">
<violation number="1" location="Sources/Sidebar/SidebarAppearanceSupport.swift:66">
P2: Auto-detect includes `.svg` candidates, but the current image loading path uses `NSImage(contentsOfFile:)`, so detected SVG files can fail to render and block available PNG fallbacks.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:4620">
P2: `--icon` accepts option-like tokens as values, so a missing icon can silently consume the next flag instead of failing fast.
(Based on your team's feedback about rejecting option-like flag values.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Sources/Workspace.swift (1)
8738-8742:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel the detached detector task directly.
Line 8739 starts the real scan with
Task.detached, so the cancellations on Lines 8712, 8725, and 7945 only cancel the wrapper task. Old scans keep running after cwd changes or when auto-detect is disabled.In Swift concurrency, does canceling a Task that is awaiting Task.detached(...).value automatically cancel the detached task, or must the detached task handle be canceled separately?♻️ Suggested fix
- workspaceIconDetectionTask?.cancel() - workspaceIconDetectionTask = Task { [weak self, directory] in - let result = await Task.detached(priority: .utility) { - WorkspaceIconDetectionResult.detect(in: directory) - }.value - - guard !Task.isCancelled, - let self, - self.workspaceIconDetectionDirectory == directory, - WorkspaceIconValue.normalizedStorageValue(self.currentDirectory) == directory else { - return - } - let didChange = self.detectedIconPath != result.path || self.detectedIconSignature != result.signature - if didChange { - self.detectedIconPath = result.path - self.detectedIconSignature = result.signature - self.detectedIconRevision &+= 1 - } - } + workspaceIconDetectionTask?.cancel() + workspaceIconDetectionTask = Task.detached(priority: .utility) { [weak self, directory] in + let result = WorkspaceIconDetectionResult.detect(in: directory) + guard !Task.isCancelled, let self else { return } + + await MainActor.run { + guard self.workspaceIconDetectionDirectory == directory, + WorkspaceIconValue.normalizedStorageValue(self.currentDirectory) == directory else { + return + } + let didChange = self.detectedIconPath != result.path || self.detectedIconSignature != result.signature + if didChange { + self.detectedIconPath = result.path + self.detectedIconSignature = result.signature + self.detectedIconRevision &+= 1 + } + } + }🤖 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/Workspace.swift` around lines 8738 - 8742, Current code awaits a Task.detached() inside workspaceIconDetectionTask but only cancels the wrapper task (workspaceIconDetectionTask?.cancel()), which does not cancel the detached detector; fix by keeping and cancelling the detached Task handle explicitly: when creating the detached operation (the Task.detached that runs WorkspaceIconDetectionResult.detect(in:)), assign that Task to a persistent variable (e.g., workspaceIconDetectionDetachedTask) before awaiting its .value, cancel any existing detached handle when starting a new scan or when disabling auto-detect, and ensure places that currently call workspaceIconDetectionTask?.cancel() also cancel the detached handle so the long-running detect(in:) is stopped promptly.Sources/ContentView.swift (1)
13229-13236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBase “Clear Icon” visibility on the targeted workspaces, not the clicked row.
Line 13230 only checks
tab.customIconPath, but the action clearstargetIds. In a multi-selection, right-clicking a workspace without a custom icon hides the clear action even if other selected workspaces have one.🛠️ Suggested fix
Menu(String(localized: "contextMenu.workspaceIcon", defaultValue: "Workspace Icon")) { - if tab.customIconPath != nil { + let hasCustomIconTarget = targetIds.contains { workspaceId in + tabManager.tabs.first(where: { $0.id == workspaceId })?.customIconPath != nil + } + if hasCustomIconTarget { Button { applyTabIcon(nil, targetIds: targetIds) } label: {🤖 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 13229 - 13236, The "Clear Icon" menu item visibility currently checks only tab.customIconPath but should check the selected/targeted workspaces (targetIds); change the condition to show the button when any workspace among targetIds has a customIconPath. Implement this by replacing the if tab.customIconPath != nil check with a predicate over targetIds (for example: targetIds.contains { id in tabsById[id]?.customIconPath != nil } or a small helper like hasCustomIcon(in: targetIds)) and keep the call to applyTabIcon(nil, targetIds: targetIds) as-is.
🤖 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/ContentView.swift`:
- Around line 12995-12997: The onChange handler on the settings snapshot is too
broad: it always calls refreshWorkspaceIconDetection(force: true) even when
unrelated fields change; update the onChange closure that observes settings
(SidebarTabItemSettingsSnapshot) to compare the previous and new
autoDetectWorkspaceIcon value and only call refreshWorkspaceIconDetection(force:
true) when that boolean actually changed, while still calling
refreshWorkspaceSnapshot(force: true) for any change. Locate the onChange usage
that references settings and the functions refreshWorkspaceIconDetection and
refreshWorkspaceSnapshot to implement a simple diff check (or capture old/new
settings) against settings.autoDetectWorkspaceIcon before invoking the forced
icon refresh.
- Around line 14035-14053: The promptEmojiIcon flow currently returns silently
when WorkspaceIconValue.normalizedStorageValue(...) fails; instead, present a
small localized warning and allow the user to correct input. Change
promptEmojiIcon so that if normalizedStorageValue returns nil you show an
NSAlert (or update the current alert) with a localized invalid-emoji
title/message (e.g. keys like "alert.emojiIcon.invalid.title" /
"alert.emojiIcon.invalid.message"), then re-present the emoji prompt (e.g. call
promptEmojiIcon(targetIds) again or loop) so the user can retry, and only call
applyTabIcon when normalized is non-nil; reference promptEmojiIcon,
WorkspaceIconValue.normalizedStorageValue(...) and applyTabIcon in your changes.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 13229-13236: The "Clear Icon" menu item visibility currently
checks only tab.customIconPath but should check the selected/targeted workspaces
(targetIds); change the condition to show the button when any workspace among
targetIds has a customIconPath. Implement this by replacing the if
tab.customIconPath != nil check with a predicate over targetIds (for example:
targetIds.contains { id in tabsById[id]?.customIconPath != nil } or a small
helper like hasCustomIcon(in: targetIds)) and keep the call to applyTabIcon(nil,
targetIds: targetIds) as-is.
In `@Sources/Workspace.swift`:
- Around line 8738-8742: Current code awaits a Task.detached() inside
workspaceIconDetectionTask but only cancels the wrapper task
(workspaceIconDetectionTask?.cancel()), which does not cancel the detached
detector; fix by keeping and cancelling the detached Task handle explicitly:
when creating the detached operation (the Task.detached that runs
WorkspaceIconDetectionResult.detect(in:)), assign that Task to a persistent
variable (e.g., workspaceIconDetectionDetachedTask) before awaiting its .value,
cancel any existing detached handle when starting a new scan or when disabling
auto-detect, and ensure places that currently call
workspaceIconDetectionTask?.cancel() also cancel the detached handle so the
long-running detect(in:) is stopped promptly.
🪄 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: 5defa880-72b6-48d1-97ef-ddb17a163514
📒 Files selected for processing (6)
Sources/ContentView.swiftSources/Sidebar/SidebarAppearanceSupport.swiftSources/Sidebar/SidebarWorkspaceSnapshotRefreshPolicy.swiftSources/Workspace.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swiftcmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
1 issue found across 6 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="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:4620">
P2: `--icon` accepts option-like tokens as values, so a missing icon can silently consume the next flag instead of failing fast.
(Based on your team's feedback about rejecting option-like flag values.) [FEEDBACK_USED]</violation>
</file>
<file name="cmuxTests/WorkspaceUnitTests.swift">
<violation number="1" location="cmuxTests/WorkspaceUnitTests.swift:270">
P2: This test has a race: the initial forced icon refresh is launched before creating `favicon.png`, so async scheduling can make the first refresh detect the icon and mask whether the final forced refresh is actually required.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1df9ec5. Configure here.
lawrencecchen
added
the
stale-revisit
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
2 participants
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.
Revives workspace sidebar icons from #1655 for #1652.
Summary:
emoji:<emoji>and image file path support.workspace.actionset_iconandclear_icon, pluscmux workspace-action --action set-icon --icon <path|emoji:🚀>.Validation:
jq empty Resources/Localizable.xcstringsgit diff --checkxcodebuild -quiet -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS,arch=arm64' -derivedDataPath /tmp/cmux-iconsv2-emoji -resultBundlePath /tmp/cmux-iconsv2-emoji.xcresult -only-testing:cmuxTests/WorkspaceIconTests testxcodebuild -quiet -project cmux.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS,arch=arm64' -derivedDataPath /tmp/cmux-iconsv2-final-build build./scripts/reload.sh --tag iconsv2Cloud recording:
Computer Use server error -10005: cgWindowNotFound./tmp/cloud-mac-cua-ssh-cloud-mac-26075390383-20260519040940/state-failure-screen.pngand/tmp/workspace-icons-cloud-screenshot.png/screenshot.png.Dogfood tag:
iconsv2