-
-
Notifications
You must be signed in to change notification settings - Fork 0
Ux UI #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ux UI #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a comprehensive UX/UI overhaul for the SilentKey password manager application, introducing new authentication views, vault management UI, and supporting infrastructure. The changes include a complete localization system, enhanced security features, and new data models.
Changes:
- New authentication UI with biometric and FIDO2 security key support
- Complete vault management interface with search, filtering, and empty states
- Localization system supporting English and French with persistent language selection
- Core security infrastructure including KeychainManager and SecurityKeyManager
- New data models for passwords, certificates, and projects with encryption support
- Enhanced VaultManager with salt-based key derivation and metadata management
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/BACKLOG.md | Updated task completion status for core files and features |
| Sources/SilentKeyApp/Views/VaultView.swift | New vault dashboard with search and item listing |
| Sources/SilentKeyApp/Views/StoreView.swift | Commented out preview for compilation |
| Sources/SilentKeyApp/Views/SharedComponents.swift | Logo and gradient components for UI |
| Sources/SilentKeyApp/Views/PermanentFooterView.swift | Persistent footer with branding and version info |
| Sources/SilentKeyApp/Views/MainView.swift | Main navigation hub with sidebar and content views |
| Sources/SilentKeyApp/Views/AuthenticationView.swift | Authentication screen with multiple auth methods |
| Sources/SilentKeyApp/Utilities/Logger.swift | Fixed enum syntax errors |
| Sources/SilentKeyApp/Utilities/Localizable.swift | New localization system with multi-language support |
| Sources/SilentKeyApp/Utilities/Color+Extensions.swift | Platform-adaptive color extensions |
| Sources/SilentKeyApp/Utilities/AuthenticationManager.swift | Enhanced authentication with password policies and generation |
| Sources/SilentKeyApp/Store/StoreManager.swift | Fixed concurrency and namespace issues |
| Sources/SilentKeyApp/SilentKeyApp.swift | Simplified app entry point with window management |
| Sources/SilentKeyApp/Models/AppModels.swift | New app state and navigation models |
| Sources/SilentKeyApp/ContentView.swift | Simplified to route between auth and main views |
| Sources/Core/Storage/VaultManager.swift | Added salt-based encryption and metadata support |
| Sources/Core/Storage/TrashManager.swift | Fixed concurrency and unused variable issues |
| Sources/Core/Storage/FileStorage.swift | Enhanced backup/restore with metadata |
| Sources/Core/Security/SecurityKeyManager.swift | New FIDO2 security key management |
| Sources/Core/Security/KeychainManager.swift | New secure storage for master password |
| Sources/Core/Protocols/SecretItemProtocol.swift | Added project category |
| Sources/Core/Plugins/PluginSystem.swift | Registered new template types |
| Sources/Core/Notifications/PushNotificationManager.swift | Native macOS notification support |
| Sources/Core/Models/SecretItem.swift | Enhanced with protocol conformance and new fields |
| Sources/Core/Models/ProjectModels.swift | Updated with breaking API changes and new fields |
| Sources/Core/Models/PasswordModels.swift | New password secret model with encryption |
| Sources/Core/Models/CertificateModels.swift | New certificate secret model |
| Sources/Core/Crypto/EncryptionManager.swift | Updated key derivation with salt parameter |
| Package.swift | Added resources support |
| .swiftpm/xcode/package.xcworkspace/contents.xcworkspacedata | Xcode workspace configuration |
| .swiftpm/SilentKey-Package.xctestplan | Test plan configuration |
Files not reviewed (1)
- .swiftpm/xcode/package.xcworkspace/contents.xcworkspacedata: Language not supported
| let devPath = "/Users/ethanbernier/Library/CloudStorage/OneDrive-Phoenix/GitHub/SilentKey/docs/assets/logo.png" | ||
| return NSImage(contentsOfFile: devPath) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded absolute development path detected. This path "/Users/ethanbernier/Library/CloudStorage/OneDrive-Phoenix/GitHub/SilentKey/docs/assets/logo.png" is specific to one developer's machine and will fail on other systems. The logo should be included in the app bundle as a resource or accessed via a relative path.
| let devPath = "/Users/ethanbernier/Library/CloudStorage/OneDrive-Phoenix/GitHub/SilentKey/docs/assets/logo.png" | |
| return NSImage(contentsOfFile: devPath) | |
| // Load logo from the app bundle using the shared asset name. | |
| return NSImage(named: "Logo") |
| public func authenticate(with password: String) async { | ||
| self.authError = nil | ||
|
|
||
| if password == "BIOMETRIC_BYPASS" { |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "BIOMETRIC_BYPASS" string is used as a magic constant for authentication bypass. This is a security anti-pattern that could be exploited if this string is accidentally passed from an untrusted source. Use a dedicated boolean flag or enum value instead of a magic string.
| // Consecutive numbers check (no more than 2) | ||
| let chars = Array(p) | ||
| for i in 0..<(chars.count - 2) { | ||
| if chars[i].isNumber && chars[i+1].isNumber && chars[i+2].isNumber { | ||
| return .failure(.message("No more than 2 consecutive numbers allowed")) | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consecutive digits check has an off-by-one error in the comment. The code checks for 3 consecutive digits (chars[i], chars[i+1], chars[i+2]) but the error message says "No more than 2 consecutive numbers allowed", which is correct. However, the validation comment on line 208 says "No more than 2 consecutive digits" which is ambiguous. Clarify whether the policy allows 2 or 3 consecutive digits.
| var notes: String | ||
| var url: String? // URL du dépôt Git ou site web | ||
| var environment: ProjectEnvironment | ||
| public var notes: String? |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notes field changed from non-optional String to optional String? which is a breaking change. Any existing code that relies on notes being non-nil will break. This change should be documented or the field should maintain backward compatibility.
| public var notes: String? | ||
| public var createdAt: Date | ||
| public var updatedAt: Date | ||
| public var modifiedAt: Date |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'updatedAt' property was renamed to 'modifiedAt' which is a breaking API change. This will break any existing code or serialized data using the old property name. Consider providing migration support or maintaining backward compatibility with a deprecated alias.
| items = [ | ||
| SecretItem(title: "Google Workspace", type: .credential, encryptedValue: Data()), | ||
| SecretItem(title: "AWS Production Key", type: .apiKey, encryptedValue: Data()), | ||
| SecretItem(title: "Phoenix Agency Token", type: .token, encryptedValue: Data()) | ||
| ] |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mock data with empty encryptedValue fields could cause runtime errors if decryption is attempted. Either remove this mock data or populate with properly encrypted dummy values to ensure the UI behavior matches production.
| private func triggerFIDO() { | ||
| // Here we would trigger the ASAuthorizationPlatformPublicKeyCredentialProvider | ||
| Task { await authManager.authenticate(with: "BIOMETRIC_BYPASS") } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FIDO authentication trigger uses the same "BIOMETRIC_BYPASS" string as biometric auth, which is incorrect. FIDO2/WebAuthn requires a separate authentication flow with challenge-response. This implementation conflates biometric and security key authentication.
| public var createdAt: Date | ||
| public var modifiedAt: Date | ||
| public var isFavorite: Bool | ||
|
|
||
| // Propriétés spécifiques au projet | ||
| var description: String | ||
| var tags: [String] | ||
| var status: ProjectStatus | ||
| var icon: ProjectIcon | ||
| var color: String // Hex color | ||
| public var description: String | ||
| public var tags: Set<String> | ||
| public var status: ProjectStatus | ||
| public var icon: ProjectIcon | ||
| public var color: String // Hex color | ||
|
|
||
| // Relations multiples (N-N) | ||
| var relatedAPIKeys: [UUID] | ||
| var relatedSecrets: [UUID] | ||
| var relatedBankingAccounts: [UUID] | ||
| var relatedCertificates: [UUID] | ||
| var relatedPasswords: [UUID] | ||
| public var relatedAPIKeys: [UUID] | ||
| public var relatedSecrets: [UUID] | ||
| public var relatedBankingAccounts: [UUID] | ||
| public var relatedCertificates: [UUID] | ||
| public var relatedPasswords: [UUID] | ||
|
|
||
| // Métadonnées | ||
| var notes: String | ||
| var url: String? // URL du dépôt Git ou site web | ||
| var environment: ProjectEnvironment | ||
| public var notes: String? | ||
| public var url: String? // URL du dépôt Git ou site web | ||
| public var environment: ProjectEnvironment | ||
|
|
||
| public var category: SecretCategory { .project } | ||
| public var iconName: String { icon.systemImage } | ||
|
|
||
| // MARK: - Initialisation | ||
|
|
||
| init( | ||
| id: UUID = UUID(), | ||
| title: String, | ||
| description: String = "", | ||
| tags: [String] = [], | ||
| tags: Set<String> = [], | ||
| status: ProjectStatus = .active, | ||
| icon: ProjectIcon = .folder, | ||
| color: String = "#007AFF", | ||
| notes: String = "", | ||
| notes: String? = nil, | ||
| url: String? = nil, | ||
| environment: ProjectEnvironment = .development | ||
| environment: ProjectEnvironment = .development, | ||
| isFavorite: Bool = false | ||
| ) { | ||
| self.id = id | ||
| self.title = title | ||
| self.version = 1 | ||
| self.createdDate = Date() | ||
| self.lastModified = Date() | ||
| self.createdAt = Date() | ||
| self.modifiedAt = Date() |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Property names changed from 'createdDate'/'lastModified' to 'createdAt'/'modifiedAt'. This is a breaking API change that will cause compilation errors in any existing code using these properties. Either maintain backward compatibility with deprecated aliases or document this as a breaking change.
| var icon: ProjectIcon | ||
| var color: String // Hex color | ||
| public var description: String | ||
| public var tags: Set<String> |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tags changed from Array ([String]) to Set (Set) which is a breaking API change. Existing serialized data with array tags won't deserialize correctly into the new Set type. Consider providing migration logic or maintaining backward compatibility.
| /// Initialise le coffre avec un nouveau mot de passe (première utilisation) | ||
| public func setup(masterPassword: String) async throws { | ||
| logger.log("Initialisation du coffre", level: .info, category: .security) | ||
|
|
||
| // Générer un nouveau sel | ||
| let salt = try KeyDerivationService.generateSalt() | ||
|
|
||
| // Créer les métadonnées | ||
| let metadata = VaultMetadata(salt: salt) | ||
| try await fileStorage.saveMetadata(metadata) | ||
|
|
||
| // Dériver la clé et déverrouiller | ||
| let derivedKey = try await encryptionManager.deriveKey(from: masterPassword, salt: salt) | ||
| masterKey = derivedKey | ||
| isUnlocked = true | ||
|
|
||
| logger.log("Coffre initialisé et déverrouillé", level: .info, category: .security) | ||
| } | ||
|
|
||
| /// Déverrouille le coffre avec le mot de passe maître | ||
| func unlock(masterPassword: String) async throws { | ||
| public func unlock(masterPassword: String) async throws { | ||
| logger.log("Tentative de déverrouillage du coffre", level: .info, category: .security) | ||
|
|
||
| // Dériver la clé maître depuis le mot de passe | ||
| guard let derivedKey = try? await encryptionManager.deriveKey(from: masterPassword) else { | ||
| logger.log("Échec de dérivation de la clé maître", level: .error, category: .security) | ||
| throw VaultError.invalidMasterPassword | ||
| // Charger les métadonnées pour récupérer le sel | ||
| guard let metadata = try await fileStorage.loadMetadata() else { | ||
| logger.log("Métadonnées introuvables. Le coffre doit être initialisé.", level: .error, category: .security) | ||
| throw VaultError.itemNotFound | ||
| } | ||
|
|
||
| // Dériver la clé maître depuis le mot de passe et le sel stocké | ||
| let derivedKey = try await encryptionManager.deriveKey(from: masterPassword, salt: metadata.salt) | ||
|
|
||
| masterKey = derivedKey | ||
| isUnlocked = true | ||
|
|
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VaultManager.setup() and updated unlock() methods introduce a breaking change to the unlock flow. The unlock method now requires metadata with a salt, but there's no migration path for existing vaults that don't have metadata. This will cause existing installations to fail.
No description provided.