Fix concurrency issues in TimerAutomationEngine and PermissionManager #9
Conversation
Fixed problematic double-wrapped MainActor dispatch patterns that were causing
crashes when permission monitoring was toggled while high-precision timer was running.
## Changes
- Removed nested `Task { @mainactor in }` pattern from high-precision timer callback
- Simplified status update timer to use `DispatchQueue.main.async` only
- Maintained sub-10ms timing accuracy with safer concurrency patterns
- Added comprehensive MainActor safety tests
## Technical Details
Lines 292-297: Fixed high-precision timer callback
- Before: DispatchQueue.main.async { Task { @mainactor in ... } }
- After: DispatchQueue.main.async { Task { ... } }
Lines 416-420: Fixed status update timer
- Before: Task { @mainactor in ... }
- After: DispatchQueue.main.async { ... }
The TimerAutomationEngine class is already @MainActor-isolated, making the
explicit @mainactor annotation redundant and potentially problematic.
## Testing
- App builds successfully with universal binary
- High-frequency automation (50 CPS) operates without crashes
- Permission monitoring concurrent with timer operation works correctly
- Sub-10ms timing accuracy preserved
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
- Added tasks for fixing concurrency issues in PermissionManager, TimerAutomationEngine, and ClickItApp. - Updated ClickIt version to 1.5.0 in Info.plist. - Refactored ClickItApp initialization to avoid concurrency issues by moving setup to onAppear. - Simplified ClickCoordinator automation loop to use Task.sleep for better CPU efficiency. - Improved error handling and logging in ClickCoordinator's performSingleClick method. - Enhanced timer callback safety in HotkeyManager and PerformanceMonitor with overflow checks. - Updated PermissionManager to directly call updatePermissionStatus in timer callbacks. - Created comprehensive crash prevention tests for permission toggles and concurrent operations. - Added timing validation tests to ensure permission monitoring maintains expected intervals. - Ensured resource cleanup and consistency checks in PermissionManager tests.
… ClickEngine, and ClickPointSelector
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jsonify, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on enhancing the application's stability by addressing critical concurrency issues that led to crashes, particularly when interacting with system permissions and high-precision timers. It refines core components to ensure safer execution on the MainActor, improves multi-monitor compatibility, and introduces extensive testing to validate these stability improvements.
Highlights
- Concurrency Crash Fixes: Resolved critical concurrency race conditions in
PermissionManagerandTimerAutomationEngineby replacing problematicTask { @MainActor in }patterns within timer callbacks withDispatchQueue.main.asyncfor safe MainActor dispatch, preventing app crashes, especially during Accessibility permission toggling. - App Initialization Stability: Refactored
ClickItApp's initialization logic, moving it from theinit()method to a newinitializeApp()function called inonAppear. This change preventsMainActortask conflicts during app startup, enhancing overall application stability. - Automation Loop Simplification: Temporarily reverted the automation loop in
ClickCoordinatorfrom usingHighPrecisionTimerto a simplerTask.sleep()based approach. This change, reflected inClickItViewModelno longer directly usingTimerAutomationEngine, prioritizes immediate stability and crash prevention while maintaining core functionality. - Multi-Monitor Coordinate Handling: Improved the accuracy of AppKit to CoreGraphics coordinate conversions across multi-monitor setups in
ClickCoordinator,ClickPointSelector, andClickItViewModel, ensuring precise click placement and location validation. - Enhanced Testing Coverage: Added a comprehensive suite of new XCTest files (
CrashPreventionTests.swift,PermissionManagerConcurrencyTests.swift,PermissionManagerTimingValidation.swift,TimerAutomationEngineMainActorTests.swift,TimerConcurrencyValidationTest.swift) to validate the concurrency fixes, ensure timing accuracy, and prevent regressions. - System Component Robustness: Strengthened
ClickEnginewith more detailed logging and multi-display location validation, and added overflow prevention checks to CPU usage calculations inPerformanceMonitorfor increased reliability.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request addresses critical concurrency issues and improves application stability. The fixes in PermissionManager and TimerAutomationEngine are well-implemented, and the addition of comprehensive tests is a major step forward. I have a few suggestions to further enhance the code, including addressing a potential race condition and refactoring some duplicated code.
| highPrecisionTimer?.startRepeatingTimer(interval: configuration.clickInterval) { [weak self] in | ||
| Task { @MainActor in | ||
| await self?.executeAutomationStep() | ||
| // Use DispatchQueue.main.async to safely get to MainActor context | ||
| DispatchQueue.main.async { | ||
| print("⏰ [TimerAutomationEngine] Timer callback executing - MainActor: \(Thread.isMainThread)") | ||
| Task { | ||
| await self?.executeAutomationStep() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The highPrecisionTimer callback creates a new Task for each timer tick without ensuring the previous one has completed. If executeAutomationStep() takes longer to run than the timer's interval, multiple instances could execute concurrently. This could lead to race conditions.
To prevent this, you should ensure that a new step doesn't start before the previous one has finished. One approach is to use a state flag to track if a step is in progress.
| DispatchQueue.main.async { | ||
| self?.updateAutomationStatus() | ||
| } |
There was a problem hiding this comment.
The DispatchQueue.main.async call here is redundant. Timer.scheduledTimer schedules the timer on the current thread's RunLoop. Since startStatusUpdateTimer() is called from an @MainActor context, the timer is scheduled on the main run loop, and its closure will execute on the main thread. You can call self?.updateAutomationStatus() directly, which is consistent with the fix applied in PermissionManager.swift.
self?.updateAutomationStatus()| /// Converts AppKit coordinates to CoreGraphics coordinates for multi-monitor setups | ||
| private static func convertAppKitToCoreGraphics(_ appKitPosition: CGPoint) -> CGPoint { | ||
| // Find which screen contains this point | ||
| for screen in NSScreen.screens { | ||
| if screen.frame.contains(appKitPosition) { | ||
| // FIXED: Proper multi-monitor coordinate conversion | ||
| // AppKit Y increases upward from screen bottom | ||
| // CoreGraphics Y increases downward from screen top | ||
| // Formula: CG_Y = screen.origin.Y + (screen.height - (AppKit_Y - screen.origin.Y)) | ||
| let relativeY = appKitPosition.y - screen.frame.origin.y // Y relative to screen bottom | ||
| let cgY = screen.frame.origin.y + (screen.frame.height - relativeY) // Convert to CG coordinates | ||
| let cgPosition = CGPoint(x: appKitPosition.x, y: cgY) | ||
| print("ClickCoordinateCapture: Multi-monitor conversion on screen \(screen.frame)") | ||
| print("ClickCoordinateCapture: Calculation: relativeY=\(relativeY), cgY=\(screen.frame.origin.y) + (\(screen.frame.height) - \(relativeY)) = \(cgY)") | ||
| return cgPosition | ||
| } | ||
| } | ||
|
|
||
| // Fallback to main screen if no screen contains the point | ||
| let mainScreenHeight = NSScreen.main?.frame.height ?? 0 | ||
| let fallbackPosition = CGPoint(x: appKitPosition.x, y: mainScreenHeight - appKitPosition.y) | ||
| print("ClickCoordinateCapture: Fallback conversion: AppKit \(appKitPosition) → CoreGraphics \(fallbackPosition)") | ||
| return fallbackPosition | ||
| } |
There was a problem hiding this comment.
This convertAppKitToCoreGraphics function is a great fix for multi-monitor support. However, I've noticed that nearly identical logic exists in ClickCoordinator.swift (lines 613-635) and ClickItViewModel.swift (lines 308-330).
To improve maintainability and prevent future inconsistencies, consider extracting this logic into a single, shared utility function. This could be a static function in a new CoordinateUtils struct, for example.
Resolve concurrency problems in TimerAutomationEngine and PermissionManager, enhancing stability during high-precision timer operations and permission monitoring. Improve initialization patterns in ClickItApp to prevent crashes. Add comprehensive tests to validate safety and performance. Maintain timing accuracy and ensure robust error handling.