Modernize DockPin for v2.2#3
Conversation
Co-authored-by: Babken Egoian <green2grey@users.noreply.github.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Co-authored-by: Babken Egoian <green2grey@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
build.sh (1)
49-49: 📐 Maintainability & Code Quality | 💤 Low valueConsider Sparkle framework discovery edge case.
Using
head -n 1assumes the first foundSparkle.frameworkis correct. In a clean build this should be safe, but if.buildcontains artifacts from multiple configurations, you might select an unintended version. The currentrm -rf "$BUILD"and single release build should prevent this, but consider documenting this assumption or using more specific path matching if needed.🤖 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 `@build.sh` at line 49, SPARKLE_FW discovery currently uses the find + head -n 1 pattern which can pick an unintended framework; update the SPARKLE_FW selection (the find invocation that assigns SPARKLE_FW) to prefer a Release/build-specific match or the most-recent match (for example restrict the find path to include Release or sort by modification time and pick the newest) so it chooses the correct Sparkle.framework, or alternatively add a comment documenting the assumption that the first match is correct.Info.plist (1)
31-32: 📐 Maintainability & Code Quality | ⚡ Quick winVerify copyright year.
The copyright year is set to 2025, but the current date is June 2026. Consider updating to 2026 unless 2025 represents the original creation year and you prefer to keep it that way.
🤖 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 `@Info.plist` around lines 31 - 32, Update the NSHumanReadableCopyright string value to reflect the current year by changing the <key>NSHumanReadableCopyright</key> <string>…</string> entry from "© 2025 green2grey. MIT License." to "© 2026 green2grey. MIT License." (or adjust to a year range like "© 2025–2026" if you intend to show the original creation year plus current year).
🤖 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/AppDelegate.swift`:
- Around line 341-357: Queued auto-switch work can still fire after the user
sets manual hold or disables auto-switch; update the flow so any pending
scheduled auto-switch is cancelled whenever manual hold is set or auto-switch is
turned off and also make performAutoSwitchIfNeeded(snapshot:) defensively bail
out if autoSwitchEnabled is false or manualHoldEnabled is true. Specifically,
add or call a cancellation path from selectProfile(id:) (before/after
profileManager.setActiveProfile), from toggleAutoSwitch() when disabling
auto-switch, and from resumeAutoSwitching() as appropriate to cancel the
debounce/task created by scheduleAutoSwitch(); also add the guard checks shown
in performAutoSwitchIfNeeded(snapshot:) to avoid applying switches if
autoSwitchEnabled is false or manualHoldEnabled is true.
In `@Sources/DockMonitor.swift`:
- Around line 67-68: TapState is snapshotted for processEvent, but
allowedDisplays and overrideModifier are plain mutable properties so runtime
writes leave TapState stale; update the implementation so writes go through a
single updater that (a) updates allowedDisplays and overrideModifier together
(or invalidates TapState) and (b) recomputes the locked snapshot used by
processEvent (same logic currently in the block at lines 109-143); add a
dedicated setter method (e.g., setAllowedDisplaysAndOverrideModifier or
refreshTapStateWith) that updates both symbols and calls the existing
refresh/snapshot logic (same work done by refreshScreenCache()) so processEvent
always sees the latest config; also apply the same pattern to the other update
sites referenced (lines 109-143 and 266-289).
In `@Sources/UI/StatusBarPopover.swift`:
- Around line 71-87: The popover VStack can grow unbounded and hide bottom
controls when the popover is height-constrained (AppDelegate fixes it at 600pt);
wrap the VStack in a vertical ScrollView so the content scrolls when it exceeds
the popover height. Concretely, in StatusBarPopover.swift replace the top-level
VStack in the body with ScrollView(.vertical, showsIndicators: true) {
VStack(alignment: .leading, spacing: 12) { ... } } and keep the existing
.padding(16) and .frame(width: 360) modifiers (optionally add .frame(maxHeight:
560) or similar to the ScrollView if you want an explicit max content height).
Apply the same ScrollView wrapping to the other similar views that render
displays(state), profiles(state) and footerActions(state) so bottom actions
remain reachable.
---
Nitpick comments:
In `@build.sh`:
- Line 49: SPARKLE_FW discovery currently uses the find + head -n 1 pattern
which can pick an unintended framework; update the SPARKLE_FW selection (the
find invocation that assigns SPARKLE_FW) to prefer a Release/build-specific
match or the most-recent match (for example restrict the find path to include
Release or sort by modification time and pick the newest) so it chooses the
correct Sparkle.framework, or alternatively add a comment documenting the
assumption that the first match is correct.
In `@Info.plist`:
- Around line 31-32: Update the NSHumanReadableCopyright string value to reflect
the current year by changing the <key>NSHumanReadableCopyright</key>
<string>…</string> entry from "© 2025 green2grey. MIT License." to "© 2026
green2grey. MIT License." (or adjust to a year range like "© 2025–2026" if you
intend to show the original creation year plus current year).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab9576f-2296-492b-a4de-0abd3aa224f0
📒 Files selected for processing (11)
Info.plistPackage.swiftSources/AppDelegate.swiftSources/DisplayLayoutAnalyzer.swiftSources/DockMonitor.swiftSources/NSScreen+DockPinDisplayID.swiftSources/ProfileManager.swiftSources/SetupWindowController.swiftSources/UI/SetupView.swiftSources/UI/StatusBarPopover.swiftbuild.sh
📜 Review details
🧰 Additional context used
🪛 SwiftLint (0.63.3)
Sources/AppDelegate.swift
[Warning] 64-64: An object should only remove itself as an observer in deinit
(notification_center_detachment)
🔇 Additional comments (18)
Info.plist (2)
12-14: LGTM!
35-36: LGTM!Package.swift (3)
14-17: LGTM!
1-1: 📐 Maintainability & Code QualityVerify Swift 5.10 toolchain availability for
Package.swift(swift-tools-version: 5.10).
swift --versionfails in this environment (swift: command not found), so Swift 5.10 availability can’t be checked here. Ensure developer/CI environments install Swift 5.10 (or newer) to support the manifest’s tools version.
8-8: 📐 Maintainability & Code QualitySparkle 2.9.2 dependency bump is safe.
Sparkle 2.9.2 exists, and its release notes focus on security/accessibility/stability fixes (e.g., delta symlink guards, installer/appcast validation) without breaking changes to the SPUStandardUpdaterController/SPUUpdater updater mechanism for existing Sparkle 2.x usage.
build.sh (6)
11-26: LGTM!
57-65: LGTM!
68-71: LGTM!
72-75: LGTM!
78-79: LGTM!
81-89: LGTM!Sources/DisplayLayoutAnalyzer.swift (1)
8-8: LGTM!Also applies to: 14-14
Sources/ProfileManager.swift (2)
76-82: LGTM!
197-202: LGTM!Sources/NSScreen+DockPinDisplayID.swift (1)
6-8: 📐 Maintainability & Code Quality
NSScreen.cgDirectDisplayIDis available in macOS 14.0+ and matches the fallback.
NSScreen.cgDirectDisplayIDis available on macOS 14.0 and later and returns the screen’sCGDirectDisplayID. The existing fallback that readsNSScreenNumberfromNSScreen.deviceDescriptionalso provides the sameCGDirectDisplayIDvalue as anNSNumber, so both paths should be consistent.Sources/UI/StatusBarPopover.swift (1)
5-65: LGTM!Sources/UI/SetupView.swift (1)
17-188: LGTM!Sources/SetupWindowController.swift (1)
29-47: LGTM!
| private func selectProfile(id: UUID) { | ||
| profileManager.setActiveProfile(id: id, manualHold: true) | ||
| applyCurrentState(snapshot: .current(), attemptReanchor: true) | ||
| } | ||
|
|
||
| @objc private func toggleAutoSwitch() { | ||
| private func toggleAutoSwitch() { | ||
| profileManager.autoSwitchEnabled.toggle() | ||
| if profileManager.autoSwitchEnabled { | ||
| scheduleAutoSwitch() | ||
| } | ||
| refreshStatusBarModel(snapshot: .current()) | ||
| } | ||
|
|
||
| @objc private func resumeAutoSwitching() { | ||
| private func resumeAutoSwitching() { | ||
| profileManager.manualHoldEnabled = false | ||
| scheduleAutoSwitch() | ||
| refreshStatusBarModel(snapshot: .current()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cancel queued auto-switch work when the user enters manual hold or turns auto-switch off.
scheduleAutoSwitch() only checks autoSwitchEnabled / manualHoldEnabled when the debounce task is created. If a task is already pending, selectProfile(id:) and the off path in toggleAutoSwitch() leave it alive, so performAutoSwitchIfNeeded(snapshot:) can still switch profiles a few hundred milliseconds later and override the user’s manual selection or disabled state.
♻️ Suggested fix
private func selectProfile(id: UUID) {
+ autoSwitchTask?.cancel()
+ autoSwitchTask = nil
profileManager.setActiveProfile(id: id, manualHold: true)
applyCurrentState(snapshot: .current(), attemptReanchor: true)
}
private func toggleAutoSwitch() {
profileManager.autoSwitchEnabled.toggle()
if profileManager.autoSwitchEnabled {
scheduleAutoSwitch()
+ } else {
+ autoSwitchTask?.cancel()
+ autoSwitchTask = nil
}
refreshStatusBarModel(snapshot: .current())
}
private func resumeAutoSwitching() {
profileManager.manualHoldEnabled = false
scheduleAutoSwitch()
refreshStatusBarModel(snapshot: .current())
}private func performAutoSwitchIfNeeded(snapshot: DisplaySnapshot) {
guard isConfigured,
profileManager.setupCompleted,
profileManager.autoSwitchEnabled,
!profileManager.manualHoldEnabled else { return }
guard let match = profileManager.bestAutoSwitchMatch(snapshot: snapshot) else { return }
guard match.id != profileManager.activeProfileID else { return }
profileManager.setActiveProfile(id: match.id, manualHold: false)
applyCurrentState(snapshot: snapshot, attemptReanchor: true)
}🤖 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/AppDelegate.swift` around lines 341 - 357, Queued auto-switch work
can still fire after the user sets manual hold or disables auto-switch; update
the flow so any pending scheduled auto-switch is cancelled whenever manual hold
is set or auto-switch is turned off and also make
performAutoSwitchIfNeeded(snapshot:) defensively bail out if autoSwitchEnabled
is false or manualHoldEnabled is true. Specifically, add or call a cancellation
path from selectProfile(id:) (before/after profileManager.setActiveProfile),
from toggleAutoSwitch() when disabling auto-switch, and from
resumeAutoSwitching() as appropriate to cancel the debounce/task created by
scheduleAutoSwitch(); also add the guard checks shown in
performAutoSwitchIfNeeded(snapshot:) to avoid applying switches if
autoSwitchEnabled is false or manualHoldEnabled is true.
| var allowedDisplays: Set<CGDirectDisplayID> = [] | ||
| var overrideModifier: ModifierOption = .option |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Refresh TapState when the live configuration changes.
processEvent now consults only the locked TapState, but Lines 67-68 are still plain mutable properties and Lines 109-143 are the only place that copy them into that snapshot. A runtime write to allowedDisplays or overrideModifier will therefore keep using stale blocked zones / override flags until some unrelated refreshScreenCache() path happens.
💡 Local fix
- var allowedDisplays: Set<CGDirectDisplayID> = []
- var overrideModifier: ModifierOption = .option
+ var allowedDisplays: Set<CGDirectDisplayID> = [] {
+ didSet {
+ guard oldValue != allowedDisplays else { return }
+ refreshScreenCache()
+ }
+ }
+ var overrideModifier: ModifierOption = .option {
+ didSet {
+ guard oldValue != overrideModifier else { return }
+ refreshScreenCache()
+ }
+ }If you want to batch both changes together, a dedicated setter that updates both properties and refreshes once would avoid duplicate recomputation.
Also applies to: 109-143, 266-289
🤖 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/DockMonitor.swift` around lines 67 - 68, TapState is snapshotted for
processEvent, but allowedDisplays and overrideModifier are plain mutable
properties so runtime writes leave TapState stale; update the implementation so
writes go through a single updater that (a) updates allowedDisplays and
overrideModifier together (or invalidates TapState) and (b) recomputes the
locked snapshot used by processEvent (same logic currently in the block at lines
109-143); add a dedicated setter method (e.g.,
setAllowedDisplaysAndOverrideModifier or refreshTapStateWith) that updates both
symbols and calls the existing refresh/snapshot logic (same work done by
refreshScreenCache()) so processEvent always sees the latest config; also apply
the same pattern to the other update sites referenced (lines 109-143 and
266-289).
| var body: some View { | ||
| let state = model.state | ||
|
|
||
| VStack(alignment: .leading, spacing: 12) { | ||
| header(state) | ||
| Divider() | ||
| primaryAction(state) | ||
| dockEdge(state) | ||
| displays(state) | ||
| modifierPicker(state) | ||
| profiles(state) | ||
| Divider() | ||
| footerActions(state) | ||
| } | ||
| .padding(16) | ||
| .frame(width: 360) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the popover content scrollable before multi-display setups clip the lower controls.
Sources/AppDelegate.swift fixes the popover at 600pt high, but this view stacks an unbounded displays section, profiles section, and footer in a plain VStack. On larger monitor setups, the bottom actions can fall below the visible area and become unreachable.
♻️ Suggested fix
- VStack(alignment: .leading, spacing: 12) {
- header(state)
- Divider()
- primaryAction(state)
- dockEdge(state)
- displays(state)
- modifierPicker(state)
- profiles(state)
- Divider()
- footerActions(state)
- }
- .padding(16)
- .frame(width: 360)
+ ScrollView {
+ VStack(alignment: .leading, spacing: 12) {
+ header(state)
+ Divider()
+ primaryAction(state)
+ dockEdge(state)
+ displays(state)
+ modifierPicker(state)
+ profiles(state)
+ Divider()
+ footerActions(state)
+ }
+ .padding(16)
+ }
+ .frame(width: 360)Also applies to: 145-175, 225-314
🤖 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/UI/StatusBarPopover.swift` around lines 71 - 87, The popover VStack
can grow unbounded and hide bottom controls when the popover is
height-constrained (AppDelegate fixes it at 600pt); wrap the VStack in a
vertical ScrollView so the content scrolls when it exceeds the popover height.
Concretely, in StatusBarPopover.swift replace the top-level VStack in the body
with ScrollView(.vertical, showsIndicators: true) { VStack(alignment: .leading,
spacing: 12) { ... } } and keep the existing .padding(16) and .frame(width: 360)
modifiers (optionally add .frame(maxHeight: 560) or similar to the ScrollView if
you want an explicit max content height). Apply the same ScrollView wrapping to
the other similar views that render displays(state), profiles(state) and
footerActions(state) so bottom actions remain reachable.
2 participants
Summary
Testing
git diff --check HEAD~2..HEADpassed.python3 -c "import plistlib; ..."passed and confirmed v2.2/220 plus required plist keys.bash -n build.shpassed.swift package dump-packagepassed and confirmed tools 5.10 plus Sparkle lower bound 2.9.2.swift build -c releasewas attempted but this Linux cloud runner lacks the macOS Cocoa module.swift build -c release -Xswiftc -warnings-as-errors -Xswiftc -strict-concurrency=completewas attempted but this Linux cloud runner lacks the macOS Cocoa module.DOCKPIN_SIGN_ID='Developer ID Application' ./build.shwas attempted and now fails clearly because this runner lacks macOSsecurity/codesign tooling.Summary by CodeRabbit
New Features
Improvements
Chores