refactor: Comprehensive code quality improvements following YAGNI+SOLID+DRY+KISS #1
Conversation
…ID+DRY+KISS This commit implements a refined improvement plan focused on production readiness, code quality, and maintainability without over-engineering. ## Infrastructure & Build (P0) - Add Package.swift for SPM support and code organization - Create BUILD.md with comprehensive build instructions - Update README to reference BUILD.md instead of non-existent .xcodeproj - Replace all "yourusername" placeholders with actual GitHub username (Saml1211) - Add SHA256 calculation instructions to Homebrew formula - Remove leftover test.txt file ## Production Essentials (P1) - Add minimal error handling with guard statements for display detection - Create Logger utility using os_log for production debugging - Add strategic logging at key lifecycle points (startup, shutdown, errors) - Implement Accessibility permission check on launch with helpful alert - Complete CONTRIBUTING.md with development guidelines and best practices ## Code Quality Improvements (P1) - Fix state management duplication (DRY violation): * Centralize state in AppDelegate with overlayEnabled/highContrastEnabled * Remove duplicate state from StatusBarController * StatusBarController now queries AppDelegate for current state - Fix DPI scaling bug in CrosshairView: * Store base values (baseCrosshairSize, baseLineWidth) * Calculate final values consistently from base + scale + mode * Prevent accumulation errors from repeated toggling - Remove unimplemented features (YAGNI): * Delete registerGlobalHotKeys() stub method * Update "Preferences" menu item to "About OverlayX" * Remove references to unimplemented keyboard shortcuts (⌘⇧R) * Remove Input Monitoring permission (not actually needed) * Clean up README to reflect only implemented features ## Documentation (P2) - Add targeted code documentation for complex logic: * Window configuration explaining each property's purpose * Coordinate space transformations (screen → window → view) * DPI scaling calculations with multi-display support * Display configuration change handling - Maintain focus on "why" not "what" in comments ## Principles Applied - **YAGNI**: Removed unimplemented features, no over-engineering - **SOLID**: Single source of truth for state, centralized logging - **DRY**: Eliminated state duplication, centralized calculations - **KISS**: Simple solutions, minimal infrastructure, targeted docs Total changes: 11 tasks completed across 10 files Lines of code: ~350 (including new files and documentation)
WalkthroughThe pull request introduces comprehensive documentation (BUILD.md, CONTRIBUTING.md), a Swift Package manifest, and logging infrastructure. Core app logic gains accessibility permission checks on launch, display-aware window setup, enhanced mouse tracking, high-contrast rendering support, and improved state management between AppDelegate and StatusBarController. Homebrew metadata is updated with new repository references. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as AppDelegate
participant Perms as System<br/>Permissions
participant UI as UI Layer<br/>(CrosshairView,<br/>StatusBarController)
User->>App: Launch OverlayX
App->>App: applicationDidFinishLaunching()
App->>Perms: checkAccessibilityPermissions()
alt Permission Granted
Perms-->>App: ✓ Permitted
App->>App: initializeOverlayWindow()
App->>UI: setupOverlay(transparent,<br/>click-through, level)
App->>UI: startMouseTracking(~60Hz)
UI-->>User: Overlay & Crosshair ready
else Permission Denied
Perms-->>App: ✗ Denied
App->>UI: showPermissionAlert()
UI->>Perms: navigateToSystemSettings()
App->>App: terminate()
end
User->>UI: Toggle overlay / Display change
UI->>App: toggleOverlay() / handleDisplayConfigurationChange()
App->>App: Update state (overlayEnabled,<br/>highContrastEnabled)
App->>UI: Resize window & refresh crosshair
UI-->>User: Reflect state change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![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 (7)
Sources/CrosshairView.swift (1)
62-71: Consider early‑returning whenwindowis nil inupdateCrosshairPosition.
updateCrosshairPosition(_:)currently falls back to using the rawscreenPositionwhenwindowisnil, then converts fromnilview space. In practice this probably only runs when the view is in a window, but guarding explicitly would avoid surprising coordinates in odd lifecycle moments:func updateCrosshairPosition(_ screenPosition: NSPoint) { - // NSEvent.mouseLocation returns screen coordinates (origin at bottom-left of main screen) - let windowPosition = window?.convertPoint(fromScreen: screenPosition) ?? screenPosition - // Convert to view coordinates for drawing - crosshairPosition = convert(windowPosition, from: nil) - needsDisplay = true + guard let window = window else { return } + // NSEvent.mouseLocation returns screen coordinates (origin at bottom-left of main screen) + let windowPosition = window.convertPoint(fromScreen: screenPosition) + // Convert to view coordinates for drawing + crosshairPosition = convert(windowPosition, from: nil) + needsDisplay = true }Please verify this doesn’t conflict with wherever
updateCrosshairPositionis called (e.g., only after the overlay window is set up).BUILD.md (1)
1-78: Build guide is thorough; only minor optional polish.The multi‑path build instructions (Xcode,
xcodebuild,swiftc, SPM caveats) are coherent and match the project structure and Package.swift.If you care about polish later, two optional tweaks:
- Consider mentioning an
arm64target in theswiftcexample for Apple Silicon dev machines.- Convert the emphasized troubleshooting labels (
**"...")into proper###headings to appease markdownlinters.Nothing blocking here.
Sources/AppDelegate.swift (5)
15-22: Accessibility gating flow is solid; consider OS prompt + early-return structure.The startup logging plus hard gate on accessibility permission is clean and easy to reason about. Two refinements to consider:
- Use
AXIsProcessTrustedWithOptions(..., kAXTrustedCheckOptionPrompt: true)so macOS can show the standard system prompt instead of relying only on the custom alert.- Have
checkAccessibilityPermissions()return aBooland early-return fromapplicationDidFinishLaunchingwhen permission is missing, so you don’t continue full initialization on a process that is about to terminate anyway.Example sketch (adapt to your style):
func applicationDidFinishLaunching(_ notification: Notification) { Logger.info("OverlayX starting...") guard checkAccessibilityPermissions() else { return } setupStatusBar() setupOverlayWindow() startTrackingMouse() observeDisplayChanges() Logger.info("OverlayX initialized successfully") } private func checkAccessibilityPermissions() -> Bool { let trusted = AXIsProcessTrusted() // or AXIsProcessTrustedWithOptions(...) guard trusted else { Logger.error("Accessibility permissions not granted") showPermissionAlert() return false } Logger.info("Accessibility permissions granted") return true }Also consider calling
NSApp.activate(ignoringOtherApps: true)before presenting the alert to guarantee it appears in front of other apps.Also applies to: 24-55
63-68: Overlay window and display-change handling are robust; consider recovery when the window is missing.The guard on available screens, explicit overlay configuration (transparent, click-through, all spaces), and the display-change handler that resizes both
overlayWindowandcrosshairViewall look solid.One edge case you might want to harden: if
setupOverlayWindow()failed earlier (no displays, or some future error),handleDisplayConfigurationChange()currently logs only about missing displays, not a missing window. A small defensive pattern could help self-heal:private func handleDisplayConfigurationChange() { Logger.info("Display configuration changed, updating overlay") guard let screen = NSScreen.main ?? NSScreen.screens.first else { Logger.error("No displays available after configuration change") return } if overlayWindow == nil { Logger.error("Overlay window missing on display change, reinitializing") setupOverlayWindow() } overlayWindow?.setFrame(screen.frame, display: true) crosshairView?.frame = screen.frame }This keeps behavior graceful in rare initialization failure scenarios without complicating the main flow.
Also applies to: 78-90, 121-133
100-108: Mouse tracking at 60fps is fine; consider pausing the timer when overlay is disabled.The 60fps timer plus
weak selfcapture is a good, leak-free way to get smooth crosshair movement. Since the guard already skips work whenisOverlayEnabledis false, the only remaining cost is the timer firing 60 times per second.If you ever want to trim background work further, you could start/stop the timer inside
toggleOverlay(enabled:)(or pause to 0.0 / slower interval) when the overlay is disabled. Not required, just an easy optimization point.
11-11: Centralizing overlay/high-contrast state in AppDelegate works well; minor guard could reduce redundant updates.Having
isOverlayEnabled/isHighContrastEnabledas private implementation details with read-only accessors and a singletoggleHighContrast(enabled:)entry point is a nice improvement in cohesion withStatusBarControllerandCrosshairView.If you want to avoid unnecessary work when the same state is applied repeatedly, a tiny guard would do it:
func toggleHighContrast(enabled: Bool) { guard isHighContrastEnabled != enabled else { return } isHighContrastEnabled = enabled crosshairView?.setHighContrastMode(enabled: enabled) }Same pattern could be applied to
toggleOverlay(enabled:)if you anticipate frequent idempotent calls.Also applies to: 137-144, 154-157
165-170: Termination logging and cleanup look correct; optionally nil out references.Logging shutdown, invalidating
mouseLocationTimer, and unregistering the display observer on termination is exactly what you want here.If you want to be extra defensive, you could also set
mouseLocationTimer = nilanddisplayObserver = nilafter cleanup to prevent accidental reuse, but that’s entirely optional given app termination.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
BUILD.md(1 hunks)CONTRIBUTING.md(1 hunks)Package.swift(1 hunks)README.md(3 hunks)Sources/AppDelegate.swift(5 hunks)Sources/CrosshairView.swift(3 hunks)Sources/Logger.swift(1 hunks)Sources/StatusBarController.swift(2 hunks)homebrew/overlayx.rb(1 hunks)test.txt(0 hunks)
💤 Files with no reviewable changes (1)
- test.txt
🧰 Additional context used
🧬 Code graph analysis (2)
Sources/StatusBarController.swift (1)
Sources/AppDelegate.swift (3)
openPreferences(159-162)toggleOverlay(145-152)toggleHighContrast(154-157)
Sources/AppDelegate.swift (3)
Sources/Logger.swift (2)
info(9-11)error(13-15)Sources/StatusBarController.swift (2)
toggleHighContrast(46-51)openPreferences(53-55)Sources/CrosshairView.swift (1)
setHighContrastMode(55-60)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~109-~109: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ... under the MIT License. ## Questions? Feel free to open an issue for questions or discussi...
(FEEL_FREE_TO_STYLE_ME)
BUILD.md
[uncategorized] ~73-~73: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... permission errors** - Ensure Xcode has full disk access - Check that all Swift files are...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
BUILD.md
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
72-72: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (7)
Package.swift (1)
1-24: SPM manifest looks coherent with the documented build story.The package definition (tools version, macOS v11 target, executable product, and
Sourcestarget withInfo.plistas a resource) is clean and matches the BUILD.md explanation that SPM is for code organization rather than generating the distributable app bundle.No changes needed from my side here.
CONTRIBUTING.md (1)
1-110: Contributor guide is clear and aligned with the current project setup.This CONTRIBUTING.md reads well and matches the actual build (BUILD.md), logging, and state‑management patterns in the codebase. I don’t see any actionable inconsistencies that need changing right now.
Sources/CrosshairView.swift (1)
5-11: Scaling and high‑contrast refactor looks solid and fixes the accumulation trap.Basing
crosshairSize/lineWidthoff immutablebase*values and recomputing inupdateScaling()/calculateLineWidth(for:)is a clean way to avoid the old “multiply on every move” bug, while also keeping physical size consistent across Retina/non‑Retina displays. The high‑contrast multiplier and glow are applied in one place and don’t mutate unrelated state, which keeps the drawing logic straightforward.No code changes needed here.
Also applies to: 36-53, 73-76, 81-111
Sources/StatusBarController.swift (2)
20-24: Menu wiring, shortcuts, and safe indexing look good.
- The updated “About OverlayX…” item correctly maps to
openPreferences(_:), which forwards to the standard About panel (per AppDelegate).- Help URL points at the README’s
#usagesection, which is a sensible anchor.- Using the
Array[safe:]helper to guard the second menu item when assigning modifier masks is a nice defensive touch.No changes needed here.
Just ensure the README anchor
#usagestays stable if you later reorganize sections.Also applies to: 32-35, 57-61, 64-68
39-51: No issues found—code is correct.Verification confirms that
overlayEnabledandhighContrastEnabledare properly exposed as public computed properties in AppDelegate (lines 137–142) and directly return their private backing fields (isOverlayEnabledandisHighContrastEnabled). StatusBarController correctly reads from these public properties, and the state stays in sync because the computed properties have no intermediate logic—they simply return the backing fields, which are updated only via the toggle methods.Sources/AppDelegate.swift (2)
2-2: AX APIs import is appropriate and minimal.Bringing in
ApplicationServicessolely for accessibility checks keeps the dependency surface tight and is the right choice here.
160-161: YAGNI-orientedopenPreferencesstub is clear.Explicitly documenting that the About panel stands in for preferences keeps expectations clear and aligns with the PR’s YAGNI goals. No changes needed here.
| # TODO: Calculate SHA256 after release: shasum -a 256 OverlayX.dmg | ||
| sha256 "REPLACE_WITH_ACTUAL_SHA256" |
There was a problem hiding this comment.
🧩 Analysis chain
Fill in the real SHA and confirm bundle identifiers before shipping the cask.
The metadata updates look good, but this cask can’t be used as‑is:
sha256is still a placeholder and will cause installs to fail.- The TODO already captures how to compute it; just make sure it’s updated at release time.
- Verify that the bundle identifier in
Info.plistmatches the updated zap paths (com.saml1211.overlayx) sobrew zapactually cleans the right files.
If you want, you can run a quick check after your first signed DMG build:
Also applies to: 6-6, 9-9, 17-18
🏁 Script executed:
#!/bin/bash
# From the directory containing OverlayX.dmg
shasum -a 256 OverlayX.dmg
# After installing via `brew install --cask overlayx`,
# confirm the zap paths are created:
ls -R ~/Library/{Application\ Support,Preferences,Caches} | grep -i overlayx || trueLength of output: 293
🏁 Script executed:
#!/bin/bash
# Locate and inspect the overlayx.rb cask file
fd -e rb overlayx | head -20
# Display the full cask file content
fd -e rb overlayx -exec cat {} \;Length of output: 208
🏁 Script executed:
#!/bin/bash
# Read the cask file directly
cat homebrew/overlayx.rbLength of output: 580
🏁 Script executed:
#!/bin/bash
# Search for Info.plist or bundle ID configuration in the repo
rg -n "com.saml1211.overlayx|CFBundleIdentifier|bundle.*identifier" --type-list | head -20
# Also check for Info.plist files
fd -name "Info.plist" -o -name "*.plist"Length of output: 620
🏁 Script executed:
#!/bin/bash
# Search for bundle identifier in the repo
rg "com.saml1211.overlayx" -n
# Search for Info.plist files
fd "Info.plist"Length of output: 303
🏁 Script executed:
#!/bin/bash
# Read Info.plist to verify CFBundleIdentifier
cat Info.plistLength of output: 1022
Fill in the real SHA256 before shipping; bundle identifiers are correctly configured.
The cask metadata is well-structured. The main issue preventing use is the SHA256 placeholder on line 4, which will cause install failures. The TODO comment already captures the fix—just ensure it's updated when you build the signed DMG.
Bundle identifiers are consistent across the codebase:
Info.plistuses$(PRODUCT_BUNDLE_IDENTIFIER), which Xcode will resolve tocom.saml1211.overlayx- Zap paths on lines 17–18 match this identifier
brew zapwill correctly clean the right preferences and caches
Once you have the signed DMG, run shasum -a 256 OverlayX.dmg and update line 4 before merging.
🤖 Prompt for AI Agents
In homebrew/overlayx.rb around lines 3–4, the sha256 is a placeholder which will
break installs; after creating and signing the release DMG run shasum -a 256
OverlayX.dmg, copy the resulting hash and replace the
"REPLACE_WITH_ACTUAL_SHA256" string on line 4 with that exact SHA256 value, then
save and commit the updated cask before merging.
| ### Manual Installation | ||
| 1. Download the latest release from the [Releases](https://github.com/yourusername/overlayx/releases) page | ||
| 1. Download the latest release from the [Releases](https://github.com/Saml1211/overlayx/releases) page | ||
| 2. Move OverlayX.app to your Applications folder | ||
| 3. First launch: Right-click and select "Open" to bypass Gatekeeper | ||
|
|
||
| ### Building from Source | ||
| 1. Open `OverlayX.xcodeproj` in Xcode | ||
| 2. Build and run the project (⌘R) | ||
| 3. Xcode 13.0 or later required for development | ||
| 1. See [BUILD.md](BUILD.md) for detailed build instructions | ||
| 2. Requires Xcode 13.0 or later | ||
| 3. Quick start: Create new Xcode project and add source files | ||
|
|
There was a problem hiding this comment.
Remove stale “Input Monitoring” reference from Troubleshooting.
The README correctly lists only Accessibility as a required permission now, but the “Overlay Not Visible” section still tells users to “Ensure OverlayX is allowed in Input Monitoring”. That’s inconsistent with the code and the stated goal of dropping that permission.
I’d remove that bullet so the permissions story is:
- System Requirements: Accessibility only.
- Troubleshooting: focus on Accessibility + overlay toggling/restart steps.
The rest of the README (shortcuts, About menu item, BUILD.md reference, multi‑display notes) lines up well with the current implementation.
Also applies to: 60-68, 75-83
🤖 Prompt for AI Agents
In README.md around lines 29-38 (and also apply the same change to lines 60-68
and 75-83), remove the stale “Input Monitoring” permission references from the
Troubleshooting “Overlay Not Visible” section and update the permissions wording
so the System Requirements and Troubleshooting exclusively mention Accessibility
permission; delete the bullet that tells users to “Ensure OverlayX is allowed in
Input Monitoring,” keep steps about Accessibility, overlay toggling, and
restart, and verify the About/shortcuts/BUILD.md notes remain unchanged.
| import Foundation | ||
| import os.log | ||
|
|
||
| /// Simple logging utility for OverlayX | ||
| enum Logger { | ||
| private static let subsystem = "com.saml1211.overlayx" | ||
| private static let log = OSLog(subsystem: subsystem, category: "general") | ||
|
|
||
| static func info(_ message: String) { | ||
| os_log("%{public}s", log: log, type: .info, message) | ||
| } | ||
|
|
||
| static func error(_ message: String) { | ||
| os_log("%{public}s", log: log, type: .error, message) | ||
| } | ||
|
|
||
| static func debug(_ message: String) { | ||
| os_log("%{public}s", log: log, type: .debug, message) | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Use %{public}@ for Swift String when logging.
The lightweight wrapper is nice, but the current format string is mismatched:
os_logwith%sis meant for C strings; with SwiftStringyou generally want%{public}@so the value is bridged correctly.
You can fix this by updating the format string:
- static func info(_ message: String) {
- os_log("%{public}s", log: log, type: .info, message)
- }
+ static func info(_ message: String) {
+ os_log("%{public}@", log: log, type: .info, message)
+ }
@@
- static func error(_ message: String) {
- os_log("%{public}s", log: log, type: .error, message)
- }
+ static func error(_ message: String) {
+ os_log("%{public}@", log: log, type: .error, message)
+ }
@@
- static func debug(_ message: String) {
- os_log("%{public}s", log: log, type: .debug, message)
- }
+ static func debug(_ message: String) {
+ os_log("%{public}@", log: log, type: .debug, message)
+ }Please double‑check the current os_log string formatting guidance for your target macOS versions; if you plan to support only newer macOS releases, you might also consider migrating to the newer os.Logger API in a follow‑up.
Use %{public}@ for Swift String when logging with os_log.
The correct format specifier for logging Swift strings with os_log is %{public}@, not %{public}s. Using %{public}s with Swift String can result in decode errors.
Update the format strings in all three logging methods:
- static func info(_ message: String) {
- os_log("%{public}s", log: log, type: .info, message)
- }
+ static func info(_ message: String) {
+ os_log("%{public}@", log: log, type: .info, message)
+ }
@@
- static func error(_ message: String) {
- os_log("%{public}s", log: log, type: .error, message)
- }
+ static func error(_ message: String) {
+ os_log("%{public}@", log: log, type: .error, message)
+ }
@@
- static func debug(_ message: String) {
- os_log("%{public}s", log: log, type: .debug, message)
- }
+ static func debug(_ message: String) {
+ os_log("%{public}@", log: log, type: .debug, message)
+ }Alternatively, the Logger class (available since iOS 14/macOS 11) is Apple's recommended approach and supersedes os_log, simplifying the complexity and automatically handling privacy considerations.
🤖 Prompt for AI Agents
In Sources/Logger.swift around lines 1 to 20, the os_log format specifier
currently uses "%{public}s" which is incorrect for Swift String and can cause
decode errors; replace "%{public}s" with "%{public}@” in info, error, and debug
calls (or alternatively migrate to Apple's Logger API on supported platforms) so
Swift strings are logged correctly and safely.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR introduces architectural breaking changes in state management and accessibility handling that could disrupt user experience, alongside important fixes for DPI scaling bugs.
📄 Documentation Diagram
This diagram documents the updated app initialization and crosshair display workflow.
sequenceDiagram
participant U as User
participant A as AppDelegate
participant S as StatusBarController
participant C as CrosshairView
participant L as Logger
U->>A: Launch app
A->>A: checkAccessibilityPermissions
note over A: PR #35;1 added permission check
A->>L: info("OverlayX starting...")
A->>S: setupStatusBar
A->>C: setupOverlayWindow
A->>A: startTrackingMouse
A->>L: info("OverlayX initialized successfully")
C->>C: updateScaling
loop Every 1/60s
A->>C: updateCrosshairPosition
end
🌟 Strengths
- Fixes critical DPI scaling accumulation error ensuring consistent rendering
- Removes unimplemented features reducing code bloat per YAGNI
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | Sources/StatusBarController.swift | Architecture | Introduces breaking API change coupling components | - |
| P1 | Sources/CrosshairView.swift | Bug | Fixes DPI scaling accumulation error | - |
| P1 | Sources/AppDelegate.swift | Architecture | Forces app termination if permissions not granted | - |
| P2 | Sources/AppDelegate.swift | Architecture | Method name mismatch confuses users | StatusBarController.swift |
| P2 | homebrew/overlayx.rb | Maintainability | Placeholder SHA256 causes installation failures | - |
| P2 | Sources/Logger.swift | Maintainability | Public log format risks info disclosure | AppDelegate.swift |
🔍 Notable Themes
- Architectural changes centralize state but introduce tight coupling between components
- Behavioral changes like forced termination may negatively impact user experience
📈 Risk Diagram
This diagram illustrates the accessibility permission check and its termination risk.
sequenceDiagram
participant U as User
participant A as AppDelegate
U->>A: Launch app
A->>A: checkAccessibilityPermissions
A->>AXIsProcessTrusted: Check permissions
alt Permissions not granted
A->>L: error("Accessibility permissions not granted")
A->>A: showPermissionAlert
A->>NSApp: terminate(nil)
note over A: R3(P1): Forces app termination without graceful degradation
end
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: Sources/CrosshairView.swift
The DPI scaling fix correctly addresses accumulation errors by storing base values and recalculating from scratch. The related_context shows the previous implementation multiplied line width repeatedly, causing unbounded growth. This change ensures consistent rendering across display scaling changes and high-contrast mode toggles.
Related Code:
private let baseCrosshairSize: CGFloat = 20.0
private let baseLineWidth: CGFloat = 2.0
private let highContrastMultiplier: CGFloat = 1.5
private func calculateLineWidth(for scale: CGFloat) -> CGFloat {
let scaledWidth = baseLineWidth * scale
return isHighContrastMode ? scaledWidth * highContrastMultiplier : scaledWidth
}💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| @objc private func toggleOverlay(_ sender: NSMenuItem) { | ||
| overlayEnabled.toggle() | ||
| appDelegate?.toggleOverlay(enabled: overlayEnabled) | ||
| sender.state = overlayEnabled ? .on : .off | ||
| guard let appDelegate = appDelegate else { return } | ||
| let newState = !appDelegate.overlayEnabled | ||
| appDelegate.toggleOverlay(enabled: newState) | ||
| sender.state = newState ? .on : .off | ||
| } |
There was a problem hiding this comment.
P1 | Confidence: High
The state centralization introduces a breaking change to StatusBarController's API. The related_context shows StatusBarController previously maintained its own overlayEnabled state, but now queries AppDelegate. This creates a tight coupling between the components and changes the controller's behavior from managing state to being a pure view component. While this eliminates state duplication, it fundamentally changes the architectural pattern.
| private func checkAccessibilityPermissions() { | ||
| let trusted = AXIsProcessTrusted() | ||
| if !trusted { | ||
| Logger.error("Accessibility permissions not granted") | ||
| showPermissionAlert() | ||
| } | ||
| } |
There was a problem hiding this comment.
P1 | Confidence: High
The accessibility permission check terminates the app if permissions aren't granted (NSApp.terminate(nil)). This is a breaking behavioral change - previously the app would continue running with limited functionality. Users may expect graceful degradation rather than forced termination, especially if they want to grant permissions later.
| private func checkAccessibilityPermissions() { | |
| let trusted = AXIsProcessTrusted() | |
| if !trusted { | |
| Logger.error("Accessibility permissions not granted") | |
| showPermissionAlert() | |
| } | |
| } | |
| private func checkAccessibilityPermissions() { | |
| let trusted = AXIsProcessTrusted() | |
| if !trusted { | |
| Logger.error("Accessibility permissions not granted") | |
| showPermissionAlert() | |
| // Continue running but log limitations | |
| } | |
| } |
| func openPreferences() { | ||
| // In a production app, you'd implement a proper preferences window | ||
| // Shows the About panel (preferences UI not implemented per YAGNI) | ||
| NSApp.sendAction(#selector(NSApplication.orderFrontStandardAboutPanel(_:)), to: nil, from: nil) | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: High
Speculative: The method name openPreferences now opens the About panel, which violates the principle of least surprise. StatusBarController's menu item still says "About OverlayX..." but calls openPreferences. This semantic mismatch could confuse future developers and users expecting actual preferences.
| func openPreferences() { | |
| // In a production app, you'd implement a proper preferences window | |
| // Shows the About panel (preferences UI not implemented per YAGNI) | |
| NSApp.sendAction(#selector(NSApplication.orderFrontStandardAboutPanel(_:)), to: nil, from: nil) | |
| } | |
| func showAboutPanel() { | |
| NSApp.sendAction(#selector(NSApplication.orderFrontStandardAboutPanel(_:)), to: nil, from: nil) | |
| } |
Evidence: path:Sources/StatusBarController.swift|method:openPreferences
| # TODO: Calculate SHA256 after release: shasum -a 256 OverlayX.dmg | ||
| sha256 "REPLACE_WITH_ACTUAL_SHA256" |
There was a problem hiding this comment.
P2 | Confidence: High
Speculative: The Homebrew formula contains a placeholder SHA256 that will cause installation failures. While this is noted with a TODO, it creates a release-time dependency that could be missed, breaking the Homebrew installation method for users.
| static func info(_ message: String) { | ||
| os_log("%{public}s", log: log, type: .info, message) | ||
| } |
There was a problem hiding this comment.
P2 | Confidence: Medium
The Logger implementation uses %{public}s format for all messages, which may expose sensitive information in production logs. The related_context shows usage for startup messages and errors, but without log level filtering or sanitization, this could lead to unintended information disclosure in system logs.
Code Suggestion:
static func info(_ message: String, isPublic: Bool = false) {
let format = isPublic ? "%{public}s" : "%{private}s"
os_log(format, log: log, type: .info, message)
}Evidence: path:Sources/AppDelegate.swift|search:Logger.info
This commit implements a refined improvement plan focused on production
readiness, code quality, and maintainability without over-engineering.
Infrastructure & Build (P0)
Production Essentials (P1)
Code Quality Improvements (P1)
Documentation (P2)
Principles Applied
Total changes: 11 tasks completed across 10 files
Lines of code: ~350 (including new files and documentation)
Summary by CodeRabbit
New Features
Documentation
Improvements