Add UnauthorizedError screen for invalid cloud access in Files app #384
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces three new source files: Changes
Poem
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: 2
🧹 Outside diff range and nitpick comments (5)
FileProviderExtensionUI/ReauthenticationViewController.swift (4)
16-31: LGTM: Well-implemented lazy property and initializer.The lazy property for the cell and the custom initializer are well-implemented. Good use of localization for the cell text.
Consider adding a comment explaining why
init(coder:)is unavailable, for better code documentation:/// This view controller is not designed to be initialized from a storyboard @available(*, unavailable) required init?(coder: NSCoder) { fatalError("init(coder:) has not been implemented") }
33-44: LGTM: UI setup and cancellation handling look good.The
viewDidLoadmethod properly sets up the UI elements, and thedonemethod correctly delegates the cancellation action to the coordinator.Consider adding a check for the coordinator's existence in the
donemethod to handle potential nil cases:@objc func done() { coordinator?.userCancelled() ?? print("Warning: Coordinator is nil") }This will help catch any unexpected nil coordinator scenarios during development.
60-70: LGTM: TableView delegate methods are well-implemented.The
UITableViewDelegatemethods are correctly implemented. The header view creation and row selection handling are appropriate.Consider adding a check for the coordinator's existence in the
didSelectRowAtmethod:override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) { tableView.deselectRow(at: indexPath, animated: true) coordinator?.openCryptomatorApp() ?? print("Warning: Coordinator is nil") }This will help catch any unexpected nil coordinator scenarios during development.
82-82: Fix trailing newline.Add a single trailing newline at the end of the file to comply with SwiftLint rules and maintain consistent formatting.
} +🧰 Tools
🪛 SwiftLint
[Warning] 82-82: Files should have a single trailing newline
(trailing_newline)
SharedResources/en.lproj/Localizable.strings (1)
104-104: LGTM! Consider adding a period at the end for consistency.The new localization string for reauthentication is clear, informative, and aligns well with the PR objectives. It provides specific instructions to the user and uses a placeholder for the vault name, which is good for localization.
For consistency with other error messages in this file, consider adding a period at the end of the sentence.
Here's a suggested minor improvement:
-"fileprovider.error.reauthentication" = "The cloud access of your vault \"%@\" needs to be re-authenticated. Open the main app to do so."; +"fileprovider.error.reauthentication" = "The cloud access of your vault \"%@\" needs to be re-authenticated. Open the main app to do so.";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- Cryptomator.xcodeproj/project.pbxproj (4 hunks)
- FileProviderExtensionUI/FileProviderCoordinator.swift (3 hunks)
- FileProviderExtensionUI/ReauthenticationViewController.swift (1 hunks)
- SharedResources/de.lproj/Localizable.strings (1 hunks)
- SharedResources/en.lproj/Localizable.strings (1 hunks)
🧰 Additional context used
🪛 SwiftLint
FileProviderExtensionUI/FileProviderCoordinator.swift
[Warning] 89-89: Lines should not have trailing whitespace
(trailing_whitespace)
FileProviderExtensionUI/ReauthenticationViewController.swift
[Warning] 82-82: Files should have a single trailing newline
(trailing_newline)
🔇 Additional comments (10)
FileProviderExtensionUI/ReauthenticationViewController.swift (4)
1-15: LGTM: Imports and class declaration are well-structured.The imports, class declaration, and property definitions are appropriate for the
ReauthenticationViewController. The weak reference to theFileProviderCoordinatoris a good practice to avoid retain cycles.
46-58: LGTM: TableView data source methods are correctly implemented.The implementation of the
UITableViewDataSourcemethods is correct and efficient for a single-cell table view. Good use of the pre-configuredopenCryptomatorCell.
72-82: LGTM: ReauthenticationHeaderView is well-implemented.The
ReauthenticationHeaderViewclass is correctly implemented as a private inner class. Good use of system images and localized strings for creating a context-specific header view.🧰 Tools
🪛 SwiftLint
[Warning] 82-82: Files should have a single trailing newline
(trailing_newline)
1-82: Overall: Well-implemented reauthentication view controller.The
ReauthenticationViewControlleris well-structured and follows good Swift programming practices. It effectively handles the reauthentication UI for the Cryptomator app. The code is clean, uses appropriate iOS UI patterns, and incorporates localization for better user experience.Minor suggestions were made for improved error handling and documentation, but these are not critical issues. The implementation aligns well with the PR objectives of adding a reauthentication screen for invalid cloud access in the Files app.
Great job on this implementation!
🧰 Tools
🪛 SwiftLint
[Warning] 82-82: Files should have a single trailing newline
(trailing_newline)
SharedResources/en.lproj/Localizable.strings (1)
Line range hint
1-104: Summary: New localization string added for reauthentication scenarioThe changes in this file are minimal and focused. A single new localization string has been added to handle the reauthentication scenario for cloud access in vaults. This addition aligns perfectly with the PR objectives of introducing a new screen for authentication failure.
The new string provides clear instructions to the user and uses proper localization techniques (placeholder for vault name). Its placement in the file is appropriate, and it follows the existing naming conventions.
These changes enhance the user experience by providing clear guidance when reauthentication is needed, which is a valuable improvement to the application.
SharedResources/de.lproj/Localizable.strings (1)
104-104: LGTM! New localization string aligns with PR objectives.The newly added German localization string for reauthentication is well-formed and consistent with the existing localization style. It clearly instructs the user to open the main app to reauthenticate when cloud access becomes invalid, which directly addresses the PR objective of adding a reauthentication screen.
Cryptomator.xcodeproj/project.pbxproj (1)
437-437: LGTM: New ReauthenticationViewController added correctlyThe
ReauthenticationViewController.swiftfile has been properly added to the project. This addition aligns with the PR objective of implementing a reauthentication screen for invalid cloud access in the Files app. The file is correctly included in the build phase and integrated into the project structure.FileProviderExtensionUI/FileProviderCoordinator.swift (3)
19-19: Initialization ofisReauthenticationInProgressThe addition of
isReauthenticationInProgresscorrectly initializes the reauthentication tracking state.
144-147: Proper handling of unauthorized errorThe unauthorized error case is handled appropriately by initiating reauthentication and updating the reauthentication state.
152-154: Guard statement prevents further actions during reauthenticationThe guard statement correctly prevents further actions if reauthentication is already in progress.
There was a problem hiding this comment.
Please also fix the SwiftLint errors that were brought up in the CI action. You can also run these checks locally, see: https://github.com/cryptomator/ios?tab=readme-ov-file#contributing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
FileProviderExtensionUI/FileProviderCoordinatorError.swift (1)
11-13: LGTM: Enum declaration is well-structured. Consider adding documentation.The
FileProviderCoordinatorErrorenum is correctly defined as public and conforms to theErrorprotocol. Theunauthorizedcase with its associatedvaultNamevalue is appropriate for the intended use case.Consider adding documentation comments to describe the enum and its case. This can improve code readability and help generate better documentation. For example:
/// Represents errors that can occur in the FileProviderCoordinator. public enum FileProviderCoordinatorError: Error { /// Indicates that access to a vault is unauthorized. /// - Parameter vaultName: The name of the vault that couldn't be accessed. case unauthorized(vaultName: String) }FileProviderExtensionUI/FileProviderCoordinator.swift (1)
143-144: LGTM: Improved error handling for unauthorized accessThe addition of the
LocalizedCloudProviderError.unauthorizedcase improves error handling and aligns with the PR objectives. Throwing aFileProviderCoordinatorError.unauthorizedwith the vault name is a good approach for triggering the reauthentication screen.Minor suggestion: Consider adding a comment explaining the purpose of this specific error case, especially since
FileProviderCoordinatorErroris likely defined in another file.FileProviderExtensionUI/UnauthorizedErrorViewController.swift (2)
37-37: Renamedonemethod tocancelfor clarity and consistencyThe
doneButtonis initialized withbarButtonSystemItem: .cancel, but the action method is nameddone. For clarity and consistency, consider renaming the method tocancelto match the button's purpose and system item.Apply this change to rename the method and selector:
- let doneButton = UIBarButtonItem(barButtonSystemItem: .cancel, target: self, action: #selector(done)) + let cancelButton = UIBarButtonItem(barButtonSystemItem: .cancel, target: self, action: #selector(cancel))Update the method accordingly:
- @objc func done() { + @objc func cancel() { coordinator?.userCancelled() }Also applies to: 43-44
Line range hint
73-82: Implement required initializer inUnauthorizedErrorHeaderViewThe subclass
UnauthorizedErrorHeaderViewshould implement the required initializerinit?(coder:). Omitting this initializer could lead to compilation errors or runtime issues if the class is ever initialized from a storyboard or nib.Add the following unavailable initializer:
@available(*, unavailable) required init?(coder: NSCoder) { fatalError("init(coder:) has not been implemented") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- Cryptomator.xcodeproj/project.pbxproj (4 hunks)
- FileProviderExtensionUI/FileProviderCoordinator.swift (2 hunks)
- FileProviderExtensionUI/FileProviderCoordinatorError.swift (1 hunks)
- FileProviderExtensionUI/UnauthorizedErrorViewController.swift (1 hunks)
- SharedResources/en.lproj/Localizable.strings (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- SharedResources/en.lproj/Localizable.strings
🧰 Additional context used
🔇 Additional comments (10)
FileProviderExtensionUI/FileProviderCoordinatorError.swift (1)
1-10: LGTM: File header and imports are appropriate.The file header contains all the necessary information, and the Foundation import is correct for defining an error type in Swift.
FileProviderExtensionUI/FileProviderCoordinator.swift (2)
83-87: LGTM: New method for showing unauthorized error screenThe
showUnauthorizedError(vaultName:)method is well-implemented. It correctly creates and presents anUnauthorizedErrorViewController, which aligns with the PR objective of introducing a reauthentication screen for invalid cloud access.
Line range hint
1-204: Overall assessment: Well-implemented reauthentication functionalityThe changes in this file successfully implement the new reauthentication screen functionality for invalid cloud access in the Files app. The additions and modifications align well with the PR objectives and follow good coding practices. The error handling has been improved to address unauthorized access scenarios, which should enhance the user experience.
Cryptomator.xcodeproj/project.pbxproj (3)
437-438: New file added: UnauthorizedErrorViewController.swiftThe
UnauthorizedErrorViewController.swiftfile has been correctly added to the project. It is included in thePBXBuildFilesection with a unique identifier, which is the standard way of adding new files to an Xcode project.
438-439: New file added: FileProviderCoordinatorError.swiftThe
FileProviderCoordinatorError.swiftfile has been properly added to the project. It is also included in thePBXBuildFilesection with a unique identifier, following the correct procedure for adding new files to an Xcode project.
437-439: Summary of changesThe project file has been updated to include two new Swift files:
UnauthorizedErrorViewController.swiftandFileProviderCoordinatorError.swift. These additions are consistent with the AI-generated summary and appear to be correctly integrated into the project structure. No other significant changes or inconsistencies were found in the project file.These new files likely introduce functionality for handling unauthorized errors and defining error types for the file provider coordinator, which aligns with the PR objective of adding a reauthentication screen for invalid cloud access in the Files app.
FileProviderExtensionUI/UnauthorizedErrorViewController.swift (4)
13-13: Use ofweakreference forcoordinatorprevents retain cyclesDeclaring
coordinatoras aweakvariable appropriately prevents retain cycles between the view controller and the coordinator.
36-36: Verify that long vault names display correctly in the navigation barSetting
title = vaultNamemay cause layout issues ifvaultNameis excessively long. Ensure that long vault names are properly truncated or adjusted to fit within the navigation bar without affecting the UI layout.
76-76: Ensure color contrast meets accessibility standardsThe
symbolImageuses.systemYellow, which might not provide sufficient contrast against certain background colors. Verify that the color contrast meets accessibility guidelines to enhance readability for all users.You can use accessibility inspection tools to check the contrast ratio.
78-78: Verify localized string formattingEnsure that the localized string for key
"fileprovider.error.reauthentication"includes the correct format specifier (e.g.,%@) to properly insert thevaultName. This prevents runtime issues and ensures the message displays correctly in all languages.
…ing and update localization strings
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
FileProviderExtensionUI/UnauthorizedErrorViewController.swift (1)
46-70: LGTM: Table view methods are well-implemented.The table view data source and delegate methods are correctly implemented for a single-section, single-row table view. The code is clear and concise.
Consider grouping the
UITableViewDataSourceandUITableViewDelegatemethods together under a singleMARK:comment for better code organization:// MARK: - UITableViewDataSource & UITableViewDelegate override func numberOfSections(in tableView: UITableView) -> Int { // ... } // ... (other methods) override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) { // ... }This grouping can improve readability, especially as the file grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- FileProviderExtensionUI/FileProviderCoordinator.swift (3 hunks)
- FileProviderExtensionUI/UnauthorizedErrorViewController.swift (1 hunks)
- SharedResources/en.lproj/Localizable.strings (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- FileProviderExtensionUI/FileProviderCoordinator.swift
- SharedResources/en.lproj/Localizable.strings
🧰 Additional context used
🔇 Additional comments (3)
FileProviderExtensionUI/UnauthorizedErrorViewController.swift (3)
1-11: LGTM: File header and imports are appropriate.The file header contains the necessary copyright information, and the imports are relevant for the functionality implemented in this file.
1-81: Summary: Solid implementation with room for minor enhancementsOverall, the
UnauthorizedErrorViewControllerandUnauthorizedErrorHeaderViewclasses are well-implemented and fulfill their purpose of handling unauthorized access errors. The code is clear, concise, and follows good practices for iOS development.Main points for improvement:
- Implement dynamic type support for better accessibility, especially in the
UnauthorizedErrorHeaderView.- Extract string literals to constants or enums for improved maintainability.
- Consider grouping table view methods under a single MARK comment for better code organization.
These enhancements will further improve the code quality and user experience. Great job on the implementation!
72-81: 🛠️ Refactor suggestionEnsure consistency with DetectedVaultFailureView and implement dynamic type support.
The
UnauthorizedErrorHeaderViewimplementation looks good overall, but there are two points to address:
Consistency: A previous comment noted that
DetectedVaultFailureViewuses a point size of 120. This class is now consistent with that, which is good.Dynamic Type Support: As mentioned in a previous comment, implement dynamic type support for better accessibility.
To address both points, update the
initmethod as follows:init(vaultName: String) { let basePointSize: CGFloat = 120 let scaledPointSize = UIFontMetrics.default.scaledValue(for: basePointSize) let config = UIImage.SymbolConfiguration(pointSize: scaledPointSize) let symbolImage = UIImage(systemName: "exclamationmark.triangle.fill", withConfiguration: config)? .withTintColor(.systemYellow, renderingMode: .alwaysOriginal) let infoText = String(format: LocalizedString.getValue("fileprovider.error.unauthorized.text"), vaultName) super.init(image: symbolImage, infoText: infoText) }This change maintains consistency with
DetectedVaultFailureViewwhile also supporting dynamic type.To ensure consistency across the app, let's check if
DetectedVaultFailureViewalso needs this update:#!/bin/bash # Search for DetectedVaultFailureView implementation rg --type swift 'class DetectedVaultFailureView'
tobihagemann
changed the title
This feature introduces a screen that is displayed when using the files app and the authentication fails for a cloud provider. The screen informs the user that they need to return to the main app to reauthenticate in order to regain access to the vault. This enhancement was implemented in response to #18.