Repository navigation
Show and customize workspace group icons - #5031
lawrencecchen wants to merge 27 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughPR adds a complete workspace group icon picker feature. Users can set custom SF Symbol or single-emoji icons via context menu. Implementation includes emoji-aware icon normalization, a searchable picker UI with system symbol and emoji catalogs, header integration with popover, sidebar wiring, multilingual localization, and test coverage. ChangesSet Workspace Group Icon
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@docs/workspace-groups.md`:
- Around line 69-70: Document that the optional flags for the CLI commands "cmux
workspace-group set-color" (--hex) and "cmux workspace-group set-icon"
(--symbol) will clear the custom value when omitted; update the docs after the
listed commands to add a short sentence such as: omitting --hex or --symbol
clears the custom color or icon and reverts to the default, so users know
omission does not produce an error but resets the value.
In `@Sources/SidebarWorkspaceGroupDialogs.swift`:
- Around line 71-78: The accessory text field isn't getting focused and its
contents selected when the icon prompt is re-shown; instead of only setting
alert.window.initialFirstResponder, call alert.window.makeFirstResponder(input)
and then invoke input.selectText(nil) (the same pattern used in the rename flow)
after attaching the accessory view so the field is focused and its text selected
for easy overwrite; locate the code that configures the NSTextField (`input`)
and the NSAlert (`alert`) and replace or augment the initialFirstResponder
assignment with the makeFirstResponder + selectText sequence.
- Around line 44-120: The prototype resource catalog is missing the new
localization keys used by presentSidebarWorkspaceGroupIconPrompt and
presentSidebarWorkspaceGroupInvalidIconAlert; add entries for
workspaceGroup.icon.title, workspaceGroup.icon.message,
workspaceGroup.icon.confirm, workspaceGroup.icon.clear,
workspaceGroup.icon.placeholder, workspaceGroup.icon.invalid.title,
workspaceGroup.icon.invalid.message, and workspaceGroup.contextMenu.setIcon to
Prototypes/SettingsShellLab/Sources/SettingsShellLab/Resources/Localizable.xcstrings
for the supported locales (at least en and ja) so the prototype target can
resolve the strings referenced by those functions, or remove/guard usages in
those functions for that target if you prefer not to add the keys.
🪄 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: ace757e3-ef03-41db-a04f-9996a154bbd3
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/SidebarWorkspaceGroupDialogs.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/WorkspaceGroupTests.swiftdocs/workspace-groups.md
| cmux workspace-group set-color <group-id> [--hex #RRGGBB] | ||
| cmux workspace-group set-icon <group-id> [--symbol <sf-symbol>] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider documenting the behavior when optional flags are omitted.
The square brackets indicate --hex and --symbol are optional, but it's not explicit whether omitting them clears the respective value or produces an error. Adding a brief note (e.g., "Omit the flag to clear the custom value") would improve clarity for CLI users.
📝 Example clarification
After line 70, you could add:
Omitting `--hex` or `--symbol` clears the custom color or icon, respectively, reverting to the default.🤖 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 `@docs/workspace-groups.md` around lines 69 - 70, Document that the optional
flags for the CLI commands "cmux workspace-group set-color" (--hex) and "cmux
workspace-group set-icon" (--symbol) will clear the custom value when omitted;
update the docs after the listed commands to add a short sentence such as:
omitting --hex or --symbol clears the custom color or icon and reverts to the
default, so users know omission does not produce an error but resets the value.
Greptile SummaryThis PR adds workspace group icon customization: headers now render emoji or SF Symbols (with
Confidence Score: 5/5The change is safe to merge: the emoji/symbol validation path is well-tested, all new user-facing strings carry complete translations, and the focus-policy fix is narrowly scoped to NSText in overlay windows with clear regression tests. The baked emoji catalog eliminates the disk I/O concern flagged in the previous review. The renderability cache is properly capped. Localization is complete across all 20 supported locales. The focus-policy change is backed by four targeted tests that cover the #5269 regression path. Two minor style items were flagged but neither affects correctness. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SidebarHeader as SidebarWorkspaceGroupHeaderView
participant Picker as WorkspaceGroupIconPickerView
participant Catalog as WorkspaceGroupEmojiCatalog
participant Symbol as RenderableSystemSymbol
participant TabMgr as TabManager
User->>SidebarHeader: Right-click → "Set Group Icon…"
SidebarHeader->>Picker: Present popover(storedIconSymbol)
Picker->>Symbol: normalizedWorkspaceGroupIcon(currentSymbol)
Symbol-->>Picker: RenderableWorkspaceGroupIcon (.emoji / .systemSymbol)
User->>Picker: Type search query
Picker->>Catalog: search(searchText)
Catalog->>Symbol: normalizedEmoji(rawQuery)
Symbol-->>Catalog: emoji? / nil
Catalog-->>Picker: [emoji…]
Picker->>Symbol: normalizedWorkspaceGroupIcon(searchText)
Symbol->>Symbol: isRenderable(symbol) → NSImage(systemSymbolName:)
Symbol-->>Picker: symbol? / nil
User->>Picker: Select emoji or symbol
Picker->>SidebarHeader: onSelect(symbol?)
SidebarHeader->>TabMgr: setWorkspaceGroupIcon(groupId, symbol)
TabMgr->>Symbol: normalizedWorkspaceGroupIcon(symbol)
Symbol-->>TabMgr: normalized?
TabMgr-->>SidebarHeader: stored icon updated
SidebarHeader->>SidebarHeader: "isIconPickerPresented = false"
Reviews (21): Last reviewed commit: "Scale workspace group header icons with ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let input = NSTextField(frame: NSRect(x: 0, y: 0, width: 240, height: 24)) | ||
| input.stringValue = nextValue | ||
| input.placeholderString = String( | ||
| localized: "workspaceGroup.icon.placeholder", | ||
| defaultValue: "folder.fill" | ||
| ) | ||
| alert.accessoryView = input | ||
| alert.window.initialFirstResponder = input |
There was a problem hiding this comment.
The rename dialog (line 34-37 of the same file) uses DispatchQueue.main.async { makeFirstResponder(input); input.selectText(nil) } queued before runModal() so AppKit honours keyboard focus reliably and the existing text is pre-selected. The icon dialog only sets alert.window.initialFirstResponder = input, which AppKit does not always honour when a modal window is first shown. As a result, the user's cursor lands on the "Set Icon" button rather than the text field, and the current symbol value is not selected — requiring an extra click before they can type.
| private var displayedIconSymbol: String { | ||
| WorkspaceGroupIconSymbol.resolved(explicit: iconSymbol, configured: nil) | ||
| } |
There was a problem hiding this comment.
NSImage allocation in SwiftUI render path
iconSymbol passed to this view is already the fully-resolved, renderable symbol produced by WorkspaceGroupIconSymbol.resolved(explicit:configured:) in VerticalTabsSidebar+WorkspaceGroups.swift. Calling resolved(explicit: iconSymbol, configured: nil) here triggers an additional NSImage(systemSymbolName:) allocation on every render pass that evaluates displayedIconSymbol, for no benefit. The symbol is guaranteed valid at this point and can be used directly.
| private var displayedIconSymbol: String { | |
| WorkspaceGroupIconSymbol.resolved(explicit: iconSymbol, configured: nil) | |
| } | |
| private var displayedIconSymbol: String { | |
| iconSymbol | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| enum WorkspaceGroupIconSymbol { | ||
| static let defaultSymbol = "folder.fill" | ||
|
|
||
| static func normalized(_ raw: String?) -> String? { | ||
| guard let trimmed = raw?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !trimmed.isEmpty else { return nil } | ||
| return trimmed | ||
| } | ||
|
|
||
| static func resolved(explicit: String?, configured: String?) -> String { | ||
| for candidate in [explicit, configured] { | ||
| guard let normalized = normalized(candidate), | ||
| isRenderable(normalized) else { continue } | ||
| return normalized | ||
| } | ||
| return defaultSymbol | ||
| } | ||
|
|
||
| static func isRenderable(_ symbol: String) -> Bool { | ||
| NSImage(systemSymbolName: symbol, accessibilityDescription: nil) != nil | ||
| } | ||
| } |
There was a problem hiding this comment.
WorkspaceGroupIconSymbol is a pure validation/normalization utility with no SwiftUI or AppKit view responsibility, but it is defined inside SidebarWorkspaceGroupHeaderView.swift. It is used by TabManager.swift, SidebarWorkspaceGroupDialogs.swift, and VerticalTabsSidebar+WorkspaceGroups.swift — all non-view call sites. Moving this enum to a dedicated WorkspaceGroupIconSymbol.swift would keep the view file focused on layout and make the utility easier to unit-test in isolation.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/workspace-groups.md (1)
77-95: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding examples for the new appearance commands.
The Examples section demonstrates
create,new-workspace, andlist, but doesn't showset-colororset-iconusage. Adding quick examples would help users understand both setting and clearing behavior.📚 Suggested examples
After line 95, you could add:
Set a custom icon and color for a group: ```bash cmux workspace-group set-icon workspace_group:1 --symbol "ladybug.fill" cmux workspace-group set-color workspace_group:1 --hex "`#7A4FD8`"Clear custom appearance to revert to defaults:
cmux workspace-group set-icon workspace_group:1 cmux workspace-group set-color workspace_group:1</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/workspace-groups.mdaround lines 77 - 95, Add examples for the new
appearance commands to the Examples section: show usage of the CLI commands
workspace-group set-icon and workspace-group set-color with arguments (e.g.,
workspace_group:1 --symbol "ladybug.fill" and workspace_group:1 --hex "#7A4FD8")
and also show how to clear them by calling the same commands without their
optional flags; place these examples near the existing create/new-workspace/list
examples so users can see both setting and clearing behavior.</details> </blockquote></details> </blockquote></details>🤖 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. Outside diff comments: In `@docs/workspace-groups.md`: - Around line 77-95: Add examples for the new appearance commands to the Examples section: show usage of the CLI commands workspace-group set-icon and workspace-group set-color with arguments (e.g., workspace_group:1 --symbol "ladybug.fill" and workspace_group:1 --hex "`#7A4FD8`") and also show how to clear them by calling the same commands without their optional flags; place these examples near the existing create/new-workspace/list examples so users can see both setting and clearing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID:
ac9251e4-f1d5-473b-9a40-93887f9936ff📒 Files selected for processing (1)
docs/workspace-groups.md
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/WorkspaceGroupIconPickerView.swift`:
- Around line 256-273: The plist-loading helpers loadSymbolNames() and
loadSearchTerms() silently return fallbacks when the private SFSymbols files
can't be read; add a one-time diagnostic (e.g., process logger or telemetry
event) inside both failure branches to record that the system plist failed to
load and include the attempted path and error info, and then strengthen the
fallback: when loadSearchTerms() would return [:], build a fallback map from
fallbackSymbolNames to at least the symbol name tokens (so matching() still
finds items via search), and ensure fallbackSymbolNames is used consistently by
loadSymbolNames() and loadSearchTerms() to preserve search UX.
🪄 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: 33965558-6e12-40bb-9994-f03bb391b664
📒 Files selected for processing (9)
Prototypes/SettingsShellLab/Sources/SettingsShellLab/Resources/Localizable.xcstringsResources/Localizable.xcstringsSources/RenderableSystemSymbol.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftSources/WorkspaceGroupIconPickerView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceGroupTests.swift
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/WorkspaceGroupEmojiCatalog.swift">
<violation number="1" location="Sources/WorkspaceGroupEmojiCatalog.swift:14">
P2: Text-based emoji search is non-functional: `localizedCaseInsensitiveContains` on emoji characters only matches if the query is a substring of the emoji's Unicode scalar representation, not the emoji's description name. Typing "rocket" to find 🚀, "folder" to find 📁, or "brain" to find 🧠 returns zero results, making the search field ineffective for its primary purpose.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Stale bot review addressed by later commits; latest CodeRabbit status is passing on 6f6d737.
# Conflicts: # Sources/SidebarWorkspaceGroupHeaderView.swift # cmux.xcodeproj/project.pbxproj
The picker rebuilt the full emoji set by scanning ~15k Unicode scalars on every body evaluation (and again per cell via the pagination onAppear), pegging the main thread while browsing. Build the de-duplicated catalog once into a cached static, load it off the main thread on first open (seeded with a small common set so the grid is instant), and keep the bounded pagination so view materialization stays capped as the catalog grows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The icon picker resolves typed search text through isRenderable, which cached every distinct query in the global renderabilityCache with no eviction, so a long typing session grew process memory without bound. Cap the cache (dropping it wholesale when full, since real symbol names are few and cheap to relookup) and reject pathologically long names before they reach AppKit. Adds a bounded- cache regression test: without the cap, 712 distinct queries exceed the 512 limit; with it, the cache stays capped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cmd+Shift+P (the command palette) now offers "Set Group Icon…" whenever the focused workspace belongs to a group, resolving from either the group's anchor or a child workspace. Running it opens the existing group icon picker inline in the palette (a new .iconPicker mode, modeled on the rename flow), sharing the same setWorkspaceGroupIcon action as the sidebar context menu. Localized en/ja. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match the full locale set used by neighboring command-palette strings and the other workspace-group icon strings in this PR. Title reuses the existing workspaceGroup.contextMenu.setIcon translations verbatim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The picker had no searchable metadata: the catalog was bare emoji characters discovered by a ~15k-scalar Unicode scan at runtime, so typing a name could never find an emoji and the first open paid the scan as latency. Replace it with a baked dataset (Sources/WorkspaceGroupEmojiData.swift, generated by scripts/generate-workspace-group-emoji.swift from Unicode scalar names). Runtime is now a static array with keyword search (exact, then word-prefix, then substring), so "rocket" finds the rocket emoji, loading is instant, and there is no runtime scan. Adds a behavioral search test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… fields Typing in the workspace group icon picker did nothing: keystrokes went to the terminal instead of the search field. Root cause is in the terminal focus policy, not the picker. cmux's keyRepair (and the symmetric applyFirstResponder paths) reclaim first responder for the terminal unless the active first responder passes shouldRespectForeignFirstResponder. That policy applied a window-membership guard (.window === window, added for issue #5269) before checking whether the responder is a text editor. SwiftUI hosts a popover/palette overlay's field editor (_SystemTextFieldFieldEditor, an NSText subclass) in a backing window that differs from the main window even though it is the main window's active first responder, so the guard rejected it and the terminal stole every keystroke. Check `firstResponder is NSText` before the window-membership guard. The #5269 stranding the guard defends against only affects terminal surfaces (plain NSViews, not NSText), so respecting in-flight text editors regardless of their backing window cannot resurrect it; the guard still applies to non-text (right-sidebar/dock) focus owners. Also focus the picker's search field on appear so typing lands immediately. Verified end-to-end via Computer Use: opening the picker and typing "rocket" filters to the rocket emoji with no keyRepair steal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The overlay-text-field exception accepted any NSText before the window guard, including a detached one (window == nil) such as a removed field left as first responder, which would permanently block terminal focus recovery. Require a non-nil backing window: an overlay editor's window is non-nil (just different from the main window), while a stale detached editor is reclaimed (issue #5269). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 60d5496. Configure here.
The previous "non-nil backing window" exception still respected a text editor stranded in an unrelated window, which could reopen the #5269 focus-recovery hole. Walk the editor window's parent chain instead: respect a text editor only when its window is the repair window or a child window it presents. Verified at runtime that a SwiftUI popover's field editor lives in an `_NSPopoverWindow` whose parent is the main window (chain `[popover, main]`), so this respects the overlay case while reclaiming from genuinely unrelated or detached editors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WorkspaceGroupIconPreview hard-coded the glyph point size, so when it replaced the header's Image(systemName:).font(.system(size: metrics.iconFontSize)) the group icon stopped scaling with the configured sidebar font size and changed its default size. Thread the metric-derived iconFontSize through the preview; picker rows keep the default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
Issues
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes first-responder yielding for overlay text fields (regression-sensitive around #5269); icon persistence is otherwise low-impact UI state.
Overview
Adds custom workspace group icons (single emoji or validated SF Symbol) with a shared
WorkspaceGroupIconPickerView: keyword search over a baked emoji catalog, typed symbol resolution, paging in the grid, and clear-to-default. Headers render viaWorkspaceGroupIconPreview/RenderableWorkspaceGroupIcon, withfolder.fillwhen nothing valid is stored;TabManager.setWorkspaceGroupIconnow normalizes throughnormalizedWorkspaceGroupIcon.Entry points: sidebar group header “Set Group Icon…” (popover) and Command Palette
palette.setGroupIconwhenworkspace.inGroupis true (inline picker, palette stays open). Newcommand.setGroupIcon.*andworkspaceGroup.icon.*strings are localized.ForeignFirstResponderPolicyis updated so keystrokes reachNSTextfield editors in the main window or child overlay windows (e.g. SwiftUI popover/command-palette search), without weakening the #5269 rule for stranded responders in unrelated windows. SF Symbol renderability lookup is capped (512 entries, 128-char names) for picker-driven searches.Reviewed by Cursor Bugbot for commit 140bbfd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Show workspace group icons and add a fast, searchable picker to set a custom emoji or SF Symbol from the header menu or the Command Palette. Icons fall back to
folder.fill; input is validated and localized. Header icons now scale with the sidebar font size.New Features
WorkspaceGroupIconPreview/RenderableWorkspaceGroupIconwith afolder.fillfallback; icons scale with the sidebar font size.WorkspaceGroupIconPickerView: keyword emoji search (baked dataset with exact/prefix/substring), typed SF Symbol support, virtualized grid (common set seeded; full catalog loads off the main thread), and “Clear Icon”. Single emoji are stored; invalid or multi-emoji inputs are rejected.TabManager.setWorkspaceGroupIconusesnormalizedWorkspaceGroupIcon.workspace.inGroupis true; opens inline and keeps the palette open. Strings (command.setGroupIcon.*,workspaceGroup.icon.*) localized.Bug Fixes
NSTextin the active window or its child windows, preserving Clicking a pane (terminal or browser) sometimes fails to activate focus — only clicking the tab works #5269. Tests updated.Written for commit 140bbfd. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests