Issue 3081 workspace color left rail - #3082
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughThe changes introduce an explicit leading rail visual indicator for custom-colored workspace rows in the sidebar when using the leftRail indicator style. The rail computation and background color styling logic are refactored to standardize on selected background colors, with tests validating the new rail color and background behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Greptile SummaryThis PR changes how workspace custom colors are displayed in the sidebar's Confidence Score: 5/5Safe to merge — logic is correct, well-tested, and only P2 style notes remain. All findings are P2 (dead computation and a local binding name that shadows an outer property). Neither affects correctness, and the three new tests clearly exercise the new code paths. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TabItemView renders row] --> B{activeTabIndicatorStyle}
B -->|leftRail| C{customColorHex set?}
B -->|solidFill| G{isActive?}
C -->|yes| D[sidebarWorkspaceRowExplicitRailNSColor → NSColor]
C -->|no| E[explicitRailColor = nil / showsLeadingRail = false]
D --> F[Render 3px Capsule overlay at leading edge]
E --> NoRail[No rail rendered]
F --> BG_LR{isActive?}
BG_LR -->|yes| SelBG[background = selectedBackground, opacity 1]
BG_LR -->|no| ClearBG[background = clear]
G -->|yes| SelBG2[background = selectedBackground, opacity 1]
G -->|no| H{customColorHex set?}
H -->|yes| CustomBG[background = customColor, opacity 0.35/0.7]
H -->|no| I{isMultiSelected?}
I -->|yes| AccentBG[background = accentBackground, opacity 0.25]
I -->|no| ClearBG2[background = clear]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let railColor = sidebarWorkspaceRowExplicitRailNSColor( | ||
| activeTabIndicatorStyle: activeTabIndicatorStyle, | ||
| customColorHex: workspaceSnapshot.customColorHex, | ||
| colorScheme: colorScheme | ||
| ) else { | ||
| return nil | ||
| } | ||
| return Color(nsColor: railColor).opacity(0.95) | ||
| } |
There was a problem hiding this comment.
railColor property
Inside explicitRailColor, the guard let railColor = ... binding is the same name as the struct-level railColor: Color computed property. Swift resolves this correctly (the guard's binding is NSColor, the outer property is Color), but it silently shadows the outer name within the guard's scope, which can confuse readers. Consider renaming the local binding to nsColor or resolvedNSColor for clarity.
| guard let railColor = sidebarWorkspaceRowExplicitRailNSColor( | |
| activeTabIndicatorStyle: activeTabIndicatorStyle, | |
| customColorHex: workspaceSnapshot.customColorHex, | |
| colorScheme: colorScheme | |
| ) else { | |
| return nil | |
| } | |
| return Color(nsColor: railColor).opacity(0.95) | |
| } | |
| guard let nsColor = sidebarWorkspaceRowExplicitRailNSColor( | |
| activeTabIndicatorStyle: activeTabIndicatorStyle, | |
| customColorHex: workspaceSnapshot.customColorHex, | |
| colorScheme: colorScheme | |
| ) else { | |
| return nil | |
| } | |
| return Color(nsColor: nsColor).opacity(0.95) |
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
13515-13524: Make the decorative rail non-interactive.The rail is visual-only; disabling hit testing/accessibility avoids any chance of stealing clicks, drags, or VoiceOver focus from the row. Please verify selecting/dragging from the rail area still works.
Suggested hardening
.overlay(alignment: .leading) { if showsLeadingRail { Capsule(style: .continuous) .fill(railColor) .frame(width: 3) .padding(.leading, 4) .padding(.vertical, 5) .offset(x: -1) + .allowsHitTesting(false) + .accessibilityHidden(true) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 13515 - 13524, The decorative leading rail inside the overlay (the block conditioned on showsLeadingRail) should be made non-interactive so it doesn't steal pointer or VoiceOver focus; update the Capsule created in the overlay (the view using railColor) to disable interactions and accessibility by applying SwiftUI modifiers such as allowsHitTesting(false) and accessibilityHidden(true) (or accessibility(hidden: true)) to that Capsule inside the overlay closure so row selection/dragging still works when started from the rail area.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 13515-13524: The decorative leading rail inside the overlay (the
block conditioned on showsLeadingRail) should be made non-interactive so it
doesn't steal pointer or VoiceOver focus; update the Capsule created in the
overlay (the view using railColor) to disable interactions and accessibility by
applying SwiftUI modifiers such as allowsHitTesting(false) and
accessibilityHidden(true) (or accessibility(hidden: true)) to that Capsule
inside the overlay closure so row selection/dragging still works when started
from the rail area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d8573d2-7ccb-419b-9a55-99186b3a52e2
📒 Files selected for processing (2)
Sources/ContentView.swiftcmuxTests/WorkspaceUnitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 b32086f. Configure here.
| ) | ||
|
|
||
| XCTAssertNotNil(railColor) | ||
| XCTAssertEqual(railColor?.hexString(), "#C0392B") |
There was a problem hiding this comment.
Test expects raw hex but function applies color brightening
Medium Severity
The test testLeftRailResolvesExplicitRailColorForCustomColoredWorkspaceRow asserts that railColor?.hexString() equals "#C0392B", but sidebarWorkspaceRowExplicitRailNSColor calls WorkspaceTabColorSettings.displayNSColor with forceBright: true. This triggers brightenedForDarkAppearance, which boosts brightness and saturation of the input color, producing a hex value different from the raw input "#C0392B". This test will fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b32086f. Configure here.
| if let customBackground { | ||
| return SidebarWorkspaceRowBackgroundStyle( | ||
| color: customBackground, | ||
| opacity: isMultiSelected ? 0.35 : 0.7 |
There was a problem hiding this comment.
Unused customBackground computation in leftRail branch
Low Severity
The customBackground variable is computed for all activeTabIndicatorStyle values but is now only consumed in the solidFill branch. In the leftRail branch it's unused dead computation. Additionally, the forceBright: activeTabIndicatorStyle == .leftRail expression is misleading — it evaluates to true only in the path that never reads customBackground, and is always false when the variable is actually used, making it functionally equivalent to forceBright: false.
Reviewed by Cursor Bugbot for commit b32086f. Configure here.
* Add regression test for workspace color left rail * Fix workspace color left rail rendering * Keep selected workspace fill above custom color
This branch was successfully deployed
1 participant
Summary
Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Note
Low Risk
Low risk UI-only change to sidebar workspace row rendering, with updated unit tests covering the new left-rail vs solid-fill behavior.
Overview
Updates sidebar workspace row styling so active rows no longer use the workspace’s custom color as the selection background in either
leftRailorsolidFillmode; selection now always usessidebarSelectedWorkspaceBackgroundNSColor.For
leftRail, custom workspace colors are now rendered as an explicit leading rail (newsidebarWorkspaceRowExplicitRailNSColor+ a leadingCapsuleoverlay), while inactive custom-colored rows remain transparent. Unit tests are updated/added to lock in the new selection/background vs rail behavior.Reviewed by Cursor Bugbot for commit b32086f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Summary by cubic
Updated the sidebar to show workspace colors as a left-rail indicator and made the selected row background consistent across styles. Addresses Linear 3081 by removing custom-color fills for active rows and adding a clear color rail in left-rail mode.
New Features
leftRailusing the workspace’s custom color.Bug Fixes
sidebarSelectedWorkspaceBackgroundNSColorin bothleftRailandsolidFill.leftRail, inactive custom-colored rows stay transparent; insolidFill, they use the custom color at 0.7 opacity. Unit tests added to lock this behavior.Written for commit b32086f. Summary will update on new commits.