Bugfix: Missing lock icons in vaults list #386
Conversation
WalkthroughThe changes primarily involve significant updates to the Changes
Possibly related PRs
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 (
|
iammajid
changed the title
iammajid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Cryptomator/VaultList/VaultListViewModel.swift (1 hunks)
- CryptomatorTests/VaultListViewModelTests.swift (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
CryptomatorTests/VaultListViewModelTests.swift (3)
134-134: LGTM. Please clarify the reason for this change.The assertion has been updated to expect a specific domain identifier (
"vault1") instead of a nil value. This change aligns with the PR objective of fixing the missing lock icon issue.Could you please provide more context on why this change was necessary? For example, was the previous behavior incorrect, or does this reflect a change in the implementation of
refreshVaultLockStates?
146-146: LGTM. Please explain the reason for increasing the expected count.The expected
xpcInvalidationCallCounthas been increased from 1 to 2. This change suggests that an additional XPC invalidation is now occurring during therefreshVaultLockStatesprocess.Could you please elaborate on why this additional invalidation is now necessary? Is it related to ensuring the lock icon is properly updated in the UI?
Line range hint
1-265: Overall, the changes look good. Please provide an overview of the fix.The modifications to the
testRefreshVaultLockedStatesmethod seem to align with the PR objective of fixing the missing lock icon issue. The test now expects a specific domain identifier to be passed and an additional XPC invalidation to occur.To ensure a comprehensive understanding of the fix:
- Could you provide an overview of the changes made in the main implementation to address the missing lock icon issue?
- How do these test modifications reflect those changes?
- Are there any other tests that needed to be updated as a result of this fix?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
CryptomatorCommon/Sources/CryptomatorCommonCore/Promise+AllIgnoringResult.swift (1)
Line range hint
12-17: Approve the visibility change and suggest a minor naming improvement.The change to make this function public is appropriate, as it allows for its use in other modules like
VaultListViewModel. This aligns with the learning from tobihagemann.However, I have a minor suggestion to improve clarity:
Consider renaming the function to
allSettledorallCompletedinstead ofall. The current name might be slightly misleading as the function usesanyinternally and doesn't behave exactly like a typicalPromise.all. The suggested names better reflect that the function waits for all promises to settle, regardless of their individual outcomes.Here's a suggested change:
-public func all<Value, Container: Sequence>(ignoringResult promises: Container) -> Promise<Void> where Container.Element == Promise<Value> { +public func allSettled<Value, Container: Sequence>(ignoringResult promises: Container) -> Promise<Void> where Container.Element == Promise<Value> {This change would make the function's behavior more immediately clear to other developers using this API.
CryptomatorCommon/Tests/CryptomatorCommonCoreTests/Promise+AllIgnoringResultsTests.swift (1)
Line range hint
13-33: LGTM: Test methods remain valid and comprehensive.The test methods
testWaitForAllandtestWaitForAllWithRejectedPromiseappear to be unchanged and still valid after the module restructuring. They provide good coverage for both fulfilled and rejected promise scenarios.For improved clarity, consider adding a brief comment before each test method explaining its purpose. For example:
// Test that all(ignoringResult:) waits for all promises to be fulfilled func testWaitForAll() throws { // ... existing code ... } // Test that all(ignoringResult:) completes even when one promise is rejected func testWaitForAllWithRejectedPromise() throws { // ... existing code ... }CryptomatorCommon/Tests/CryptomatorCommonCoreTests/XCTestCase+Promises.swift (1)
41-48: Excellent addition to enhance promise testing capabilities!The
XCTAssertGetsNotExecutedmethod is a valuable addition to theXCTestCaseextension. It provides a clean way to assert that a promise does not execute within a specified timeframe, which is crucial for negative testing scenarios.The implementation is well-thought-out:
- Using an inverted expectation is an elegant solution.
- The
alwaysblock ensures the expectation is fulfilled regardless of the promise's outcome.- The method signature is consistent with XCTest conventions.
Consider adding a brief comment explaining the method's purpose and how it works. This would enhance code readability and make it easier for other developers to understand and use this method. For example:
/// Asserts that the given promise does not execute within the specified timeout. /// This is useful for testing scenarios where a promise is expected to not resolve or reject. func XCTAssertGetsNotExecuted<T>(_ promise: Promise<T>, timeout seconds: TimeInterval = 1.0, file: StaticString = #filePath, line: UInt = #line) { // ... (existing implementation) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- Cryptomator.xcodeproj/project.pbxproj (0 hunks)
- Cryptomator/VaultList/VaultListViewModel.swift (1 hunks)
- CryptomatorCommon/Sources/CryptomatorCommonCore/Promise+AllIgnoringResult.swift (1 hunks)
- CryptomatorCommon/Tests/CryptomatorCommonCoreTests/Promise+AllIgnoringResultsTests.swift (1 hunks)
- CryptomatorCommon/Tests/CryptomatorCommonCoreTests/XCTestCase+Promises.swift (1 hunks)
- CryptomatorFileProvider/Workflow/WorkflowDependencyFactory.swift (1 hunks)
💤 Files with no reviewable changes (1)
- Cryptomator.xcodeproj/project.pbxproj
✅ Files skipped from review due to trivial changes (1)
- CryptomatorFileProvider/Workflow/WorkflowDependencyFactory.swift
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: tobihagemann PR: cryptomator/ios#386 File: Cryptomator/VaultList/VaultListViewModel.swift:112-112 Timestamp: 2024-10-14T15:45:13.118Z Learning: In `VaultListViewModel`, use `all(ignoringResult: promises)` instead of `all(promises).then { }` and omit unnecessary logging when refreshing vault lock states.Cryptomator/VaultList/VaultListViewModel.swift (2)
Learnt from: tobihagemann PR: cryptomator/ios#386 File: Cryptomator/VaultList/VaultListViewModel.swift:112-112 Timestamp: 2024-10-14T15:45:13.118Z Learning: In `VaultListViewModel`, use `all(ignoringResult: promises)` instead of `all(promises).then { }` and omit unnecessary logging when refreshing vault lock states.Learnt from: tobihagemann PR: cryptomator/ios#386 File: Cryptomator/VaultList/VaultListViewModel.swift:123-126 Timestamp: 2024-10-14T15:45:19.005Z Learning: Prefer using `DDLogDebug` instead of `DDLogInfo` to avoid log spam.
🔇 Additional comments (2)
Cryptomator/VaultList/VaultListViewModel.swift (2)
107-111: Excellent refactoring ofrefreshVaultLockStatesThe implementation has been improved to use concurrent asynchronous checks for vault unlock statuses. The use of
all(ignoringResult: promises)is an excellent choice, as it allows the operation to proceed even if individual checks fail. This aligns with the best practices for handling multiple promises in this codebase.
107-123: Overall improvements in vault lock state handlingThe changes in this file significantly improve the handling of vault lock states:
- The
refreshVaultLockStatesmethod now efficiently manages concurrent asynchronous checks for all vaults.- The new
checkVaultUnlockStatusmethod provides a clear, promise-based approach to checking individual vault lock states.These improvements should effectively address the issue of missing lock icons in the vaults list (bug ticket #346). The code is now more robust and should handle the lock state updates more reliably, especially after the iOS 17.4 update.
CryptomatorCommon/Tests/CryptomatorCommonCoreTests/Promise+AllIgnoringResultsTests.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Cryptomator/VaultList/VaultListViewModel.swift (1)
113-126: LGTM! Well-implemented unlock status check with appropriate logging.The
checkVaultUnlockStatusmethod efficiently checks the unlock status for a single vault and uses appropriate logging levels (DDLogDebugandDDLogError) as suggested in previous feedback.Consider adding a specific error type or enum for better error handling and consistency across the codebase.
You could improve error handling by defining a specific error type:
enum VaultUnlockStatusError: Error { case failedToGetXPC case failedToCheckUnlockStatus(underlyingError: Error) }Then update the error handling:
}.catch { error in let vaultError = VaultUnlockStatusError.failedToCheckUnlockStatus(underlyingError: error) DDLogError("Failed to check unlock status for vault \(vault.vaultUID): \(vaultError)") }This approach would provide more structured error information and improve error handling consistency throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Cryptomator/VaultList/VaultListViewModel.swift (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: tobihagemann PR: cryptomator/ios#386 File: Cryptomator/VaultList/VaultListViewModel.swift:112-112 Timestamp: 2024-10-14T15:45:13.118Z Learning: In `VaultListViewModel`, use `all(ignoringResult: promises)` instead of `all(promises).then { }` and omit unnecessary logging when refreshing vault lock states.Cryptomator/VaultList/VaultListViewModel.swift (1)
Learnt from: tobihagemann PR: cryptomator/ios#386 File: Cryptomator/VaultList/VaultListViewModel.swift:112-112 Timestamp: 2024-10-14T15:45:13.118Z Learning: In `VaultListViewModel`, use `all(ignoringResult: promises)` instead of `all(promises).then { }` and omit unnecessary logging when refreshing vault lock states.
🔇 Additional comments (1)
Cryptomator/VaultList/VaultListViewModel.swift (1)
107-111: LGTM! Efficient implementation of concurrent vault lock state refresh.The refactored
refreshVaultLockStatesmethod now usesall(ignoringResult: promises)to handle multiple promises concurrently, which is an improvement in efficiency. The implementation aligns with the previous feedback and learnings.
tobihagemann
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Cryptomator/VaultList/VaultCellViewModel.swift (1)
48-48: Approve the direct update of vault unlock statusThe change to directly update
self.vault.vaultIsUnlocked.value = falsein thelockVault()method is a good simplification. It centralizes the responsibility of updating the vault's unlock status and removes the need for a separatesetVaultUnlockStatusmethod.Consider adding a comment explaining why this update is done here, to improve code readability:
- self.vault.vaultIsUnlocked.value = false + // Update the vault's unlock status after successfully locking it + self.vault.vaultIsUnlocked.value = false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- Cryptomator/Snapshots/SnapshotCoordinator.swift (0 hunks)
- Cryptomator/VaultList/VaultCellViewModel.swift (1 hunks)
- CryptomatorCommon/Sources/CryptomatorCommonCore/FileProviderXPC/VaultLocking.swift (0 hunks)
- CryptomatorFileProvider/FileProviderAdapterManager.swift (0 hunks)
- CryptomatorFileProvider/ServiceSource/VaultLockingServiceSource.swift (0 hunks)
- CryptomatorFileProviderTests/FileProviderAdapterManagerTests.swift (0 hunks)
- CryptomatorFileProviderTests/Mocks/FileProviderAdapterCacheTypeMock.swift (0 hunks)
- CryptomatorTests/VaultListViewModelTests.swift (2 hunks)
💤 Files with no reviewable changes (6)
- Cryptomator/Snapshots/SnapshotCoordinator.swift
- CryptomatorCommon/Sources/CryptomatorCommonCore/FileProviderXPC/VaultLocking.swift
- CryptomatorFileProvider/FileProviderAdapterManager.swift
- CryptomatorFileProvider/ServiceSource/VaultLockingServiceSource.swift
- CryptomatorFileProviderTests/FileProviderAdapterManagerTests.swift
- CryptomatorFileProviderTests/Mocks/FileProviderAdapterCacheTypeMock.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- CryptomatorTests/VaultListViewModelTests.swift
🧰 Additional context used
🔇 Additional comments (2)
Cryptomator/VaultList/VaultCellViewModel.swift (2)
Line range hint
1-67: Clarify the connection between these changes and the missing lock iconsWhile the changes improve the code by simplifying the update of the vault's unlock status, it's not immediately clear how this directly addresses the issue of missing lock icons in the vaults list (as mentioned in the PR objective).
Could you please clarify:
- How does updating
self.vault.vaultIsUnlocked.valuerelate to the display of lock icons?- Are there any UI components that observe this value to update the lock icon visibility?
To help investigate this, please run the following script:
Line range hint
13-18: Verify the impact of removingsetVaultUnlockStatus(unlocked: Bool)from the protocolThe
setVaultUnlockStatus(unlocked: Bool)method has been removed from theVaultCellViewModelProtocol. This change might affect other parts of the codebase that implement or use this protocol.Please run the following script to check for any remaining implementations or usages of this method:
✅ Verification successful
Removal of
setVaultUnlockStatus(unlocked: Bool)VerifiedThe removal of
setVaultUnlockStatus(unlocked: Bool)fromVaultCellViewModelProtocolandVaultCellViewModelhas been successfully verified. No remaining implementations or usages were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for implementations or usages of setVaultUnlockStatus method # Test 1: Search for implementations of setVaultUnlockStatus echo "Searching for implementations of setVaultUnlockStatus:" rg --type swift 'func\s+setVaultUnlockStatus' # Test 2: Search for calls to setVaultUnlockStatus echo "Searching for calls to setVaultUnlockStatus:" rg --type swift 'setVaultUnlockStatus\(' # Test 3: Search for references to VaultCellViewModelProtocol echo "Searching for references to VaultCellViewModelProtocol:" rg --type swift 'VaultCellViewModelProtocol'Length of output: 720
This pull request fixes the issue described in bug ticket #346, where the lock icons are missing in the vaults list after iOS 17.4 and newer.