Commit sidebar selection synchronously so rename shortcut targets the focused workspace - #9336
Closed
austinywang wants to merge 6 commits into
Closed
austinywang wants to merge 6 commits into
austinywang wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughSidebar cells now resolve dynamic colors against their effective appearance. Workspace and group-header selections execute synchronously. The selection coalescer and its tests were removed. New tests cover appearance-aware rendering and grouped selection. ChangesSidebar appearance and selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarWorkspaceTableController
participant SelectionAction
User->>SidebarWorkspaceTableController: click group header or child row
SidebarWorkspaceTableController->>SelectionAction: invoke selection action immediately
SelectionAction-->>SidebarWorkspaceTableController: apply selected workspace or group
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
force-pushed
the
issue-9199-rename-shortcut-targets-group
branch
from
August 1, 2026 04:59
2641dc5 to
25f63bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift`:
- Around line 288-297: Remove the test-only properties
dropIndicatorPaintForTesting, titlePaintForTesting, and
headerBackgroundLayerColorForTesting from
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift. Update
cmuxTests/SidebarAppKitRowCellTests.swift lines 957-980 to inspect the supported
view behavior without these production accessors.
- Around line 281-285: Remove the user-provided model.name prefix from the debug
event assembled in the line diagnostic block. Keep the remaining fixed
diagnostic fields and cmuxDebugLog call unchanged, or remove the probe if no
safe fields remain.
🪄 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 Plus
Run ID: 57e34613-039d-4012-a157-c7c80fc3bd7f
📒 Files selected for processing (8)
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftSources/Sidebar/AppKitList/SidebarSelectionCoalescer.swiftSources/Sidebar/AppKitList/SidebarWorkspaceTableController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarAppKitRowCellTests.swiftcmuxTests/SidebarSelectionCoalescerTests.swiftcmuxTests/SidebarWorkspaceTableTests.swift
💤 Files with no reviewable changes (2)
- cmuxTests/SidebarSelectionCoalescerTests.swift
- Sources/Sidebar/AppKitList/SidebarSelectionCoalescer.swift
Comment on lines
+281
to
+285
| var line = "sidebar.groupHeader.titlePaint name=\(model.name.prefix(12)) active=\(activeFlag)" | ||
| line += " ambient=\(ambientName) effective=\(effectiveName)" | ||
| line += " brightness=\(brightnessText) colorAlpha=\(colorAlphaText)" | ||
| line += " fieldAlpha=\(fieldAlphaText) frameW=\(frameWidth)" | ||
| cmuxDebugLog(line) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the raw group name from the debug event.
model.name is user-provided workspace content. Line 281 writes its prefix to the debug record. Keep only fixed diagnostic fields, or remove this probe.
As per coding guidelines, production Swift runtime logs must not expose customer content without explicit private redaction.
Proposed fix
- var line = "sidebar.groupHeader.titlePaint name=\(model.name.prefix(12)) active=\(activeFlag)"
+ var line = "sidebar.groupHeader.titlePaint active=\(activeFlag)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var line = "sidebar.groupHeader.titlePaint name=\(model.name.prefix(12)) active=\(activeFlag)" | |
| line += " ambient=\(ambientName) effective=\(effectiveName)" | |
| line += " brightness=\(brightnessText) colorAlpha=\(colorAlphaText)" | |
| line += " fieldAlpha=\(fieldAlphaText) frameW=\(frameWidth)" | |
| cmuxDebugLog(line) | |
| var line = "sidebar.groupHeader.titlePaint active=\(activeFlag)" | |
| line += " ambient=\(ambientName) effective=\(effectiveName)" | |
| line += " brightness=\(brightnessText) colorAlpha=\(colorAlphaText)" | |
| line += " fieldAlpha=\(fieldAlphaText) frameW=\(frameWidth)" | |
| cmuxDebugLog(line) |
🤖 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/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift` around
lines 281 - 285, Remove the user-provided model.name prefix from the debug event
assembled in the line diagnostic block. Keep the remaining fixed diagnostic
fields and cmuxDebugLog call unchanged, or remove the probe if no safe fields
remain.
Source: Coding guidelines
Comment on lines
288
to
+297
| var dropIndicatorPaintForTesting: (top: Bool, bottom: Bool) { | ||
| (!topDropIndicator.isHidden, !bottomDropIndicator.isHidden) | ||
| } | ||
|
|
||
| var titlePaintForTesting: (text: String, color: NSColor?, alpha: CGFloat) { | ||
| (nameField.stringValue, nameField.textColor, nameField.alphaValue) | ||
| } | ||
|
|
||
| var headerBackgroundLayerColorForTesting: CGColor? { | ||
| backgroundView.layer?.backgroundColor |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the shared test-only paint-state seam.
The DEBUG-only properties in production source exist only to support these tests. Production source must not export paint state solely for cmuxTests.
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift#L288-L297: removedropIndicatorPaintForTesting,titlePaintForTesting, andheaderBackgroundLayerColorForTesting.cmuxTests/SidebarAppKitRowCellTests.swift#L957-L980: inspect supported view behavior after removing the production-only accessors.
As per coding guidelines, production Swift source must not add test-only members. As per path instructions, production Sources code must not add test-only or debug-only seams.
📍 Affects 2 files
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift#L288-L297(this comment)cmuxTests/SidebarAppKitRowCellTests.swift#L957-L980
🤖 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/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift` around
lines 288 - 297, Remove the test-only properties dropIndicatorPaintForTesting,
titlePaintForTesting, and headerBackgroundLayerColorForTesting from
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift. Update
cmuxTests/SidebarAppKitRowCellTests.swift lines 957-980 to inspect the supported
view behavior without these production accessors.
Sources: Coding guidelines, Path instructions
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
force-pushed
the
issue-9199-rename-shortcut-targets-group
branch
from
August 1, 2026 05:10
25f63bb to
c223940
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift (1)
259-290: 🔒 Security & Privacy | 🟠 MajorRemove the temporary title-paint probe before merge.
logTitlePaintProbeis investigation code called from normal model application and optimistic deselection paths. It also writesmodel.name.prefix(12)tocmuxDebugLog. Group names are user-provided workspace content and are not explicitly redacted.Remove the probe and its call sites. If a permanent diagnostic is required, log fixed fields only and use the Sidebar
dloglogger.Minimum privacy fix if the probe remains temporarily
- var line = "sidebar.groupHeader.titlePaint name=\(model.name.prefix(12)) active=\(activeFlag)" + var line = "sidebar.groupHeader.titlePaint active=\(activeFlag)"As per coding guidelines, remove temporary debug probes before merge and do not log customer content without explicit private redaction. Based on learnings, Sidebar diagnostics should use
dlog, notcmuxDebugLog.🤖 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/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift` around lines 259 - 290, Remove the temporary logTitlePaintProbe method and every call to it from the normal model-application and optimistic-deselection paths. Do not log model.name or other user-provided content; if any permanent diagnostic remains, use fixed fields only through the Sidebar dlog logger rather than cmuxDebugLog.Sources: Coding guidelines, Learnings
🤖 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.
Duplicate comments:
In `@Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift`:
- Around line 259-290: Remove the temporary logTitlePaintProbe method and every
call to it from the normal model-application and optimistic-deselection paths.
Do not log model.name or other user-provided content; if any permanent
diagnostic remains, use fixed fields only through the Sidebar dlog logger rather
than cmuxDebugLog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae9ab0a1-da34-4a4d-8c61-1c70aeae1378
📒 Files selected for processing (1)
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift
Contributor
Author
|
Live verification of the disappearing-title mechanism and fix, from the tagged dev build's debug probe (
Startup entries also caught the cell's own 🤖 Generated with Claude Code |
This was referenced Aug 3, 2026
lawrencecchen
added
the
stale-revisit
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
3 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.
Fixes #9199
Bug 1: rename shortcut targeted the group
With a workspace inside a group focused, Cmd+Shift+R opened the rename prompt for the parent workspace group instead of the focused workspace.
Root cause: plain sidebar clicks routed through
SidebarSelectionCoalescer, which deferred the selection commit by up to 100 ms after a preceding click. Clicking a grouped child right after its group header left the header as the committed command target during that window, so the rename shortcut resolved against the group.Fix: remove the coalescer entirely — plain clicks commit selection synchronously in
SidebarWorkspaceTableController, the same path modifier clicks already used.Bug 2 (dogfood follow-up): group title disappeared after header → child clicks
Clicking the group header and then a grouped child left the group's title invisible (white-on-white) until an unrelated repaint.
Root cause:
NSColor.labelColor.withAlphaComponent(0.9)resolves the dynamic color against the ambient drawing appearance at call time and returns a static color. With selection now committing synchronously inside the click dispatch, the header's authoritative reconfigure can run while a dark appearance (the swapped-in terminal view's) is current — the title snapshots the dark variant (white) and keeps it in the light sidebar. Verified with a standalone AppKit probe:labelColor.withAlphaComponent(0.9)called under darkAqua stays brightness 1.0 when later resolved under aqua, while plainlabelColorresolves to 0.0.Fix: the header title keeps the dynamic
labelColorand dims viaalphaValue(no appearance resolution at configure time); all layer-color conversions inSidebarGroupHeaderTableCellViewandSidebarWorkspaceRowCellViewresolve through the cell's owneffectiveAppearance; the header re-resolves onviewDidChangeEffectiveAppearance. A DEBUG-only probe logs the resolved title paint to the debug event log for live diagnosis.Tests
Failing tests commit first, fix after, per the regression-test commit policy:
rapidGroupHeaderThenChildClickCommitsChildSelectionSynchronously/rapidChildThenGroupHeaderClickCommitsAnchorSelectionSynchronously— realSidebarWorkspaceTableControllerrow clicks asserting the command target synchronously after the click (bug 1's exact repro path).groupHeaderTitleIgnoresAmbientAppearanceAtConfigureTime/anchorActiveGroupHeaderTintResolvesAgainstCellAppearance— configure the header cell under a darkAqua ambient appearance and assert the title/tint still resolve dark in a light appearance (bug 2's mechanism).Notes
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests