Enable offline unlock in File Provider#456
Conversation
WalkthroughThis pull request refactors error-checking logic for network and connectivity errors by moving two computed properties ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 (2)
CryptomatorCommon/Tests/CryptomatorCommonCoreTests/ErrorExtensionsTests.swift (1)
37-40: Coverage gap:LocalizedCloudProviderError.noInternetConnectionnot covered forisTransientConnectivityError.The umbrella test asserts transitivity for
CloudProviderError.noInternetConnectionandNSFileProviderError(.serverUnreachable), but skips theLocalizedCloudProviderError.noInternetConnectionbranch — which is the exact variant that motivated this PR (errors wrapped byLocalizedCloudProviderDecorator). Worth pinning so a future refactor ofisNoInternetConnectionErrorcannot silently drop that branch without a test failing.💚 Proposed addition
func testIsTransientConnectivityErrorIncludesNoInternetConnectionErrors() { XCTAssertTrue(CloudProviderError.noInternetConnection.isTransientConnectivityError) + XCTAssertTrue(LocalizedCloudProviderError.noInternetConnection.isTransientConnectivityError) XCTAssertTrue(NSFileProviderError(.serverUnreachable).isTransientConnectivityError) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CryptomatorCommon/Tests/CryptomatorCommonCoreTests/ErrorExtensionsTests.swift` around lines 37 - 40, Add coverage for the LocalizedCloudProviderError wrapper by asserting that LocalizedCloudProviderError.noInternetConnection.isTransientConnectivityError returns true; update the existing test function testIsTransientConnectivityErrorIncludesNoInternetConnectionErrors (or add a small new test) to include an XCTAssertTrue call against LocalizedCloudProviderError.noInternetConnection so the isTransientConnectivityError logic for the LocalizedCloudProviderDecorator variant is exercised.CryptomatorTests/UnlockVaultViewModelTests.swift (1)
140-170: Consider using a Sourcery-generatedVaultAccountManagerMockinstead of a hand-written stub.Most other test doubles here (
VaultCacheMock,VaultPasswordManagerMock,FileProviderConnectorMock,VaultUnlockingMock) come from the mock-generation pipeline, which keeps them in lockstep with the protocol. The hand-rolledVaultAccountManagerStubwill silently go stale ifVaultAccountManagergains/renames members. If a generated mock already exists (or the protocol is in a mockable target), prefer it; otherwise the stub is fine as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CryptomatorTests/UnlockVaultViewModelTests.swift` around lines 140 - 170, Replace the hand-written VaultAccountManagerStub with the Sourcery-generated VaultAccountManagerMock to keep the test double in sync with the VaultAccountManager protocol: locate the VaultAccountManagerStub class and swap usages to VaultAccountManagerMock (or import/use the generated mock target) so tests use the generated mock implementation instead of the manual getAccount/throwing-method stubs; if a generated mock does not yet exist, mark the protocol as mockable for the generator or add the protocol to the mock target so a VaultAccountManagerMock is produced and then update tests to instantiate and configure that mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@CryptomatorCommon/Tests/CryptomatorCommonCoreTests/ErrorExtensionsTests.swift`:
- Around line 37-40: Add coverage for the LocalizedCloudProviderError wrapper by
asserting that
LocalizedCloudProviderError.noInternetConnection.isTransientConnectivityError
returns true; update the existing test function
testIsTransientConnectivityErrorIncludesNoInternetConnectionErrors (or add a
small new test) to include an XCTAssertTrue call against
LocalizedCloudProviderError.noInternetConnection so the
isTransientConnectivityError logic for the LocalizedCloudProviderDecorator
variant is exercised.
In `@CryptomatorTests/UnlockVaultViewModelTests.swift`:
- Around line 140-170: Replace the hand-written VaultAccountManagerStub with the
Sourcery-generated VaultAccountManagerMock to keep the test double in sync with
the VaultAccountManager protocol: locate the VaultAccountManagerStub class and
swap usages to VaultAccountManagerMock (or import/use the generated mock target)
so tests use the generated mock implementation instead of the manual
getAccount/throwing-method stubs; if a generated mock does not yet exist, mark
the protocol as mockable for the generator or add the protocol to the mock
target so a VaultAccountManagerMock is produced and then update tests to
instantiate and configure that mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aba3f61f-5f50-4e4b-bd1b-faca46abd400
📒 Files selected for processing (11)
Cryptomator.xcodeproj/project.pbxprojCryptomatorCommon/Sources/CryptomatorCommonCore/ErrorExtensions.swiftCryptomatorCommon/Tests/CryptomatorCommonCoreTests/ErrorExtensionsTests.swiftCryptomatorFileProvider/Middleware/ErrorMapper.swiftCryptomatorFileProvider/Middleware/TaskExecutor/ItemEnumerationTaskExecutor.swiftCryptomatorFileProvider/Middleware/TaskExecutor/UploadTaskExecutor.swiftCryptomatorFileProviderTests/Middleware/ErrorMapperTests.swiftCryptomatorTests/UnlockVaultViewModelTests.swiftCryptomatorTests/VaultUnlockingMock.swiftFileProviderExtensionUI/FileProviderCoordinator.swiftFileProviderExtensionUI/UnlockVaultViewModel.swift
💤 Files with no reviewable changes (2)
- CryptomatorFileProvider/Middleware/ErrorMapper.swift
- CryptomatorFileProviderTests/Middleware/ErrorMapperTests.swift
1 participant
Completes the offline-operation story PR #446 started. #446 added the offline folder-enumeration fallback and the upload auto-retry, but the piece users hit first (the unlock screen in Files.app) still failed offline, so the fallback was unreachable in practice. This PR closes that gap: unlock now succeeds from the cached masterkey when connectivity drops, and the #446 fallback finally does what it was built to do.
What was in the way: the recover blocks in
UnlockVaultViewModel.refreshVaultCache()andFileProviderCoordinator.showManualLogin()matched onlyCloudProviderError.noInternetConnection, but those errors go throughLocalizedCloudProviderDecoratorwhich converts them toLocalizedCloudProviderError.noInternetConnection. Offline unlock always fell intodefault: throw errorand surfaced as "Server Unreachable" even with a fully cached masterkey. The latent type-mismatch predates #446 (commita4ce8c33e, PR #161).Both recover blocks now use
error.isTransientConnectivityError, the same predicateUploadTaskExecutor.handleUploadErroruses, so the full connectivity-error surface (including rawNSURLErrorvariants) is covered consistently.CloudProviderError.itemNotFoundjoinsLocalizedCloudProviderError.itemNotFoundin the swallow branch.FileProviderCoordinator'sunauthorizedrethrow stays untouched.isNoInternetConnectionErrorandisTransientConnectivityErrormoved fromCryptomatorFileProvider/Middleware/ErrorMapper.swiftinto a new public extension inCryptomatorCommonCore/ErrorExtensions.swiftsoFileProviderExtensionUIcan reach them. Their tests moved toCryptomatorCommonCoreTests/ErrorExtensionsTests.swift;ErrorMapperTests.swiftkeeps itstoPresentableError()coverage.New
CryptomatorTests/UnlockVaultViewModelTests.swift(plus a minimalVaultUnlockingMock) covers the recover block across the swallow/reject matrix.UnlockVaultViewModel.swiftpicks up a secondary target membership inCryptomatorTestsso the test can reference the type directly, avoiding the appex-linkage puzzle a dedicatedFileProviderExtensionUITestsbundle would have.To test: