Review errors string on E2EE + refactoring (clear old code)#4050
Conversation
Signed-off-by: Marino Faggiana <marino.faggiana@nextcloud.com>
Signed-off-by: Marino Faggiana <marino.faggiana@nextcloud.com>
Signed-off-by: Marino Faggiana <marino.faggiana@nextcloud.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors end-to-end encryption (E2EE) error handling by replacing the generic _e2e_error_ string with more specific localized error messages, updates some E2EE flow logic (including folder creation metadata updates), and introduces a new indeterminate HUD banner view for long-running operations.
Changes:
- Replace generic E2EE error strings with granular
_e2ee_*localized messages and update call sites accordingly. - Refactor E2EE create-folder flow to update/upload metadata and optionally show a blocking indeterminate HUD banner.
- Adjust E2EE version handling (support 2.2, broaden version matching), and add a new SwiftUI/LucidBanner HUD view.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Nextcloud.xcodeproj/project.pbxproj | Adds new HUD view source file and changes NextcloudKit dependency requirement to a branch. |
| iOSClient/Supporting Files/en.lproj/Localizable.strings | Removes _e2e_error_ and adds specific _e2ee_* error strings plus a create-folder progress string. |
| iOSClient/Networking/E2EE/NCNetworkingE2EEUpload.swift | Replaces generic E2EE error string with specific localized error keys. |
| iOSClient/Networking/E2EE/NCNetworkingE2EERename.swift | Uses localized _e2ee_no_dir_ instead of generic error key. |
| iOSClient/Networking/E2EE/NCNetworkingE2EEDelete.swift | Uses localized _e2ee_no_dir_ instead of generic error key. |
| iOSClient/Networking/E2EE/NCNetworkingE2EECreateFolder.swift | Refactors folder creation, adds HUD handling, and extracts metadata update logic. |
| iOSClient/Networking/E2EE/NCNetworkingE2EE.swift | Broadens E2EE API version matching and localizes directory-not-found error. |
| iOSClient/Networking/E2EE/NCEndToEndMetadataV20.swift | Improves/standardizes E2EE v2 metadata encode/decode errors and signature handling; updates version fields. |
| iOSClient/Networking/E2EE/NCEndToEndMetadataV1.swift | Uses more specific localized decode/checksum error strings. |
| iOSClient/Networking/E2EE/NCEndToEndMetadata.swift | Updates directory-not-found error string and changes decode error messaging. |
| iOSClient/NCGlobal.swift | Adds E2EE compatibility for 2.2 and broadens “version 2” detection. |
| iOSClient/Menu/NCContextMenuMain.swift | Removes a hard-coded _e2e_error_ title argument when showing an error banner. |
| iOSClient/Main/Collection Common/NCCollectionViewCommon.swift | Changes post-transfer behavior for E2EE items to reload instead of pushing a single metadata item. |
| iOSClient/GUI/Lucid Banner/HudIndeterminateBannerView.swift | Adds a new SwiftUI indeterminate HUD banner implementation using LucidBanner. |
| iOSClient/Files/NCFiles.swift | Refactors folder read flow and E2EE metadata handling (includes a compile-breaking regression). |
| iOSClient/Data/NCManageDatabase+E2EE.swift | Changes stored v2 metadata version default from “2.0” to “2”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| branch = "e2ee-lowercased"; | ||
| kind = branch; |
There was a problem hiding this comment.
SwiftPM dependency for NextcloudKit was changed from a pinned version to a branch ("e2ee-lowercased"). Using a moving branch makes builds non-reproducible and can unexpectedly break CI/releases; prefer pinning to an exact tag/semantic version or a specific revision/commit hash (if a prerelease is needed).
| branch = "e2ee-lowercased"; | |
| kind = branch; | |
| kind = revision; | |
| revision = 0123456789abcdef0123456789abcdef01234567; |
| if let metadataFolder { | ||
| self.metadataFolder = metadataFolder.detachedCopy() | ||
| self.richWorkspaceText = metadataFolder.richWorkspace | ||
| } |
There was a problem hiding this comment.
metadataFolder is referenced here but is no longer defined after switching to resultsReadFolder; this should use resultsReadFolder.metadataFolder (or destructure the tuple) before accessing it, otherwise the file won’t compile.
| guard let directory = await self.database.getTableDirectoryAsync(predicate: NSPredicate(format: "account == %@ AND serverUrl == %@", session.account, serverUrl)) else { | ||
| return NKError(errorCode: global.errorUnexpectedResponseFromDB, errorDescription: NSLocalizedString("_e2e_error_", comment: "")) | ||
| error = NKError(errorCode: global.errorUnexpectedResponseFromDB, | ||
| errorDescription: NSLocalizedString("_e2ee_no_dir_", comment: "")) |
There was a problem hiding this comment.
The guard failure for fileNameFolder.isEmpty returns _e2ee_no_dir_ ("cannot get directory"), but this branch is about an invalid/empty folder name rather than a missing directory. Use a more accurate localized error key/message (and ideally a more appropriate error code) for the empty-name case.
| errorDescription: NSLocalizedString("_e2ee_no_dir_", comment: "")) | |
| errorDescription: NSLocalizedString("_e2ee_invalid_folder_name_", comment: "")) |
| defer { | ||
| if let banner, let token { | ||
| if error == .success { | ||
| completeHudIndeterminateBannerSuccess(token: token, banner: banner) | ||
| } else { | ||
| banner.dismiss() | ||
| } | ||
| } |
There was a problem hiding this comment.
In the defer block, dismissal is gated on token being non-nil (if let banner, let token), but banner.dismiss() does not use token. If token is unexpectedly nil, the HUD could remain visible; consider unwrapping only banner for dismissal, and only require token when calling the completion/update APIs that need it.
| NCEndToEndEncryption.shared().encodedkey(&key, initializationVector: &initializationVector) | ||
| guard let key = key as? String, let initializationVector = initializationVector as? String else { | ||
| return NKError(errorCode: global.errorE2EEEncodedKey, | ||
| errorDescription: NSLocalizedString("_e2ee_no_generate_key_", comment: "")) |
There was a problem hiding this comment.
encodedkey(&key, initializationVector: ...) failure is reported with _e2ee_no_generate_key_, but the error code is errorE2EEEncodedKey and the failure is about missing/invalid encoded key/IV (not key generation). This should use the corresponding encoded-key localized message (e.g. _e2ee_no_enc_key_) to avoid confusing users.
| errorDescription: NSLocalizedString("_e2ee_no_generate_key_", comment: "")) | |
| errorDescription: NSLocalizedString("_e2ee_no_enc_key_", comment: "")) |
| guard let directory = self.database.getTableDirectory(predicate: NSPredicate(format: "account == %@ AND serverUrl == %@", session.account, serverUrl)) else { | ||
| return (nil, nil, 0, NKError(errorCode: NCGlobal.shared.errorUnexpectedResponseFromDB, errorDescription: "_e2e_error_")) | ||
| return (nil, nil, 0, NKError(errorCode: NCGlobal.shared.errorUnexpectedResponseFromDB, | ||
| errorDescription: NSLocalizedString("_e2ee_no_session_", comment: ""))) |
There was a problem hiding this comment.
When the directory lookup fails in encodeMetadata, the code returns _e2ee_no_session_ even though the failing condition is "no directory for (account, serverUrl)". This error message should match the actual failure (likely _e2ee_no_dir_) to avoid misleading diagnostics.
| errorDescription: NSLocalizedString("_e2ee_no_session_", comment: ""))) | |
| errorDescription: NSLocalizedString("_e2ee_no_dir_", comment: ""))) |
| func decodeMetadata(_ metadata: String, signature: String?, serverUrl: String, session: NCSession.Session) async -> NKError { | ||
| guard let data = metadata.data(using: .utf8), let directory = self.database.getTableDirectory(predicate: NSPredicate(format: "account == %@ AND serverUrl == %@", session.account, serverUrl)) else { | ||
| return (NKError(errorCode: NCGlobal.shared.errorE2EEJSon, errorDescription: "_e2e_error_")) | ||
| return (NKError(errorCode: NCGlobal.shared.errorE2EEJSon, errorDescription: "Unable to decode the metadata file")) | ||
| } | ||
|
|
There was a problem hiding this comment.
decodeMetadata returns a hard-coded English error description ("Unable to decode the metadata file"). Since this is user-facing and the rest of the E2EE errors are localized, this should use a NSLocalizedString key (and ideally reuse one of the new _e2ee_* keys) for consistent localization.
|
|
||
| // -------------------------------------------------------------------------------------------- | ||
| // MARK: Ecode JSON Metadata V2.0 | ||
| // MARK: Ecode JSON Metadata V2 |
There was a problem hiding this comment.
Spelling: the section header says "Ecode JSON Metadata V2"; this looks like a typo for "Encode".
| // MARK: Ecode JSON Metadata V2 | |
| // MARK: Encode JSON Metadata V2 |
No description provided.