fix: hide UI immediately on quit to prevent freeze#30
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VPNBypassApp.swift (1)
115-121:⚠️ Potential issue | 🔴 CriticalMain thread deadlock during quit cleanup.
Line 121 creates a deadlock: the main thread blocks on
semaphore.wait()whilecleanupOnQuit()(a@MainActormethod) cannot run because it is the main thread. SinceRouteManageris@MainActor, all work incleanupOnQuit()and its callees (removeAllRoutes(), XPC calls, property mutations) must execute on the main actor. The blocked thread prevents the task from ever running.Use the proper async termination flow: implement
applicationShouldTerminate(_:), return.terminateLater, and callreply(toApplicationShouldTerminate:)after cleanup completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/VPNBypassApp.swift` around lines 115 - 121, The current quit path blocks the main thread with a DispatchSemaphore while launching Task { await RouteManager.shared.cleanupOnQuit() }, causing a deadlock because cleanupOnQuit() is `@MainActor`; remove the semaphore-based blocking and instead implement applicationShouldTerminate(_:) to return .terminateLater, call RouteManager.shared.cleanupOnQuit() (awaiting it on the main actor), and when it completes invoke reply(toApplicationShouldTerminate:) to allow termination; replace the semaphore/Task pattern in VPNBypassApp.swift with this async termination flow so all `@MainActor` work (cleanupOnQuit, removeAllRoutes, XPC calls, property mutations) runs on the main actor before replying.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/VPNBypassApp.swift`:
- Around line 115-121: The current quit path blocks the main thread with a
DispatchSemaphore while launching Task { await
RouteManager.shared.cleanupOnQuit() }, causing a deadlock because
cleanupOnQuit() is `@MainActor`; remove the semaphore-based blocking and instead
implement applicationShouldTerminate(_:) to return .terminateLater, call
RouteManager.shared.cleanupOnQuit() (awaiting it on the main actor), and when it
completes invoke reply(toApplicationShouldTerminate:) to allow termination;
replace the semaphore/Task pattern in VPNBypassApp.swift with this async
termination flow so all `@MainActor` work (cleanupOnQuit, removeAllRoutes, XPC
calls, property mutations) runs on the main actor before replying.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91c14789-dc43-4ad9-9cca-75e292b07818
📒 Files selected for processing (1)
Sources/VPNBypassApp.swift
Use applicationShouldTerminate with terminateLater instead of blocking the main thread with a semaphore. Since RouteManager is @mainactor, the previous approach deadlocked — cleanup never ran, just timed out.
1 participant
Summary
applicationWillTerminateso the app visually disappears before route cleanup runsTest plan
Summary by CodeRabbit