Refactor OATHSample #41
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jdmoreira
force-pushed
the
joao/refactor-oathsample
branch
from
59d4a6e to
dd372fb
Compare
jdmoreira
force-pushed
the
joao/refactor-oathsample
branch
from
dd372fb to
308fef9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the OATHSample application to improve its architecture and user experience. The main changes consolidate the separate model classes into a unified Model class and introduce a ConnectionManager for better connection handling.
- Unified data model replacing separate OATHListModel and SettingsModel classes
- Centralized connection management through a new ConnectionManager singleton
- Enhanced UI with better empty states, improved error handling, and modern SwiftUI patterns
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
YubiKit.docc/Resources/OATHSampleCode.md |
Updated documentation to reference new Model class |
NFCSmartCardConnection.swift |
Refactored NFC connection handling with improved cleanup and error mapping |
Connection.swift |
Added new cancelledByUser error case for better user dismissal handling |
SettingsView.swift |
Redesigned with modern UI and simplified to use unified Model |
SettingsModel.swift |
Removed - functionality consolidated into Model.swift |
OATHSampleApp.swift |
Simplified app initialization removing model parameter |
OATHListView.swift |
Complete redesign with improved UI, empty states, and reactive connection handling |
OATHListModel.swift |
Removed - functionality consolidated into Model.swift |
Model.swift |
New unified model handling both OATH codes and key information |
ConnectionManager.swift |
New centralized connection management singleton |
project.pbxproj |
Updated project file references |
ConnectionFullStackTests.swift |
Added time limits to test methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| switch error { | ||
| case .none: | ||
| await currentState.didCloseConnection?.fulfill(nil) | ||
| await currentState.connectionPromise?.cancel(with: ConnectionError.cancelledByUser) |
There was a problem hiding this comment.
The connectionPromise is being cancelled with cancelledByUser even when there's no error, but this should only be used when the user explicitly cancels. When error is nil, it should use ConnectionError.cancelled instead.
| await currentState.connectionPromise?.cancel(with: ConnectionError.cancelledByUser) | |
| await currentState.connectionPromise?.cancel(with: ConnectionError.cancelled) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
In the case of error being nil it means the user dismissed the NFC alert. That in turn would map to ConnectionError.cancelledByUser here.
| error = closeError | ||
| } | ||
| } catch { | ||
| if error is CancellationError { return } |
There was a problem hiding this comment.
The condition checks for CancellationError but should check if the error is a cancellation error using 'error as? CancellationError != nil' or '!Task.isCancelled'. The current check will never be true since CancellationError is not being thrown in this context.
| if error is CancellationError { return } | |
| if error as? CancellationError != nil { return } |
Copilot uses AI. Check for mistakes.
| case _ as LightningSmartCardConnection: | ||
| return "Lightning" | ||
| #endif | ||
| case _ as SmartCardConnection: |
There was a problem hiding this comment.
This catch-all case for SmartCardConnection will match all connection types since other specific types inherit from SmartCardConnection. This should be more specific (like USBSmartCardConnection) or moved to the default case.
| case _ as SmartCardConnection: | |
| case _ as WiredSmartCardConnection: |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
It should be USBSmartCardConnection. Will fix.
No description provided.