Repository navigation
Fix titlebar icon ownership and chrome alignment - #3883
lawrencecchen wants to merge 2 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR removes traffic-light debug offset support from ChangesTraffic-light offset removal & minimal-mode inset persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (12 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 SummaryThis PR makes two independent improvements: it replaces the
Confidence Score: 5/5Safe to merge — the child-panel removal is a clean simplification and the offset arithmetic is correct. All changes are narrowly scoped UI positioning fixes. The child-panel teardown is fully removed with no dangling observers or stale references. The NSMapTable-based frame-state storage is correctly lifecycle-bound to the button via weak keys, and the new alignment constants produce stable, non-accumulating offsets. The only gap is that updateNSView calls NSWorkspace.icon(forFile:) on every parent re-render rather than only when the directory actually changes, which is a performance concern but not a correctness issue. Sources/DetachedFolderDragIcon.swift — updateNSView unconditionally reloads the folder icon from disk on every SwiftUI update cycle. Important Files Changed
Sequence DiagramsequenceDiagram
participant SUI as SwiftUI (ContentView)
participant DFD as DetachedFolderDragIcon
participant DFV as DraggableFolderNSView
participant WS as NSWorkspace
Note over SUI,WS: New inline approach (this PR)
SUI->>DFD: makeNSView
DFD->>DFV: init(directory:)
DFV->>WS: icon(forFile:)
WS-->>DFV: NSImage
SUI->>DFD: updateNSView (any parent re-render)
DFD->>DFV: "directory = newDir"
DFD->>DFV: updateIcon()
DFV->>WS: "icon(forFile:) <- called unconditionally"
participant WDC as WindowDecorationsController
participant BTN as NSButton (traffic light)
participant MT as NSMapTable
WDC->>MT: trafficLightFrameState(for: button)
MT-->>WDC: TrafficLightFrameState (weak key)
WDC->>BTN: currentFrameMatchesApplied?
alt frame moved externally
WDC->>BTN: setFrameOrigin(baseOrigin + offset)
end
WDC->>MT: "state.appliedFrame = button.frame"
Reviews (8): Last reviewed commit: "Move traffic lights slightly higher" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cmuxTests/WindowAndDragTests.swift`:
- Around line 816-822: Add a private helper function named
restoreDefaultsValue(_ value: Any?, forKey key: String, defaults: UserDefaults)
to the WindowAndDragTests test class so the defer call in
testWindowDecorationsDoNotMoveNativeTrafficLights compiles; the method should
set the provided value into the given UserDefaults when value is non-nil and
remove the object for the key when value is nil (mirror the suggested behavior),
ensuring the helper is marked private and has the same signature used in the
test.
🪄 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: 8cbfdee6-f084-4f19-8143-f07c99ea6f5a
📒 Files selected for processing (3)
Sources/WindowDecorationsController.swiftSources/WindowDragHandleView.swiftcmuxTests/WindowAndDragTests.swift
💤 Files with no reviewable changes (1)
- Sources/WindowDecorationsController.swift
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/WindowAndDragTests.swift (1)
816-863:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd the missing
restoreDefaultsValuehelper function to avoid compilation error.Line 821 calls
restoreDefaultsValue(savedMode, forKey:defaults:), but this function is not defined in WindowAndDragTests.swift. The code will fail to compile.Add the helper function to the test class:
Suggested implementation
private func restoreDefaultsValue(_ value: Any?, forKey key: String, defaults: UserDefaults) { if let value { defaults.set(value, forKey: key) } else { defaults.removeObject(forKey: key) } }🤖 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 `@cmuxTests/WindowAndDragTests.swift` around lines 816 - 863, Add a private helper named restoreDefaultsValue(_ value: Any?, forKey key: String, defaults: UserDefaults) to the test class so the call in testMinimalModeSidebarTitlebarClickTargetUsesStableRaisedFrame compiles; the helper should set the value for the key when value is non-nil and remove the key when value is nil (i.e., if let value { defaults.set(value, forKey: key) } else { defaults.removeObject(forKey: key) }) to properly restore UserDefaults state after the test.
🤖 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 `@cmuxTests/WindowAndDragTests.swift`:
- Around line 816-863: Add a private helper named restoreDefaultsValue(_ value:
Any?, forKey key: String, defaults: UserDefaults) to the test class so the call
in testMinimalModeSidebarTitlebarClickTargetUsesStableRaisedFrame compiles; the
helper should set the value for the key when value is non-nil and remove the key
when value is nil (i.e., if let value { defaults.set(value, forKey: key) } else
{ defaults.removeObject(forKey: key) }) to properly restore UserDefaults state
after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51fa192c-83bc-436f-bc4b-24f90641cda4
📒 Files selected for processing (2)
Sources/WindowDragHandleView.swiftcmuxTests/WindowAndDragTests.swift
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/DetachedFolderDragIcon.swift`:
- Around line 51-59: The setFrameOrigin(_:) and setFrameSize(_:) overrides call
syncDetachedIconFrame() while frame/bounds change observers (enabled via
postsFrameChangedNotifications and NotificationCenter) also call it, causing
duplicate invocations; pick one synchronization path and remove the other to
simplify behavior: either delete the overrides setFrameOrigin and setFrameSize
and rely on the NotificationCenter observers, or keep the overrides and remove
the observers/disable postsFrameChangedNotifications; ensure
syncDetachedIconFrame() (which already has a guard to avoid redundant work)
continues to be called from the chosen path and update any observer
registration/removal code accordingly (e.g., the NotificationCenter
addObserver/removeObserver references).
- Around line 150-164: The code currently registers both
NSView.frameDidChangeNotification and NSView.boundsDidChangeNotification in
installHostGeometryObservers to call syncDetachedIconFrame; remove
NSView.boundsDidChangeNotification (and any code that sets
postsBoundsChangedNotifications on the host view) so only frame changes drive
the child-window sync, keeping hostGeometryObservers mapping and the closure
that calls MainActor.assumeIsolated { self?.syncDetachedIconFrame() } intact; if
you expect transforms/flipping in the host view, keep the bounds observer,
otherwise eliminate it to reduce unnecessary notifications.
🪄 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: f55cef1b-e156-4d46-85a5-bca1f5117717
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/DetachedFolderDragIcon.swiftcmuxTests/WindowAndDragTests.swift
![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 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 3fd3169. Configure here.
aa3589f to
b984be9
Compare
b984be9 to
90cdd7a
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lawrencecchen
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func applyTrafficLightOffsets(to window: NSWindow) { | ||
| let snapshot = MinimalModeTitlebarDebugSettings.snapshot() | ||
| let offset = NSPoint( | ||
| x: CGFloat(snapshot.trafficLightsXOffset), | ||
| y: CGFloat(snapshot.trafficLightsYOffset) | ||
| ) | ||
| for buttonType in [NSWindow.ButtonType.closeButton, .miniaturizeButton, .zoomButton] { | ||
| guard let button = window.standardWindowButton(buttonType) else { continue } | ||
| let state = trafficLightFrameState(for: button) | ||
| let baseOrigin = state.currentFrameMatchesApplied(button.frame) | ||
| ? state.baseOrigin | ||
| : button.frame.origin | ||
| let nextOrigin = NSPoint(x: baseOrigin.x + offset.x, y: baseOrigin.y + offset.y) | ||
| if abs(button.frame.origin.x - nextOrigin.x) > 0.25 | ||
| || abs(button.frame.origin.y - nextOrigin.y) > 0.25 { | ||
| button.setFrameOrigin(nextOrigin) | ||
| } | ||
| state.baseOrigin = baseOrigin | ||
| state.appliedFrame = button.frame | ||
| } | ||
| } | ||
|
|
||
| private func trafficLightFrameState(for button: NSButton) -> TrafficLightFrameState { | ||
| if let state = trafficLightFrameStates.object(forKey: button) { | ||
| return state | ||
| } | ||
| let state = TrafficLightFrameState(baseOrigin: button.frame.origin, appliedFrame: button.frame) | ||
| trafficLightFrameStates.setObject(state, forKey: button) | ||
| return state | ||
| } |
There was a problem hiding this comment.
The PR description states "Regression test added in the first commit — testWindowDecorationsDoNotMoveNativeTrafficLights" but the PR contains a single commit ("Fix titlebar icon ownership and chrome alignment") and no test file appears anywhere in the diff. A search of cmuxTests/ confirms no such function exists. The regression safety net that was the stated justification for the change is missing.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Verification
Note
Medium Risk
Touches AppKit titlebar chrome positioning and traffic-light button frame adjustments, which can regress window controls across macOS versions. Scope is localized to UI/layout with no data/auth changes.
Overview
Keeps the focused-directory folder drag icon inline in the SwiftUI titlebar by removing the detached child
NSPanelhosting approach and renderingDraggableFolderNSViewdirectly.Adjusts minimal-mode titlebar chrome alignment by nudging left controls and traffic lights upward, and makes traffic-light offsets non-accumulating by tracking per-button base/applied frames inside
WindowDecorationsControllervia anNSMapTable(replacing associated-object storage on the buttons).Reviewed by Cursor Bugbot for commit f79dac5. Bugbot is set up for automated code reviews on this repo. Configure here.