Add offline fallback for folder enumeration and auto-retry failed uploads#446
Open
tobihagemann wants to merge 4 commits intodevelopfrom
Open
Add offline fallback for folder enumeration and auto-retry failed uploads#446tobihagemann wants to merge 4 commits intodevelopfrom
tobihagemann wants to merge 4 commits intodevelopfrom
Conversation
WalkthroughMoves an Xcode project group (Hub) from Settings to Purchase. Introduces error-classification helpers ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swift (1)
129-147: Consider logging when offline fallback fails due to cache errors.The catch block at line 145-146 returns the caught error (e.g., database error) without logging, which could make debugging difficult when offline fallback silently fails. The success path logs at line 143, but failure paths don't.
💡 Proposed logging for fallback failures
} catch { + DDLogError("Offline fallback failed for folder \(folderMetadata.cloudPath.path): \(error)") return Promise(error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swift` around lines 129 - 147, In buildOfflineItemList, add a log statement in the catch block to record the caught error and context before returning Promise(error); specifically use DDLogError (or the existing DDLogInfo style) to include folderMetadata.id and/or folderMetadata.cloudPath.path, the caught error, and originalError so cache/database failures during the offline fallback are visible; place this logging immediately before the current return Promise(error) in the catch clause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@CryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swift`:
- Around line 129-147: In buildOfflineItemList, add a log statement in the catch
block to record the caught error and context before returning Promise(error);
specifically use DDLogError (or the existing DDLogInfo style) to include
folderMetadata.id and/or folderMetadata.cloudPath.path, the caught error, and
originalError so cache/database failures during the offline fallback are
visible; place this logging immediately before the current return Promise(error)
in the catch clause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c92c9abc-0ae3-446c-b7bc-35140f954feb
📒 Files selected for processing (6)
Cryptomator.xcodeproj/project.pbxprojCryptomatorFileProvider/FileProviderAdapter.swiftCryptomatorFileProvider/Middleware/ErrorMapper.swiftCryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterEnumerateItemTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swift (1)
107-116: Consider removing this test as it duplicatestestRecoverStuckUploads_noActiveUploads.This test (
testRecoverStuckUploads_noRetryableUploads) has identical setup and nearly identical assertions totestRecoverStuckUploads_noActiveUploads(lines 20-29). Both test the "no uploads to process" scenario with empty return values for both mock methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swift` around lines 107 - 116, Remove the duplicate test method testRecoverStuckUploads_noRetryableUploads from CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swift because it duplicates testRecoverStuckUploads_noActiveUploads; locate the function named testRecoverStuckUploads_noRetryableUploads (which calls adapter.recoverStuckUploads and asserts on uploadTaskManagerMock.getActiveUploadTaskRecordsCallsCount, getRetryableUploadTaskRecordsCallsCount, and createNewTaskRecordForCalled) and delete that entire test block to avoid redundant coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swift`:
- Around line 107-116: Remove the duplicate test method
testRecoverStuckUploads_noRetryableUploads from
CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swift
because it duplicates testRecoverStuckUploads_noActiveUploads; locate the
function named testRecoverStuckUploads_noRetryableUploads (which calls
adapter.recoverStuckUploads and asserts on
uploadTaskManagerMock.getActiveUploadTaskRecordsCallsCount,
getRetryableUploadTaskRecordsCallsCount, and createNewTaskRecordForCalled) and
delete that entire test block to avoid redundant coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eeb4e3de-74e1-4a0d-8f78-5ff6e20e5eb8
📒 Files selected for processing (4)
CryptomatorFileProvider/DB/UploadTaskDBManager.swiftCryptomatorFileProvider/FileProviderAdapter.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swiftCryptomatorFileProviderTests/Mocks/UploadTaskManagerMock.swift
tobihagemann
changed the title
… errors, and add offline-fallback tests
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@CryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swift`:
- Around line 129-132: The current logic in ItemEnumerationTaskExecutor
conflates an empty cached child list with "never enumerated" by relying on
cachedMetadata.isEmpty; modify the check to use an explicit sentinel on the
folder metadata (e.g., an "enumeratedOnce" or "lastEnumeratedAt" flag
stored/returned by itemMetadataManager) instead of child count. Update
itemMetadataManager.getCachedMetadata usage and the guard so that if the folder
has been previously enumerated (enumeratedOnce == true or lastEnumeratedAt !=
nil) you treat an empty cachedMetadata as a valid empty-folder result
(resolve/succeed) rather than returning Promise(originalError), and only fall
back to originalError when the sentinel indicates the folder was never
enumerated. Ensure you reference and update the folder metadata field
(folderMetadata.id and the new sentinel) and adjust any places that set the
sentinel when an online enumeration completes.
- Around line 133-138: The offline path currently builds items directly from
cachedMetadata without applying the reconciliation steps used by the online
path; update the offline branch in ItemEnumerationTaskExecutor so you first run
cachedMetadata through filterOutWaitingReparentTasks(...),
filterOutWaitingDeletionTasks(...), getReparentMetadata(...), and
getPlaceholderMetadata(...) (the same pipeline used for the online path), then
obtain upload task records for that reconciled metadata (call
uploadTaskManager.getTaskRecords(for: reconciledMetadata) or map tasks by
metadata identifier) and finally map the reconciled metadata to FileProviderItem
using those task records so indices and error states remain consistent with the
online enumeration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da12ee66-8e4f-453c-8cb0-0eb4a724db31
📒 Files selected for processing (10)
CryptomatorFileProvider/DB/WorkingSetObserver.swiftCryptomatorFileProvider/FileProviderAdapter.swiftCryptomatorFileProvider/FileProviderItem.swiftCryptomatorFileProvider/Middleware/ErrorMapper.swiftCryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/ReparentTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/UploadTaskExecutor.swiftCryptomatorFileProviderTests/DB/UploadTaskManagerTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterStartProvidingItemTests.swiftCryptomatorFileProviderTests/Middleware/ErrorMapperTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- CryptomatorFileProvider/Middleware/ErrorMapper.swift
Comment on lines
+129
to
+132
| let cachedMetadata = try itemMetadataManager.getCachedMetadata(withParentID: folderMetadata.id!) | ||
| guard !cachedMetadata.isEmpty else { | ||
| return Promise(originalError) | ||
| } |
There was a problem hiding this comment.
Line 130 conflates “empty folder” with “uncached folder”.
This check only looks at child rows, so a folder that was successfully enumerated online and legitimately had zero children still falls back to originalError offline. That misses the previously visited empty-folder case from the offline browsing objective. Use a separate “enumerated once” marker/sentinel instead of cachedMetadata.isEmpty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@CryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swift`
around lines 129 - 132, The current logic in ItemEnumerationTaskExecutor
conflates an empty cached child list with "never enumerated" by relying on
cachedMetadata.isEmpty; modify the check to use an explicit sentinel on the
folder metadata (e.g., an "enumeratedOnce" or "lastEnumeratedAt" flag
stored/returned by itemMetadataManager) instead of child count. Update
itemMetadataManager.getCachedMetadata usage and the guard so that if the folder
has been previously enumerated (enumeratedOnce == true or lastEnumeratedAt !=
nil) you treat an empty cachedMetadata as a valid empty-folder result
(resolve/succeed) rather than returning Promise(originalError), and only fall
back to originalError when the sentinel indicates the folder was never
enumerated. Ensure you reference and update the folder metadata field
(folderMetadata.id and the new sentinel) and adjust any places that set the
sentinel when an online enumeration completes.
Comment on lines
+133
to
+138
| let uploadTasks = try uploadTaskManager.getTaskRecords(for: cachedMetadata) | ||
| assert(cachedMetadata.count == uploadTasks.count) | ||
| let items = try cachedMetadata.enumerated().map { index, metadata -> FileProviderItem in | ||
| let localCachedFileInfo = try self.cachedFileManager.getLocalCachedFileInfo(for: metadata) | ||
| return FileProviderItem(metadata: metadata, domainIdentifier: self.domainIdentifier, localCachedFileInfo: localCachedFileInfo, error: uploadTasks[index]?.failedWithError) | ||
| } |
There was a problem hiding this comment.
Reconcile local task state before building the offline list.
This branch builds directly from cachedMetadata, but it never applies filterOutWaitingReparentTasks, filterOutWaitingDeletionTasks, getReparentMetadata, or getPlaceholderMetadata like the online path does. Offline enumeration can therefore re-show items pending local move/delete and hide moved-in or placeholder-only entries.
💡 Suggested direction
private func buildOfflineItemList(folderMetadata: ItemMetadata, originalError: Error) -> Promise<FileProviderItemList> {
do {
- let cachedMetadata = try itemMetadataManager.getCachedMetadata(withParentID: folderMetadata.id!)
- guard !cachedMetadata.isEmpty else {
+ var metadataList = try itemMetadataManager.getCachedMetadata(withParentID: folderMetadata.id!)
+ metadataList = try filterOutWaitingReparentTasks(parentID: folderMetadata.id!, for: metadataList)
+ metadataList = try filterOutWaitingDeletionTasks(parentID: folderMetadata.id!, for: metadataList)
+ metadataList.append(contentsOf: try getReparentMetadata(for: folderMetadata.id!))
+ metadataList.append(contentsOf: try itemMetadataManager.getPlaceholderMetadata(withParentID: folderMetadata.id!))
+ guard !metadataList.isEmpty else {
return Promise(originalError)
}
- let uploadTasks = try uploadTaskManager.getTaskRecords(for: cachedMetadata)
- assert(cachedMetadata.count == uploadTasks.count)
- let items = try cachedMetadata.enumerated().map { index, metadata -> FileProviderItem in
+ let uploadTasks = try uploadTaskManager.getTaskRecords(for: metadataList)
+ assert(metadataList.count == uploadTasks.count)
+ let items = try metadataList.enumerated().map { index, metadata -> FileProviderItem in
let localCachedFileInfo = try self.cachedFileManager.getLocalCachedFileInfo(for: metadata)
return FileProviderItem(metadata: metadata, domainIdentifier: self.domainIdentifier, localCachedFileInfo: localCachedFileInfo, error: uploadTasks[index]?.failedWithError)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@CryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swift`
around lines 133 - 138, The offline path currently builds items directly from
cachedMetadata without applying the reconciliation steps used by the online
path; update the offline branch in ItemEnumerationTaskExecutor so you first run
cachedMetadata through filterOutWaitingReparentTasks(...),
filterOutWaitingDeletionTasks(...), getReparentMetadata(...), and
getPlaceholderMetadata(...) (the same pipeline used for the online path), then
obtain upload task records for that reconciled metadata (call
uploadTaskManager.getTaskRecords(for: reconciledMetadata) or map tasks by
metadata identifier) and finally map the reconciled metadata to FileProviderItem
using those task records so indices and error states remain consistent with the
online enumeration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #144. When the device is offline, folder enumeration in the File Provider now falls back to serving cached metadata from the local database instead of failing with a "server unreachable" error. This makes previously browsed folders accessible in Files.app without connectivity.
The approach catches
CloudProviderError.noInternetConnectionin.recoverblocks rather than proactively detecting network state. If a folder has cached children, they are served as a read-only listing. If the cache is empty (folder never visited online), the original error propagates as before.localFileIsCurrentalso gains an offline recovery path: when enumeration fails due to connectivity, it assumes the local file is current, allowing cached files to open offline.Files copied into the vault while offline end up with a
serverUnreachableupload error. Previously these stayed stuck until the user manually triggered "Retry Upload." NowrecoverStuckUploads()also picks up connectivity-failed upload tasks on vault unlock and reschedules them automatically.markUploadAsErrorwas changed to use.noSuchItemfor missing local files so those do not get caught by the retry query.handleUploadErrornow normalizes transient NSURLErrors (timeout, DNS failure, connection lost) toNSFileProviderError(.serverUnreachable)before persisting, so the retry query catches them even if a cloud provider leaks a raw URL error.A
FileProviderItemconvenience init acceptingLocalCachedFileInfo?replaces the duplicatedisCurrentVersion+localURLextraction pattern at 9 call sites acrossFileProviderAdapter,ItemEnumerationTaskExecutor,WorkingSetObserver,ReparentTaskExecutor, andUploadTaskExecutor.Flow
sequenceDiagram participant Files as Files.app participant Enum as ItemEnumerationTaskExecutor participant Cloud as CloudProvider participant DB as SQLite Cache Files->>Enum: fetchItemList(folder) Enum->>Cloud: fetchItemList(cloudPath) Cloud-->>Enum: noInternetConnection Enum->>DB: getCachedMetadata(parentID) alt cached children exist DB-->>Enum: [ItemMetadata] Enum-->>Files: FileProviderItemList (cached) else no cached children Enum-->>Files: original error (serverUnreachable) end