feat: add troubleshooting tab and improve extension management #105
Conversation
ThomasK33
force-pushed
the
troubleshooting-tab-and-extension-management
branch
2 times, most recently
from
960cacf to
fed46bd
Compare
| // Subscribe to reconfiguration requests | ||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(networkExtensionNeedsReconfiguration(_:)), | ||
| name: .networkExtensionNeedsReconfiguration, | ||
| object: nil | ||
| ) |
There was a problem hiding this comment.
What's the point of making this a custom event and sending it ourselves? Why wouldn't we just call reconfigure?
| extension NSApplication { | ||
| @objc func showLoginWindow() { | ||
| NSApp.sendAction(#selector(NSWindowController.showWindow(_:)), to: nil, from: Windows.login.rawValue) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why would we use this over @Environment(\.openWindow)?
| @@ -8,7 +8,7 @@ class AppState: ObservableObject { | |||
| let appId = Bundle.main.bundleIdentifier! | |||
|
|
|||
| // Stored in UserDefaults | |||
| @Published private(set) var hasSession: Bool { | |||
There was a problem hiding this comment.
Why are we getting rid of this? The whole point is the class maintains the invariant that if hasSession is true, the auth details exist.
| // A dedicated delegate class for system extension deregistration | ||
| private class DeregistrationDelegate: NSObject, OSSystemExtensionRequestDelegate { |
There was a problem hiding this comment.
This should just use the existing delegate?
| let initialNeState = neState | ||
|
|
||
| // Post a notification that the app should reconfigure | ||
| NotificationCenter.default.post(name: .networkExtensionNeedsReconfiguration, object: nil) |
There was a problem hiding this comment.
This either calls state.reconfigure or opens the login window. Both things we could do from this function already.
| var sysExtnState: SystemExtensionState { get } | ||
| var neState: NetworkExtensionState { get } |
There was a problem hiding this comment.
I don't think we should expose these, I think it's sufficient to rely on specific VPNServiceStates.
EDIT: It's probably better to, actually, but I wish we didn't :/
| if tunnelState == .connected || tunnelState == .connecting { | ||
| await stop() | ||
| } |
There was a problem hiding this comment.
This just starts the stop operation. It's not complete until the tunnelState changes to disabled.
| @@ -1,13 +1,16 @@ | |||
| import LaunchAtLogin | |||
| import SwiftUI | |||
|
|
|||
| struct GeneralTab: View { | |||
| struct GeneralTab<VPN: VPNService>: View { | |||
There was a problem hiding this comment.
Why's this need to be here?
| isProcessing = false | ||
|
|
||
| if !success { | ||
| networkExtensionError = "Failed to enable network extension. Check logs for details." |
There was a problem hiding this comment.
This isn't a reasonable request on macOS. enableExtension is also not successful if the configuration didn't change, in which case we shouldn't show an error.
- Add troubleshooting tab to settings with system and network extension controls - Address review feedback related to extension management: - Replace custom notifications with direct method calls for reconfiguration - Restore proper encapsulation for session state management - Improve extension uninstallation with proper state monitoring - Fix deregistration to use existing delegate pattern - Enhance error handling for network extension operations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Change-Id: I947bae1bc7680bd1f670245e4541a95619ab41ee Signed-off-by: Thomas Kosiewski <[email protected]>
ThomasK33
force-pushed
the
troubleshooting-tab-and-extension-management
branch
from
fed46bd to
24d5538
Compare
Summary
Test plan
🤖 Generated with Claude Code