Improve Hub error handling and updates #404
Conversation
WalkthroughThe pull request introduces several changes across multiple files in the Cryptomator iOS application, focusing on enhancing the Hub authentication and vault management flow. The primary modifications include adding support for handling archived vaults, updating the authentication view models and views, and refining localization strings. Key changes involve introducing a new Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift (1)
Line range hint
51-77: Consider refactoring for better readability while maintaining the current functionality.The changes correctly implement the removal of biometric authentication options for Hub vaults. However, the code could be more readable with a functional programming approach.
Consider this refactor to improve readability:
private static func getTitleText(vaultUnlocked: Bool, biometricalUnlockEnabled: Bool, biometryTypeName: String?, keepUnlockedDuration: KeepUnlockedDuration, vaultInfo: VaultInfo) -> String { - let unlockedText: String - if vaultUnlocked { - unlockedText = LocalizedString.getValue("vaultDetail.unlocked.footer") - } else { - unlockedText = LocalizedString.getValue("vaultDetail.locked.footer") - } - let keepUnlockedText: String - switch keepUnlockedDuration { - case .auto: - keepUnlockedText = LocalizedString.getValue("vaultDetail.keepUnlocked.footer.off") - case .indefinite: - keepUnlockedText = LocalizedString.getValue("vaultDetail.keepUnlocked.footer.unlimitedDuration") - case .fiveMinutes, .tenMinutes, .thirtyMinutes, .oneHour: - keepUnlockedText = String(format: LocalizedString.getValue("vaultDetail.keepUnlocked.footer.limitedDuration"), keepUnlockedDuration.description ?? "") - } - var footerText = "\(unlockedText)\n\n\(keepUnlockedText)" - if vaultInfo.vaultConfigType != .hub, let biometryTypeName = biometryTypeName { - let biometricalUnlockText: String - if biometricalUnlockEnabled { - biometricalUnlockText = String(format: LocalizedString.getValue("vaultDetail.enabledBiometricalUnlock.footer"), biometryTypeName) - } else { - biometricalUnlockText = String(format: LocalizedString.getValue("vaultDetail.disabledBiometricalUnlock.footer"), biometryTypeName) - } - footerText += "\n\n\(biometricalUnlockText)" - } - return footerText + let unlockedText = vaultUnlocked + ? LocalizedString.getValue("vaultDetail.unlocked.footer") + : LocalizedString.getValue("vaultDetail.locked.footer") + + let keepUnlockedText = { + switch keepUnlockedDuration { + case .auto: + return LocalizedString.getValue("vaultDetail.keepUnlocked.footer.off") + case .indefinite: + return LocalizedString.getValue("vaultDetail.keepUnlocked.footer.unlimitedDuration") + case .fiveMinutes, .tenMinutes, .thirtyMinutes, .oneHour: + return String(format: LocalizedString.getValue("vaultDetail.keepUnlocked.footer.limitedDuration"), + keepUnlockedDuration.description ?? "") + } + }() + + let biometricText = vaultInfo.vaultConfigType != .hub && biometryTypeName != nil + ? "\n\n" + String(format: LocalizedString.getValue(biometricalUnlockEnabled + ? "vaultDetail.enabledBiometricalUnlock.footer" + : "vaultDetail.disabledBiometricalUnlock.footer"), + biometryTypeName!) + : "" + + return "\(unlockedText)\n\n\(keepUnlockedText)\(biometricText)" }This refactoring:
- Uses ternary operators for simpler conditional text selection
- Isolates the keep-unlocked text logic in a closure
- Combines the biometric text generation into a single expression
- Reduces nesting and variable declarations
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubErrorWithRefreshView.swift (3)
3-6: Add documentation to improve code maintainability.Consider adding documentation comments to describe the view's purpose and its properties. This will help other developers understand how to use this view correctly.
+/// A view that displays an error message with a refresh button. struct CryptomatorErrorWithRefreshView: View { + /// The title to display in the header of the error view. var headerTitle: String + /// A closure that is called when the refresh button is tapped. var onRefresh: () -> Void
7-13: Consider adding error handling for the refresh action.Since this is an error view that handles Hub vault access, the refresh action might fail. Consider adding error handling and a loading state.
struct CryptomatorErrorWithRefreshView: View { var headerTitle: String var onRefresh: () -> Void + @State private var isLoading: Bool = false var body: some View { CryptomatorSimpleButtonView( buttonTitle: LocalizedString.getValue("common.button.refresh"), - onButtonTap: onRefresh, + onButtonTap: { + isLoading = true + Task { + defer { isLoading = false } + onRefresh() + } + }, headerTitle: headerTitle ) + .disabled(isLoading) } }
16-20: Enhance preview provider for better development experience.Consider adding more preview cases to showcase different states and color schemes.
struct CryptomatorErrorWithRefreshView_Previews: PreviewProvider { static var previews: some View { - CryptomatorErrorWithRefreshView(headerTitle: "Example Header Title", onRefresh: {}) + Group { + CryptomatorErrorWithRefreshView( + headerTitle: "Vault Access Error", + onRefresh: {} + ) + .previewDisplayName("Light Mode") + + CryptomatorErrorWithRefreshView( + headerTitle: "Network Error", + onRefresh: {} + ) + .preferredColorScheme(.dark) + .previewDisplayName("Dark Mode") + } } }CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift (1)
246-249: Consider refactoring to reduce complexity.The status code handling section has triggered a cyclomatic complexity warning. Consider extracting the status code handling into a separate method.
private func getVaultMasterKey(vaultBaseURL: URL, authState: OIDAuthState, webAppURL: URL) async throws -> RetrieveVaultMasterkeyEncryptedForUserResponse { // ... existing code ... - switch httpResponse?.statusCode { - case 200: - guard let body = String(data: data, encoding: .utf8) else { - throw CryptomatorHubAuthenticatorError.unexpectedResponse - } - return .success(encryptedVaultKey: body, header: httpResponse?.allHeaderFields ?? [:]) - case 402: - return .licenseExceeded - case 403: - return .accessNotGranted - case 410: - return .vaultArchived - case 404: - return .legacyHubVersion - case 449: - let profileURL = webAppURL.appendingPathComponent("profile") - return .requiresAccountInitialization(at: profileURL) - default: - throw CryptomatorHubAuthenticatorError.unexpectedResponse - } + return try handleVaultMasterKeyResponse(data: data, + statusCode: httpResponse?.statusCode, + headers: httpResponse?.allHeaderFields, + webAppURL: webAppURL) } + +private func handleVaultMasterKeyResponse(data: Data, + statusCode: Int?, + headers: [AnyHashable: Any]?, + webAppURL: URL) throws -> RetrieveVaultMasterkeyEncryptedForUserResponse { + switch statusCode { + case 200: + guard let body = String(data: data, encoding: .utf8) else { + throw CryptomatorHubAuthenticatorError.unexpectedResponse + } + return .success(encryptedVaultKey: body, header: headers ?? [:]) + case 402: + return .licenseExceeded + case 403: + return .accessNotGranted + case 410: + return .vaultArchived + case 404: + return .legacyHubVersion + case 449: + let profileURL = webAppURL.appendingPathComponent("profile") + return .requiresAccountInitialization(at: profileURL) + default: + throw CryptomatorHubAuthenticatorError.unexpectedResponse + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift(3 hunks)Cryptomator/VaultDetail/VaultDetailViewModel.swift(2 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift(4 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationViewModel.swift(2 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubDeviceRegisteredSuccessfullyView.swift(0 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubErrorWithRefreshView.swift(1 hunks)SharedResources/en.lproj/Localizable.strings(1 hunks)
💤 Files with no reviewable changes (1)
- CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubDeviceRegisteredSuccessfullyView.swift
🧰 Additional context used
🪛 GitHub Actions: Build
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift
[warning] 52-52: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 11
🔇 Additional comments (15)
Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift (3)
9-9: LGTM! The new imports and property addition look good.The addition of the
vaultInfoproperty aligns with the PR objective of improving Hub vault handling.Also applies to: 35-35
37-44: LGTM! Constructor changes are well-implemented.The constructor properly initializes the new
vaultInfoproperty and maintains a clean initialization order.
47-49: LGTM! Title update method changes are consistent.The
updateTitlemethod correctly passes thevaultInfoparameter to maintain consistency with the new requirements.CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubErrorWithRefreshView.swift (2)
7-13: LGTM! Clean and focused implementation.The view follows SwiftUI best practices with a clear single responsibility.
1-20: Implementation aligns well with PR objectives.This view effectively supports Issue #398 by providing a clear error message and refresh capability for archived Hub vaults. The implementation is clean, reusable, and follows SwiftUI best practices.
Let's verify the localization string usage:
✅ Verification successful
Localization implementation verified successfully
The
common.button.refreshstring key is properly implemented and available across all 46 supported languages in the application's localization files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the localization key exists and has appropriate translations rg -g '*.strings' -l "common.button.refresh"Length of output: 2134
Cryptomator/VaultDetail/VaultDetailViewModel.swift (2)
124-127: LGTM! Biometric authentication correctly disabled for Hub vaults.The implementation properly removes the biometric authentication option for Hub vaults while maintaining it for other vault types.
Let's verify that biometric authentication is consistently handled for Hub vaults across the codebase:
✅ Verification successful
Verified: Biometric authentication is consistently disabled for Hub vaults
The implementation correctly and consistently handles the removal of biometric authentication for Hub vaults across both UI components:
- VaultDetailViewModel: Removes the biometric switch cell
- UnlockSectionFooterViewModel: Removes the biometric authentication text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining biometric authentication code paths for Hub vaults # that might have been missed. # Search for biometric/Face ID/Touch ID related code near Hub vault conditions rg -l 'hub.*bio|bio.*hub|FaceID.*hub|TouchID.*hub' --type swift # Search for potential biometric authentication implementations ast-grep --pattern 'if $COND { $$$ biometric$$$ $$$ }'Length of output: 238
Script:
#!/bin/bash # Description: Inspect the UnlockSectionFooterViewModel implementation # Show the content of UnlockSectionFooterViewModel.swift with context rg -A 5 -B 5 "bio|FaceID|TouchID" Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift # Look for Hub-related conditions rg "hub" Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swiftLength of output: 2676
149-149: Verify UnlockSectionFooterViewModel implementation.The
vaultInfoparameter has been added to the footer view model initialization. Please ensure thatUnlockSectionFooterViewModelproperly handles this new parameter to display appropriate text based on the vault type.Let's verify the implementation:
✅ Verification successful
Implementation verified and properly handles vaultInfo parameter
The
UnlockSectionFooterViewModelcorrectly stores and utilizes thevaultInfoparameter to customize the footer text display. The implementation includes proper initialization and title text generation based on the vault information.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check UnlockSectionFooterViewModel implementation # Look for UnlockSectionFooterViewModel class definition and its usage of vaultInfo ast-grep --pattern 'class UnlockSectionFooterViewModel { $$$ }' # Search for any text/message customization based on vault type rg -A 5 'UnlockSectionFooterViewModel.*init.*vaultInfo|func.*message.*vault'Length of output: 146
Script:
#!/bin/bash # First, find files that might contain UnlockSectionFooterViewModel fd -e swift . | xargs rg -l "UnlockSectionFooterViewModel" # Then, get the content of files containing UnlockSectionFooterViewModel rg -B 5 -A 10 "class UnlockSectionFooterViewModel|init.*vaultInfo|UnlockSectionFooterViewModel.*init" # Broader AST pattern to catch any class definition containing UnlockSectionFooterViewModel ast-grep --pattern 'class $_ { init($$$) { $$$ } $$$ }'Length of output: 10269
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift (1)
23-25: LGTM! Consistent error handling implementation.The changes improve error handling by:
- Using a consistent
CryptomatorErrorWithRefreshViewfor both error states- Providing refresh capability for recovery
- Using localized strings for user-friendly messages
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationViewModel.swift (2)
35-35: LGTM! Clean state management addition.The new
vaultArchivedcase follows the existing pattern and Swift naming conventions.
112-113: LGTM! Proper state transition handling.The state transition for archived vaults is handled consistently with other states.
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift (3)
23-23: LGTM! New authentication flow state.The
vaultArchivedcase is a clean addition to the authentication flow states.
83-84: LGTM! Proper handling of archived vault response.Clean implementation of the archived vault state transition.
313-314: LGTM! Clear documentation of HTTP status code mapping.The comment clearly indicates that the
vaultArchivedcase corresponds to HTTP 410 status.SharedResources/en.lproj/Localizable.strings (2)
119-119: LGTM! Improved clarity in error message.The message now correctly refers to "user" instead of "device" authorization, aligning with Hub version 1.3.0 changes.
129-129: LGTM! Clear message for archived vaults.The message clearly explains the vault state and provides actionable guidance to users.
| "hubAuthentication.licenseExceeded" = "Your Cryptomator Hub instance has an invalid license. Please inform a Hub administrator to upgrade or renew the license."; | ||
| "hubAuthentication.deviceRegistration.deviceName.cells.name" = "Device Name"; | ||
| "hubAuthentication.deviceRegistration.deviceName.footer.title" = "This seems to be the first Hub access from this device. In order to identify it for access authorization, you need to name this device."; | ||
| "hubAuthentication.deviceRegistration.accountKey.footer.title" = "Your Account Key is required to login from new apps or browsers. It can be found in your profile."; | ||
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.title" = "Register Device Successful"; | ||
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault, your device needs to be authorized by the vault owner."; | ||
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault"; |
There was a problem hiding this comment.
💡 Codebase verification
English translation is incomplete and missing crucial information
The English alert message is truncated and missing essential information that is present in all other language translations. The message should include that the device needs authorization from the vault owner, similar to other language versions.
🔗 Analysis chain
Verify if the message is incomplete.
The alert message appears to be truncated. Please verify if this is intentional or if some text is missing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar alert messages to verify the pattern
rg "alert\.message.*=.*\"To.*\"" -g "*.strings"
Length of output: 985
Script:
#!/bin/bash
# Search for hubAuthentication related messages across all language files
rg "hubAuthentication.*\.alert\.message.*=.*\".*\"" -g "*.strings"
Length of output: 12625
There was a problem hiding this comment.
I agree that this change feels incomplete. @iammajid What was your intention? Maybe something like this?
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault"; | |
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault, the vault owner needs to grant you permission."; |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Apart from my other comments, you may have seen that the CI build has failed due to this SwiftLint warning:
CryptomatorHubAuthenticator.swift:52:9: warning: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 11 (cyclomatic_complexity)
I don't see a problem with the complexity, so you can use // swiftlint:disable:next cyclomatic_complexity to disable the warning.
| "hubAuthentication.licenseExceeded" = "Your Cryptomator Hub instance has an invalid license. Please inform a Hub administrator to upgrade or renew the license."; | ||
| "hubAuthentication.deviceRegistration.deviceName.cells.name" = "Device Name"; | ||
| "hubAuthentication.deviceRegistration.deviceName.footer.title" = "This seems to be the first Hub access from this device. In order to identify it for access authorization, you need to name this device."; | ||
| "hubAuthentication.deviceRegistration.accountKey.footer.title" = "Your Account Key is required to login from new apps or browsers. It can be found in your profile."; | ||
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.title" = "Register Device Successful"; | ||
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault, your device needs to be authorized by the vault owner."; | ||
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault"; |
There was a problem hiding this comment.
I agree that this change feels incomplete. @iammajid What was your intention? Maybe something like this?
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault"; | |
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault, the vault owner needs to grant you permission."; |
| @@ -116,16 +116,17 @@ | |||
| "getFolderIntent.error.noVaultSelected" = "No vault has been selected."; | |||
|
|
|||
| "hubAuthentication.title" = "Hub Vault"; | |||
| "hubAuthentication.accessNotGranted" = "Your device has not yet been authorized to access this vault. Ask the vault owner to authorize it."; | |||
| "hubAuthentication.accessNotGranted" = "Your user has not yet been authorized to access this vault. Ask the vault owner to authorize it."; | |||
There was a problem hiding this comment.
I know that you just followed a suggestion, but I believe this can be improved. Maybe something like this?
| "hubAuthentication.accessNotGranted" = "Your user has not yet been authorized to access this vault. Ask the vault owner to authorize it."; | |
| "hubAuthentication.accessNotGranted" = "You do not have permission to access this vault. Ask the vault owner to authorize you."; |
This PR fixes the following issues:
#398 : Show a clear error message for archived Hub vaults (HTTP 410).
#397 : Update texts to align with Hub 1.3.0 (user keys).
#396 : Remove biometric authentication options for Hub vaults.
Changes
• Added HTTP 410 error handling for archived vaults.
• Updated localized strings for user authorization texts.
• Removed Face ID/Touch ID options for Hub vaults.