Repository navigation
fix: keep selection highlight uniform across colored and uncolored workspaces (#3308) - #3310
Conversation
Custom workspace colors currently bleed into the selected sidebar row, so this regression asserts that selected colored rows use the same background as uncolored selected rows while unselected colored rows retain their assigned fill. Constraint: Direct local xcodebuild is prohibited for this branch; unit verification must run through GitHub Actions or the approved reload script.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check -- cmuxTests/SidebarWidthPolicyTests.swift\nNot-tested: Unit test execution before the fix is pending GitHub Actions red-run verification
Workspace color remains an identity treatment for unselected sidebar rows, but selected rows now share the same configured selection background as uncolored workspaces. Removing the selected-state custom color branch restores a consistent focus cue across the sidebar. Constraint: This intentionally reverses issue #2565 in favor of issue #3308.\nRejected: Keep darkened tab colors for selected rows | still makes the selection cue inconsistent across colored and uncolored workspaces.\nConfidence: high\nScope-risk: narrow\nTested: git diff --check -- Sources/Sidebar/SidebarAppearanceSupport.swift cmuxTests/SidebarWidthPolicyTests.swift\nNot-tested: Local unit tests not run because direct xcodebuild is prohibited; GitHub Actions unit run is used for verification
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryReverts the selected-state custom-colour behaviour introduced in #2565: Confidence Score: 4/5Safe to merge; the code change is correct and pre-existing tests in WorkspaceUnitTests.swift already cover both indicator styles. The implementation change is a clean, surgical removal with no logic surprises, and is backed by existing integration tests. Two P2-only findings (test-file placement and missing dark-mode coverage in one test case) prevent a perfect 5. cmuxTests/SidebarWidthPolicyTests.swift — new test class could be relocated and dark-mode coverage added to the configured-hex test. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sidebarWorkspaceRowBackgroundStyle] --> B{isActive?}
B -- yes --> C[sidebarSelectedWorkspaceBackgroundNSColor]
C --> D{sidebarSelectionColorHex set?}
D -- yes --> E[Return configured hex colour]
D -- no --> F[Return cmuxAccentNSColor for scheme]
B -- no, solidFill --> G{customColorHex set?}
G -- yes --> H[WorkspaceTabColorSettings.displayNSColor]
H --> I[Return custom colour at opacity 0.7 / 0.35]
G -- no --> J{isMultiSelected?}
J -- yes --> K[Return accent at 0.25 opacity]
J -- no --> L[Return .clear]
B -- no, leftRail --> M{isMultiSelected?}
M -- yes --> N[Return accent at 0.25 opacity]
M -- no --> O[Return .clear]
Reviews (1): Last reviewed commit: "Keep colored workspace selection highlig..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func testSelectedColoredWorkspaceUsesConfiguredSelectionBackground() { | ||
| let selectionHex = "#123456" | ||
| let coloredSelected = sidebarWorkspaceRowBackgroundStyle( | ||
| activeTabIndicatorStyle: .solidFill, | ||
| isActive: true, | ||
| isMultiSelected: false, | ||
| customColorHex: "#E85D75", | ||
| colorScheme: .light, | ||
| sidebarSelectionColorHex: selectionHex | ||
| ) | ||
| let standardSelected = sidebarWorkspaceRowBackgroundStyle( | ||
| activeTabIndicatorStyle: .solidFill, | ||
| isActive: true, | ||
| isMultiSelected: false, | ||
| customColorHex: nil, | ||
| colorScheme: .light, | ||
| sidebarSelectionColorHex: selectionHex | ||
| ) | ||
|
|
||
| XCTAssertEqual(coloredSelected.opacity, 1, accuracy: 0.001) | ||
| assertColor(coloredSelected.color, equals: standardSelected.color) | ||
| assertColor(coloredSelected.color, equals: NSColor(hex: selectionHex)) | ||
| } |
There was a problem hiding this comment.
testSelectedColoredWorkspaceUsesConfiguredSelectionBackground passes colorScheme: .light exclusively, whereas the test above it iterates over both schemes in a loop. Because sidebarSelectedWorkspaceBackgroundNSColor uses cmuxAccentNSColor(for:) — which returns different sRGB values in light vs. dark — a sidebarSelectionColorHex-override test in dark mode would exercise the same code path but confirm there's no colour-scheme-specific conditional that could accidentally re-introduce the custom-colour branch for dark mode. Worth adding .dark coverage here to match the companion test.
| } | ||
| } | ||
|
|
||
| final class SidebarWorkspaceSelectionColorTests: XCTestCase { |
There was a problem hiding this comment.
SidebarWorkspaceSelectionColorTests is added to SidebarWidthPolicyTests.swift, a file that tests resize-range arithmetic and clamping logic. The existing parallel coverage lives in WorkspaceUnitTests.swift alongside SidebarSelectedWorkspaceColorTests. Moving the new class there (or into a new SidebarSelectionColorTests.swift) would keep test organisation consistent and make it easier to find all selection-colour tests in one place.
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!
📝 WalkthroughWalkthroughRemoves custom hex color handling from sidebar selection background logic, ensuring selected workspace rows always display the standard system selection color. Custom colors now only apply to unselected rows, eliminating the custom hex resolution path and related luminance/contrast utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/SidebarWidthPolicyTests.swift (1)
68-129: Expand the regression matrix to include.leftRailstyle.The new tests currently assert only
.solidFill. Please add.leftRailcoverage too, so selection-color uniformity is guarded across both sidebar indicator styles.Suggested test matrix tweak
- for colorScheme in [ColorScheme.light, .dark] { + for colorScheme in [ColorScheme.light, .dark] { + for style in [SidebarActiveTabIndicatorStyle.solidFill, .leftRail] { let coloredSelected = sidebarWorkspaceRowBackgroundStyle( - activeTabIndicatorStyle: .solidFill, + activeTabIndicatorStyle: style, isActive: true, isMultiSelected: false, customColorHex: "#E85D75", colorScheme: colorScheme, sidebarSelectionColorHex: nil ) let standardSelected = sidebarWorkspaceRowBackgroundStyle( - activeTabIndicatorStyle: .solidFill, + activeTabIndicatorStyle: style, isActive: true, isMultiSelected: false, customColorHex: nil, colorScheme: colorScheme, sidebarSelectionColorHex: nil ) @@ let unselectedColored = sidebarWorkspaceRowBackgroundStyle( - activeTabIndicatorStyle: .solidFill, + activeTabIndicatorStyle: style, isActive: false, isMultiSelected: false, customColorHex: "#E85D75", colorScheme: colorScheme, sidebarSelectionColorHex: nil ) - XCTAssertEqual(unselectedColored.opacity, 0.7, accuracy: 0.001) + if style == .solidFill { + XCTAssertEqual(unselectedColored.opacity, 0.7, accuracy: 0.001) + } XCTAssertFalse( colorsAreEqual(coloredSelected.color, unselectedColored.color), "Selected row should use the standard selection background, not the workspace tab color" ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/SidebarWidthPolicyTests.swift` around lines 68 - 129, Tests only cover activeTabIndicatorStyle .solidFill; expand the matrix to also test .leftRail so selection-color behavior is validated for both indicator styles. Update both testSelectedColoredWorkspaceUsesStandardSelectionBackgroundInLightAndDark and testSelectedColoredWorkspaceUsesConfiguredSelectionBackground to either loop over [.solidFill, .leftRail] for the activeTabIndicatorStyle parameter passed to sidebarWorkspaceRowBackgroundStyle or duplicate assertions for .leftRail, ensuring you call sidebarWorkspaceRowBackgroundStyle with activeTabIndicatorStyle: .leftRail and verify the same opacity and color equality checks as currently done for .solidFill.
🤖 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/SidebarWidthPolicyTests.swift`:
- Around line 68-129: Tests only cover activeTabIndicatorStyle .solidFill;
expand the matrix to also test .leftRail so selection-color behavior is
validated for both indicator styles. Update both
testSelectedColoredWorkspaceUsesStandardSelectionBackgroundInLightAndDark and
testSelectedColoredWorkspaceUsesConfiguredSelectionBackground to either loop
over [.solidFill, .leftRail] for the activeTabIndicatorStyle parameter passed to
sidebarWorkspaceRowBackgroundStyle or duplicate assertions for .leftRail,
ensuring you call sidebarWorkspaceRowBackgroundStyle with
activeTabIndicatorStyle: .leftRail and verify the same opacity and color
equality checks as currently done for .solidFill.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24a7dbc7-b593-4ca7-bf77-5877e4d4e0d8
📒 Files selected for processing (2)
Sources/Sidebar/SidebarAppearanceSupport.swiftcmuxTests/SidebarWidthPolicyTests.swift
💤 Files with no reviewable changes (1)
- Sources/Sidebar/SidebarAppearanceSupport.swift
This branch was successfully deployed
1 participant
Summary
Closes #3308.
This reverses #2565: workspace tab colors still identify unselected sidebar rows, but selected rows now always use the same standard selection background as uncolored workspaces.
Changes
SidebarWorkspaceSelectionColorTests.sidebarWorkspaceRowBackgroundStyle.Verification
xcodebuildSwift harness:selected colored row did not use standard selection background../scripts/reload.sh --tag issue-3308-uniform-selection-color --launchsucceeded and launched the tagged dev app.Manual Verification
ColoredandPlainworkspaces.#C0392BtoColoredviaworkspace-action set-color.Colored: selected row used the standard blue selection highlight, with only the color rail retaining the assigned red.Plain: selected row used the same standard blue selection highlight, and the unselectedColoredrow retained its assigned red rail.Note
Low Risk
UI-only color selection behavior change with limited scope, plus new unit tests; no security or data-handling impact.
Overview
Selected sidebar rows no longer derive their background from per-workspace custom tab colors; selection now always uses the standard configured selection color (or the default accent) regardless of workspace coloring.
This removes the contrast-adjustment/custom selected-background path from
sidebarSelectedWorkspaceBackgroundNSColor/sidebarWorkspaceRowBackgroundStyleand addsSidebarWorkspaceSelectionColorTeststo lock in the behavior across light/dark modes and with an explicitsidebarSelectionColorHex.Reviewed by Cursor Bugbot for commit 64e098d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Make selected sidebar rows use the same selection highlight across colored and uncolored workspaces. Unselected rows keep their workspace color for identity.
sidebarWorkspaceRowBackgroundStyle; selected rows now use the standard selection background or the configuredsidebarSelectionColorHexin both light and dark modes.SidebarWorkspaceSelectionColorTeststo verify uniform selection for colored workspaces and preservation of custom color on unselected rows.Written for commit 64e098d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests