Repository navigation
Fall back for blank sidebar icons - #5037
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces ChangesIcon symbol normalization and integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge — changes are confined to icon normalisation on the main actor, with no data-loss, auth, or persistence side effects. All call sites are on @MainActor-isolated types (TabManager, Workspace) or SwiftUI views, so the new @mainactor cache is accessed correctly throughout. The one redundant re-validation in the view body is harmless (cache hit every time). The API response fix and three new unit tests give solid confidence the feature behaves as intended. No files require special attention; SidebarWorkspaceGroupHeaderView.swift has a minor redundancy worth tidying. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Raw icon string] --> B{RenderableSystemSymbol.normalized}
B --> C{trimmed?}
C -- nil/empty --> D[return nil]
C -- non-empty --> E{isRenderable?\nNSImage check + cache}
E -- false --> D
E -- true --> F[return trimmed string]
F --> G1[setWorkspaceGroupIcon\nstores valid symbol or nil]
F --> G2[Workspace.swift\nBonsplit tab create/update]
F --> G3[SidebarWorkspaceGroupHeaderView\ndisplayedIconSymbol]
D --> H1[group.iconSymbol = nil\nfallback → folder.fill]
D --> H2[tab icon fallback → doc.text]
Reviews (4): Last reviewed commit: "Use main actor for symbol renderability ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func normalized(_ raw: String?) -> String? { | ||
| RenderableSystemSymbol.trimmed(raw) | ||
| } |
There was a problem hiding this comment.
WorkspaceGroupIconSymbol.normalized skips the renderability check
normalized calls RenderableSystemSymbol.trimmed — which only strips whitespace — not RenderableSystemSymbol.normalized, which is the method that also calls isRenderable. As a result, setWorkspaceGroupIcon(groupId:symbol:) stores any non-blank string (e.g. "not.an.sf.symbol") directly into the model without validating that it resolves to a real SF Symbol. The bad value persists until resolved() discards it at render time via RenderableSystemSymbol.normalized, but the authoritative iconSymbol property on the group carries stale invalid data. Rename the wrapper or change its body to RenderableSystemSymbol.normalized(raw) so storage and display share the same validation semantics.
There was a problem hiding this comment.
Fixed by making WorkspaceGroupIconSymbol.normalized use the same renderability validation as the render path, and added a regression test that invalid group symbols are dropped before storage.
— Claude Code
| enum RenderableSystemSymbol { | ||
| static func trimmed(_ raw: String?) -> String? { | ||
| guard let trimmed = raw?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !trimmed.isEmpty else { | ||
| return nil | ||
| } | ||
| return trimmed | ||
| } | ||
|
|
||
| static func normalized(_ raw: String?) -> String? { | ||
| guard let trimmed = trimmed(raw), | ||
| isRenderable(trimmed) else { | ||
| return nil | ||
| } | ||
| return trimmed | ||
| } | ||
|
|
||
| static func isRenderable(_ symbol: String) -> Bool { | ||
| NSImage(systemSymbolName: symbol, accessibilityDescription: nil) != nil | ||
| } | ||
| } | ||
|
|
||
| enum WorkspaceGroupIconSymbol { | ||
| static let defaultSymbol = "folder.fill" | ||
|
|
||
| static func normalized(_ raw: String?) -> String? { | ||
| RenderableSystemSymbol.trimmed(raw) | ||
| } | ||
|
|
||
| static func resolved(explicit: String?, configured: String?) -> String { | ||
| for candidate in [explicit, configured] { | ||
| guard let normalized = RenderableSystemSymbol.normalized(candidate) else { continue } | ||
| return normalized | ||
| } | ||
| return defaultSymbol | ||
| } | ||
| } | ||
|
|
||
| enum SurfaceTabIconSymbol { | ||
| static let defaultSymbol = "doc.text" | ||
|
|
||
| static func resolved(_ raw: String?, fallback: String = defaultSymbol) -> String { | ||
| RenderableSystemSymbol.normalized(raw) | ||
| ?? RenderableSystemSymbol.normalized(fallback) | ||
| ?? defaultSymbol | ||
| } | ||
| } |
There was a problem hiding this comment.
RenderableSystemSymbol, WorkspaceGroupIconSymbol, and SurfaceTabIconSymbol are referenced from TabManager.swift, Workspace.swift, VerticalTabsSidebar+WorkspaceGroups.swift, and FilePreviewPanel.swift — yet they live in SidebarWorkspaceGroupHeaderView.swift, which is a SwiftUI view file. This mixes shared, independently-testable icon-validation logic into a UI component file, making the dependency graph read backwards (the model layer importing from a view file). A dedicated Sources/IconSymbol.swift (or similar) would give these helpers the right home without adding file count.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func setWorkspaceGroupIcon(groupId: UUID, symbol: String?) { | ||
| let normalized = WorkspaceGroupIconSymbol.normalized(symbol) | ||
| guard let index = workspaceGroups.firstIndex(where: { $0.id == groupId }) else { return } | ||
| guard workspaceGroups[index].iconSymbol != symbol else { return } | ||
| workspaceGroups[index].iconSymbol = symbol | ||
| guard workspaceGroups[index].iconSymbol != normalized else { return } | ||
| workspaceGroups[index].iconSymbol = normalized | ||
| } |
There was a problem hiding this comment.
setWorkspaceGroupIcon now silently rejects unrenderable symbols (storing nil instead), but the only existing caller — v2WorkspaceGroupSetIcon in TerminalController.swift — still echoes its own pre-normalization normalized variable in the success response body ("icon_symbol": v2OrNull(normalized)). A caller that sends "not.an.sf.symbol" receives a success response claiming that exact string was applied, but the group actually stores nil and displays folder.fill. Before this PR that was harmless because every symbol was accepted; now the response can misrepresent the stored state for any non-renderable input. setWorkspaceGroupIcon should either return the effective stored symbol so callers can reflect it, or the TerminalController call-site should run WorkspaceGroupIconSymbol.normalized before composing its response.
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/VerticalTabsSidebar`+WorkspaceGroups.swift:
- Around line 16-19: RenderableSystemSymbol.resolvedWorkspaceGroupIcon is
calling isRenderable(_:) on each candidate every render, which repeatedly
constructs NSImage(systemSymbolName:) — add memoization inside
RenderableSystemSymbol (e.g., a static [String: Bool] cache keyed by the symbol
name) and have isRenderable(_) consult and populate that cache instead of always
instantiating NSImage; update resolvedWorkspaceGroupIcon to use the cached
result and ensure simple thread-safety (DispatchQueue or atomic access) for
cache reads/writes and an option to invalidate if needed.
🪄 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: 810b8332-5fb8-491d-bc8f-cafab0bc05ba
📒 Files selected for processing (10)
Sources/Panels/FilePreviewPanel.swiftSources/Panels/PanelContentView.swiftSources/RenderableSystemSymbol.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceGroupTests.swift
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/RenderableSystemSymbol.swift`:
- Around line 3-20: The current RenderableSystemSymbolCache uses NSLock and
`@unchecked` Sendable; rewrite it as an actor named RenderableSystemSymbolCache
that owns the values dictionary and exposes an async method value(for:compute:)
(or value(for:) that accepts a synchronous closure but is called via await) so
locking is handled by actor isolation, remove `@unchecked` Sendable and NSLock,
and update all call sites (e.g., isRenderable(_:), any callers that invoke
RenderableSystemSymbolCache.value(for:compute:)) to await the new actor method,
preserving the same semantics of caching the computed Bool result.
🪄 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: 704096b1-976f-4110-ba41-f0fbb034022b
📒 Files selected for processing (1)
Sources/RenderableSystemSymbol.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Stale CodeRabbit review. The reported NSLock cache issue was fixed in 70c7261 with a MainActor-isolated cache; the inline thread is resolved and the current CodeRabbit check is passing.
This branch was successfully deployed
1 participant
Summary
folder.fill.doc.text.Testing
./scripts/reload.sh --tag iconfix --swift-frontend-workaroundDogfood
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
UI-only icon normalization and layout tweaks; no auth, data, or security-sensitive paths.
Overview
Adds
RenderableSystemSymbolto trim names, check SF Symbol availability viaNSImage, cache results, and fall back tofolder.fill(workspace groups) ordoc.text(surface/file-preview tabs).Workspace groups: Sidebar headers and config resolution use the helper instead of raw strings;
setWorkspaceGroupIconstores only renderable symbols (ornil) and returns the stored value; the v2 API echoes that normalizedicon_symbol.File preview tabs: Bonsplit create/update paths, drag transfer payloads, and live icon updates all publish resolved tab icons so invalid or empty names don’t render blank.
UI polish:
PanelFilePathHeaderuses a fixed 14×14 icon slot and tighter horizontal padding; group header icons are semibold in a 14×14 frame and hidden from accessibility as decorative.Tests: Coverage for resolution, invalid symbol rejection, and surface tab fallbacks.
Reviewed by Cursor Bugbot for commit 62ca9a8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes disappearing sidebar and file‑preview icons by validating SF Symbols with a cached main‑actor renderability check and falling back to safe defaults. Aligns file‑path header icon size/spacing with tabs; invalid or blank names resolve to "folder.fill" (groups) or "doc.text" (tabs).
RenderableSystemSymboland use it for all icon resolution.nil; API echoes the stored value; fallback to "folder.fill".Written for commit 70c7261. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores