Open
Conversation
Collaborator
Author
|
I have no idea why I can't link #393 in the sidebar but this would resolve this issue 😬 |
WalkthroughAdded a cache abstraction and XPC-backed implementation (protocol Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 4
🧹 Nitpick comments (2)
CryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swift (1)
235-237: Consider assertingitemIdentifieralongside filename/parent to keep test strength.The new comparison avoids reference-equality issues, but checking
itemIdentifiertoo would better catch regressions.Suggested assertion additions
XCTAssertEqual(expectedRootFolderFileProviderItems.map(\.filename), fileProviderItemList.items.map(\.filename)) XCTAssertEqual(expectedRootFolderFileProviderItems.map(\.parentItemIdentifier), fileProviderItemList.items.map(\.parentItemIdentifier)) +XCTAssertEqual(expectedRootFolderFileProviderItems.map(\.itemIdentifier), fileProviderItemList.items.map(\.itemIdentifier)) ... XCTAssertEqual(expectedSubFolderFileProviderItems.map(\.filename), fileProviderItemList.items.map(\.filename)) XCTAssertEqual(expectedSubFolderFileProviderItems.map(\.parentItemIdentifier), fileProviderItemList.items.map(\.parentItemIdentifier)) +XCTAssertEqual(expectedSubFolderFileProviderItems.map(\.itemIdentifier), fileProviderItemList.items.map(\.itemIdentifier))Also applies to: 256-258, 304-306, 316-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swift` around lines 235 - 237, Add assertions comparing itemIdentifier alongside filename and parentItemIdentifier in the ItemEnumerationTaskTests to strengthen equality checks; specifically, for the existing comparisons that use expectedRootFolderFileProviderItems.map(\.filename) and .map(\.parentItemIdentifier) vs fileProviderItemList.items.map(...), also assert expectedRootFolderFileProviderItems.map(\.itemIdentifier) == fileProviderItemList.items.map(\.itemIdentifier). Apply the same additional assertion pattern to the other failing spots referenced (the comparisons around the variables expectedRootFolderFileProviderItems, fileProviderItemList.items, and metadataManagerMock.cachedMetadata.count at the other test locations).Cryptomator/Settings/XPCCacheController.swift (1)
46-49: Use an unimplemented stub fortestValueinstead of the live XPC controller.Line 47 currently allows tests to fall back to real XPC behavior, weakening isolation and masking missing overrides. Per swift-dependencies conventions, side-effectful dependencies should have
testValueuse failing closures (e.g., via@DependencyClientmacro or manual implementations that throw/fail if called). This ensures tests fail explicitly if unmocked side effects run rather than silently using live behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cryptomator/Settings/XPCCacheController.swift` around lines 46 - 49, The test dependency `CacheControllerKey.testValue` currently returns the live `XPCCacheController`, allowing tests to hit real XPC; change `CacheControllerKey.testValue` to an unimplemented/failing stub that conforms to `CacheControlling` (e.g., a struct or closures that call fatalError/throw when any method is invoked) so tests must explicitly provide a mock, while keeping `CacheControllerKey.liveValue = XPCCacheController()` unchanged; update or add the stub type referenced by `testValue` and ensure it implements the same `CacheControlling` API as `XPCCacheController`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cryptomator/Settings/ClearCacheAppIntent.swift`:
- Around line 5-7: The intent strings in ClearCacheAppIntent (static properties
title, description, dialog, shortTitle and any other user-facing phrases) are
hard-coded in English; replace them with localized
LocalizedStringResource/LocalizedStringKey references and add corresponding
translations in the app’s Localizable.strings (and any Intent-specific strings
files) for all supported languages; update ClearCacheAppIntent to use
NSLocalizedString or LocalizedStringResource initializers (e.g.,
LocalizedStringResource("clear_cache_title")) and ensure matching keys are added
for title, description, dialog, shortTitle and other phrases so the intent is
fully localized across supported languages.
In `@CryptomatorFileProviderTests/FileImportingServiceSourceTests.swift`:
- Around line 34-43: The test is scoping the mock fullVersionChecker only for
construction but FileImportingServiceSource uses the `@Dependency-wrapped`
fullVersionChecker later (e.g., in importFile()), causing resolution to occur
outside the withDependencies scope; fix by making the dependency stable at
instance level—either capture and store the resolved fullVersionChecker during
init (add a stored property on FileImportingServiceSource and assign
Dependency(\.fullVersionChecker) or pass the checker into the initializer) or
ensure tests call importFile() inside the same withDependencies block; update
FileImportingServiceSource initializer and usages (constructor, importFile(),
any other methods accessing fullVersionChecker) to use the stored checker
instead of resolving the `@Dependency` at access time.
In `@CryptomatorFileProviderTests/PermissionProviderImplTests.swift`:
- Around line 24-29: The test scopes dependency overrides only during
PermissionProviderImpl() creation, but PermissionProviderImpl reads `@Dependency`
properties inside getPermissions() and getPermissionsForRootItem(), so the mocks
(hubRepositoryMock, fullVersionCheckerMock) must be active when those methods
run; move the calls to getPermissions() and getPermissionsForRootItem() inside
the same withDependencies block that creates PermissionProviderImpl(), or
alternatively wrap the entire test (both initialization and subsequent method
invocations) in withDependencies so PermissionProviderImpl uses the mocked
dependencies at call time.
In
`@CryptomatorFileProviderTests/ServiceSource/CacheManagingServiceSourceTests.swift`:
- Line 87: The test currently uses [testItem].compactMap { $0 } which can mask a
nil setup; first add an explicit non-nil assertion for testItem (e.g.
XCTAssertNotNil(testItem)) and then compare the unwrapped value directly against
notificatorMock.signalUpdateForReceivedInvocations (for example assert equality
between [testItem!] and notificatorMock.signalUpdateForReceivedInvocations as?
[FileProviderItem]); update the assertion in CacheManagingServiceSourceTests
(referencing testItem and notificatorMock.signalUpdateForReceivedInvocations) to
use the unwrapped value so a nil testItem fails explicitly.
---
Nitpick comments:
In `@Cryptomator/Settings/XPCCacheController.swift`:
- Around line 46-49: The test dependency `CacheControllerKey.testValue`
currently returns the live `XPCCacheController`, allowing tests to hit real XPC;
change `CacheControllerKey.testValue` to an unimplemented/failing stub that
conforms to `CacheControlling` (e.g., a struct or closures that call
fatalError/throw when any method is invoked) so tests must explicitly provide a
mock, while keeping `CacheControllerKey.liveValue = XPCCacheController()`
unchanged; update or add the stub type referenced by `testValue` and ensure it
implements the same `CacheControlling` API as `XPCCacheController`.
In
`@CryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swift`:
- Around line 235-237: Add assertions comparing itemIdentifier alongside
filename and parentItemIdentifier in the ItemEnumerationTaskTests to strengthen
equality checks; specifically, for the existing comparisons that use
expectedRootFolderFileProviderItems.map(\.filename) and
.map(\.parentItemIdentifier) vs fileProviderItemList.items.map(...), also assert
expectedRootFolderFileProviderItems.map(\.itemIdentifier) ==
fileProviderItemList.items.map(\.itemIdentifier). Apply the same additional
assertion pattern to the other failing spots referenced (the comparisons around
the variables expectedRootFolderFileProviderItems, fileProviderItemList.items,
and metadataManagerMock.cachedMetadata.count at the other test locations).
🪄 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: ed971909-428f-4655-8570-8bc324eac228
📒 Files selected for processing (27)
Cryptomator.xcodeproj/project.pbxprojCryptomator.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedCryptomator/Settings/ClearCacheAppIntent.swiftCryptomator/Settings/SettingsViewModel.swiftCryptomator/Settings/XPCCacheController.swiftCryptomatorCommon/Package.swiftCryptomatorCommon/Tests/CryptomatorCommonCoreTests/FullVersionCheckerTests.swiftCryptomatorCommon/Tests/CryptomatorCommonCoreTests/Hub/HubAuthenticationViewModelTests.swiftCryptomatorFileProviderTests/FileImportingServiceSourceTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterEnumerateItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterImportDocumentTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterTestCase.swiftCryptomatorFileProviderTests/FileProviderEnumeratorTests.swiftCryptomatorFileProviderTests/FileProviderItemTests.swiftCryptomatorFileProviderTests/FileProviderNotificatorTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swiftCryptomatorFileProviderTests/PermissionProviderImplTests.swiftCryptomatorFileProviderTests/ServiceSource/CacheManagingServiceSourceTests.swiftCryptomatorFileProviderTests/WorkingSetObserverTests.swiftCryptomatorTests/ChangePasswordViewModelTests.swiftCryptomatorTests/DatabaseManagerTests.swiftCryptomatorTests/MoveVaultViewModelTests.swiftCryptomatorTests/RenameVaultViewModelTests.swiftCryptomatorTests/SettingsViewModelTests.swiftCryptomatorTests/VaultKeepUnlockedViewModelTests.swiftCryptomatorTests/VaultListViewModelTests.swiftCryptomatorTests/XPCCacheControllerTests.swift
2 tasks
Member
|
@phil1995 two things I want your take on before I touch anything: 1.
Both are valid per the library's lifetime docs, but I'd like to pick one for new tests so we stop having this discussion per file. Do you have a preference? 2. A few ways to put the full equality back:
What's your call? Happy to go with whichever. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cryptomator/Settings/XPCCacheController.swift (1)
46-54: Minor: consider a computedtestValuefor consistency withliveValue.
liveValueis a computed property returning a freshXPCCacheController(), buttestValueis a storedvarsharing a singleUnimplementedCacheControllerinstance across tests. SinceUnimplementedCacheControlleris stateless here, this is harmless in practice, but a computed property keeps the pattern uniform and avoids accidental state leakage if the stub ever gains properties.Suggested tweak
- `#if` DEBUG - static var testValue: any CacheControlling = UnimplementedCacheController() - `#endif` + `#if` DEBUG + static var testValue: any CacheControlling { + UnimplementedCacheController() + } + `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cryptomator/Settings/XPCCacheController.swift` around lines 46 - 54, The testValue in CacheControllerKey is a stored var returning a single UnimplementedCacheController instance which differs from the computed liveValue pattern; change testValue to be a computed property (like liveValue) so it returns a new UnimplementedCacheController() each access to maintain consistency and avoid accidental shared state—update CacheControllerKey.testValue to be a computed var returning UnimplementedCacheController() (while keeping the `#if` DEBUG guard and the DependencyKey conformance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cryptomator/Settings/XPCCacheController.swift`:
- Around line 46-54: The testValue in CacheControllerKey is a stored var
returning a single UnimplementedCacheController instance which differs from the
computed liveValue pattern; change testValue to be a computed property (like
liveValue) so it returns a new UnimplementedCacheController() each access to
maintain consistency and avoid accidental shared state—update
CacheControllerKey.testValue to be a computed var returning
UnimplementedCacheController() (while keeping the `#if` DEBUG guard and the
DependencyKey conformance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 933c45b5-8ca8-43ee-b8a9-bb573d5b4d7c
📒 Files selected for processing (26)
Cryptomator/Settings/ClearCacheAppIntent.swiftCryptomator/Settings/XPCCacheController.swiftCryptomatorCommon/Tests/CryptomatorCommonCoreTests/FullVersionCheckerTests.swiftCryptomatorCommon/Tests/CryptomatorCommonCoreTests/Hub/HubAuthenticationViewModelTests.swiftCryptomatorCommon/Tests/CryptomatorCommonCoreTests/Manager/CloudProviderAccountManagerTests.swiftCryptomatorFileProviderTests/FileImportingServiceSourceTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterEnumerateItemTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterImportDocumentTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swiftCryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterTestCase.swiftCryptomatorFileProviderTests/FileProviderEnumeratorTests.swiftCryptomatorFileProviderTests/FileProviderItemTests.swiftCryptomatorFileProviderTests/FileProviderNotificatorTests.swiftCryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swiftCryptomatorFileProviderTests/PermissionProviderImplTests.swiftCryptomatorFileProviderTests/ServiceSource/CacheManagingServiceSourceTests.swiftCryptomatorFileProviderTests/WorkingSetObserverTests.swiftCryptomatorTests/ChangePasswordViewModelTests.swiftCryptomatorTests/DatabaseManagerTests.swiftCryptomatorTests/MoveVaultViewModelTests.swiftCryptomatorTests/RenameVaultViewModelTests.swiftCryptomatorTests/SettingsViewModelTests.swiftCryptomatorTests/VaultKeepUnlockedViewModelTests.swiftCryptomatorTests/VaultListViewModelTests.swiftCryptomatorTests/XPCCacheControllerTests.swiftSharedResources/en.lproj/Localizable.strings
✅ Files skipped from review due to trivial changes (5)
- CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterRecoverUploadsTests.swift
- CryptomatorFileProviderTests/PermissionProviderImplTests.swift
- SharedResources/en.lproj/Localizable.strings
- CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterTestCase.swift
- CryptomatorFileProviderTests/Middleware/TaskExecutor/ItemEnumerationTaskTests.swift
🚧 Files skipped from review as they are similar to previous changes (15)
- CryptomatorFileProviderTests/FileProviderEnumeratorTests.swift
- CryptomatorCommon/Tests/CryptomatorCommonCoreTests/Hub/HubAuthenticationViewModelTests.swift
- CryptomatorFileProviderTests/FileProviderItemTests.swift
- CryptomatorTests/RenameVaultViewModelTests.swift
- CryptomatorFileProviderTests/FileImportingServiceSourceTests.swift
- CryptomatorTests/VaultKeepUnlockedViewModelTests.swift
- CryptomatorFileProviderTests/FileProviderAdapter/FileProviderAdapterEnumerateItemTests.swift
- CryptomatorFileProviderTests/WorkingSetObserverTests.swift
- CryptomatorTests/VaultListViewModelTests.swift
- CryptomatorCommon/Tests/CryptomatorCommonCoreTests/FullVersionCheckerTests.swift
- CryptomatorFileProviderTests/FileProviderNotificatorTests.swift
- CryptomatorTests/XPCCacheControllerTests.swift
- CryptomatorTests/DatabaseManagerTests.swift
- CryptomatorTests/ChangePasswordViewModelTests.swift
- Cryptomator/Settings/ClearCacheAppIntent.swift
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
2 participants
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.
Adds an shortcut to clear the cache by adding a new
AppIntent.As part of this PR I also migrated away from the dependency I introduced 1-2 years ago to the official one
swift-dependenciesas I hit a wall with the simplified version. Also it no longer hits the build time, is better supported for future Swift versions and has nice feature like automatically created mock with the@DependencyClientmacro.I'm already sorry for the huge diff which mainly comes from the migration to the official
swift-dependenciespackage 🙈To test the feature:
Clear CacheWarning
Localization for the newly added strings is still missing