Adds "Share Vault" option to vault detail settings. #424
Conversation
Refactored setupConstraints() to fix function body length violation and applied SwiftFormat auto-formatting.
iammajid
force-pushed
the
feature/hub-sharebutton
branch
from
5c561a1 to
a317f83
Compare
WalkthroughAdds a new ShareVault feature and integrates it into the VaultDetail flow. New files: ShareVaultCoordinator (navigation), ShareVaultViewController (UI), ShareVaultViewModel (data). VaultDetailCoordinator gains showShareVault(); VaultDetailViewController handles a new .shareVault action; VaultDetailViewModel adds a shareVault section and action. New localization keys and two image asset sets are added. Project file updated to include the new sources in build targets. A HubConfig extension visibility was changed to public in CryptomatorCommon. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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: 0
🧹 Nitpick comments (2)
Cryptomator/VaultDetail/VaultDetailViewModel.swift (1)
115-117: Consider using the disclosure-style button for navigation.Since tapping this row launches the Share Vault flow, it would align with the rest of the settings UI to present it via
ButtonCellViewModel.createDisclosureButton(...), giving the familiar chevron cue alongside rename/move/change password.Cryptomator/VaultDetail/ShareVault/ShareVaultViewController.swift (1)
123-134: Hide/disable the CTA when no URL is provided
ShareVaultViewModelProtocol.forTeamsURLis optional, yet the button stays tappable when that value is missing, leaving users with a dead control. Please disable (and visually soften) the CTA whenever the view model cannot supply a destination so the UI reflects the available action.let button = UIButton(type: .system) button.setTitle(viewModel.forTeamsButtonTitle, for: .normal) button.backgroundColor = .cryptomatorPrimary button.setTitleColor(.white, for: .normal) button.titleLabel?.font = .preferredFont(forTextStyle: .headline) button.titleLabel?.adjustsFontForContentSizeCategory = true button.layer.cornerRadius = LayoutConstants.cornerRadius button.translatesAutoresizingMaskIntoConstraints = false + let hasTarget = viewModel.forTeamsURL != nil + button.isEnabled = hasTarget + button.alpha = hasTarget ? 1 : 0.5 button.addTarget(self, action: #selector(visitHubButtonTapped), for: .touchUpInside)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
SharedResources/Assets.xcassets/cryptomator-hub-logo.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/cryptomator-hub-logo.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/cryptomator-hub-logo.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/cryptomator-hub.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/cryptomator-hub.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/cryptomator-hub.imageset/[email protected]is excluded by!**/*.png
📒 Files selected for processing (10)
Cryptomator.xcodeproj/project.pbxproj(6 hunks)Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift(1 hunks)Cryptomator/VaultDetail/ShareVault/ShareVaultViewController.swift(1 hunks)Cryptomator/VaultDetail/ShareVault/ShareVaultViewModel.swift(1 hunks)Cryptomator/VaultDetail/VaultDetailCoordinator.swift(1 hunks)Cryptomator/VaultDetail/VaultDetailViewController.swift(1 hunks)Cryptomator/VaultDetail/VaultDetailViewModel.swift(4 hunks)SharedResources/Assets.xcassets/cryptomator-hub-logo.imageset/Contents.json(1 hunks)SharedResources/Assets.xcassets/cryptomator-hub.imageset/Contents.json(1 hunks)SharedResources/en.lproj/Localizable.strings(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-10T15:32:49.838Z
Learnt from: tobihagemann
Repo: cryptomator/ios PR: 384
File: FileProviderExtensionUI/FileProviderCoordinator.swift:85-86
Timestamp: 2024-10-10T15:32:49.838Z
Learning: In `ReauthenticationViewController.swift`, the `coordinator` property is declared as `weak`, preventing retain cycles with `FileProviderCoordinator`.
Applied to files:
Cryptomator.xcodeproj/project.pbxprojCryptomator/VaultDetail/VaultDetailViewController.swiftCryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swiftCryptomator/VaultDetail/VaultDetailCoordinator.swiftCryptomator/VaultDetail/ShareVault/ShareVaultViewController.swift
📚 Learning: 2024-10-14T15:45:13.118Z
Learnt from: tobihagemann
Repo: cryptomator/ios PR: 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.
Applied to files:
Cryptomator/VaultDetail/VaultDetailViewModel.swift
🧬 Code graph analysis (4)
Cryptomator/VaultDetail/VaultDetailViewModel.swift (1)
CryptomatorCommon/Sources/CryptomatorCommonCore/LocalizedString.swift (1)
getValue(12-22)
Cryptomator/VaultDetail/VaultDetailViewController.swift (1)
Cryptomator/VaultDetail/VaultDetailCoordinator.swift (1)
showShareVault(101-106)
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift (1)
Cryptomator/VaultDetail/VaultDetailCoordinator.swift (1)
start(32-37)
Cryptomator/VaultDetail/VaultDetailCoordinator.swift (1)
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift (1)
start(24-34)
🔇 Additional comments (1)
Cryptomator.xcodeproj/project.pbxproj (1)
448-450: ShareVault sources correctly hooked up.Thanks for wiring the new coordinator, view model, and view controller into the Cryptomator target—everything lands in the right build phase with no stray memberships.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new "Share Vault" feature to the Cryptomator iOS app, allowing users to share vaults with team members using Cryptomator Hub. The feature includes UI screens with instructions and links to access the Hub.
Key changes:
- New localized strings for the share vault feature in multiple languages
- Image assets for Cryptomator Hub branding (logo and screenshots)
- New ShareVault module with coordinator, view model, and view controller
- Integration of share vault button in the vault detail screen
Reviewed Changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| SharedResources/en.lproj/Localizable.strings | Added 13 new localization keys for share vault UI text |
| SharedResources/Assets.xcassets/cryptomator-hub.imageset/* | Added Hub screenshot images at 1x, 2x, and 3x resolutions |
| SharedResources/Assets.xcassets/cryptomator-hub-logo.imageset/* | Added Hub logo images at 1x, 2x, and 3x resolutions |
| Cryptomator/VaultDetail/VaultDetailViewModel.swift | Added share vault section and button action to vault detail screen |
| Cryptomator/VaultDetail/VaultDetailViewController.swift | Added handler for share vault button action |
| Cryptomator/VaultDetail/VaultDetailCoordinator.swift | Added navigation to share vault screen |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tobihagemann
left a comment
tobihagemann
left a comment
There was a problem hiding this comment.
Since we're building new UI here: Was there a reason for using UIKit instead of SwiftUI? Would've thought that SwiftUI could simplify things. I think it should be a long-term goal to migrate to SwiftUI, so we shouldn't pile up our technical debt. 😄 What do you think?
| // Extract base Hub URL | ||
| let apiBaseURL: URL? | ||
| if let apiBaseUrl = hubConfig.apiBaseUrl { | ||
| apiBaseURL = URL(string: apiBaseUrl) | ||
| } else if let deviceResourceURL = URL(string: hubConfig.devicesResourceUrl) { | ||
| apiBaseURL = deviceResourceURL.deletingLastPathComponent() | ||
| } else { | ||
| apiBaseURL = nil | ||
| } | ||
|
|
||
| return apiBaseURL?.deletingLastPathComponent() |
There was a problem hiding this comment.
Simplify this by returning hubConfig.getWebAppURL().
There was a problem hiding this comment.
I went with UIKit mainly for consistency, since the rest of the app is still based on it. 😄 But absolutely, it makes total sense to gradually move more towards SwiftUI in the future. 👍
There was a problem hiding this comment.
All right, I understand. But then lets refactor this to SwiftUI.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift (1)
368-382: Based on the verification results, I can now finalize the review. Let me search for any additional context about the codebase's documentation standards.Add documentation for the public HubConfig extension and its methods.
The extension and its methods are now part of the public API (used by ShareVault code at line 44 of ShareVaultCoordinator.swift) but lack documentation. Public declarations require documentation comments, which improve API clarity and enable Xcode Quick Help support.
Apply this diff:
+/// Public helpers for deriving Hub URLs from HubConfig. public extension HubConfig { + /// Returns the API base URL for the Hub instance. + /// + /// If `apiBaseUrl` is set, returns it directly. Otherwise, derives the API base URL + /// by removing the last path component from `devicesResourceUrl`. + /// + /// - Returns: The API base URL, or `nil` if it cannot be determined. func getAPIBaseURL() -> URL? { if let apiBaseUrl { return URL(string: apiBaseUrl) } guard let deviceResourceURL = URL(string: devicesResourceUrl) else { return nil } return deviceResourceURL.deletingLastPathComponent() } + /// Returns the web app URL for the Hub instance. + /// + /// Derives the web app URL by navigating up one level from the API base URL + /// and appending the "app" path component. + /// + /// - Returns: The web app URL, or `nil` if the API base URL cannot be determined. func getWebAppURL() -> URL? { getAPIBaseURL()?.deletingLastPathComponent().appendingPathComponent("app") } }
🧹 Nitpick comments (1)
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift (1)
36-45: Consider adding logging for Hub URL extraction failures.The method correctly simplifies the extraction by returning
hubConfig.getWebAppURL()(as per past review feedback). However, thetry?operators silently ignore errors, which could make debugging difficult if URL extraction fails.Consider adding error logging when extraction fails:
private func extractHubVaultURL() -> URL? { - guard let cachedVault = try? VaultDBCache().getCachedVault(withVaultUID: vaultInfo.vaultUID), - let vaultConfigToken = cachedVault.vaultConfigToken, - let vaultConfig = try? UnverifiedVaultConfig(token: vaultConfigToken), - let hubConfig = vaultConfig.allegedHubConfig else { + do { + let cachedVault = try VaultDBCache().getCachedVault(withVaultUID: vaultInfo.vaultUID) + guard let vaultConfigToken = cachedVault.vaultConfigToken, + let vaultConfig = try? UnverifiedVaultConfig(token: vaultConfigToken), + let hubConfig = vaultConfig.allegedHubConfig else { + DDLogWarn("ShareVaultCoordinator: Failed to extract Hub config from vault") + return nil + } + return hubConfig.getWebAppURL() + } catch { + DDLogError("ShareVaultCoordinator: Failed to get cached vault: \(error)") return nil } - - return hubConfig.getWebAppURL() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-10T15:32:49.838Z
Learnt from: tobihagemann
Repo: cryptomator/ios PR: 384
File: FileProviderExtensionUI/FileProviderCoordinator.swift:85-86
Timestamp: 2024-10-10T15:32:49.838Z
Learning: In `ReauthenticationViewController.swift`, the `coordinator` property is declared as `weak`, preventing retain cycles with `FileProviderCoordinator`.
Applied to files:
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift
🧬 Code graph analysis (1)
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift (3)
Cryptomator/MainCoordinator.swift (1)
navigationController(92-102)Cryptomator/VaultDetail/VaultDetailCoordinator.swift (1)
start(32-37)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift (1)
getWebAppURL(379-381)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and test (freemium)
🔇 Additional comments (1)
Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift (1)
13-22: Code follows correct memory management pattern.Verification confirms that
ShareVaultViewController.coordinatoris correctly declared asweakat line 13, preventing retain cycles withShareVaultCoordinator. The coordinator setup follows best practices.
| func start() { | ||
| let viewModel: ShareVaultViewModel | ||
| if vaultInfo.vaultConfigType == .hub, let hubURL = extractHubVaultURL() { | ||
| viewModel = ShareVaultViewModel(type: .hub(hubURL)) | ||
| } else { | ||
| viewModel = ShareVaultViewModel(type: .normal) | ||
| } | ||
| let shareVaultViewController = ShareVaultViewController(viewModel: viewModel) | ||
| shareVaultViewController.coordinator = self | ||
| navigationController.pushViewController(shareVaultViewController, animated: true) | ||
| } |
There was a problem hiding this comment.
Consider handling Hub URL extraction failures more explicitly.
When vaultInfo.vaultConfigType == .hub but extractHubVaultURL() returns nil, the code silently falls back to .normal type. This could confuse users who expect Hub-specific features. Consider:
- Logging the failure for debugging purposes
- Showing an error message to the user if Hub configuration is expected but unavailable
- Distinguishing between "normal vault" and "Hub vault with extraction error" states
🤖 Prompt for AI Agents
In Cryptomator/VaultDetail/ShareVault/ShareVaultCoordinator.swift around lines
24 to 34, when vaultInfo.vaultConfigType == .hub and extractHubVaultURL()
returns nil the code currently falls back silently to .normal; change this to
explicitly handle the failure by (1) logging the extraction failure with context
(vault id/name) via the app logger, (2) presenting a user-facing error
(UIAlertController) on the main thread that explains the Hub configuration could
not be loaded and offers to retry or cancel, and (3) updating the created
ViewModel/state so the UI can reflect the distinct case (either add a new
ShareVaultViewModel case like .hubExtractionError or pass a flag into .normal to
show an error state) — implement logging before constructing the view
controller, present the alert if extraction fails and only push the view
controller after user confirmation or push a view controller initialized with
the error state so the UI clearly distinguishes "normal vault" from "Hub vault
with extraction error".
There was a problem hiding this comment.
@tobihagemann I see three ways to fix this:
- Option 1: Show an error alert and don't navigate at all if URL extraction fails
- Option 2: Add a .hubError state that shows "Failed to load Hub information"
- Option 3: Hide the Share button upstream if we detect URL extraction would fail I'd lean towards
Option 1 - it's the clearest for users. Alternatively, is this just an edge case that's unlikely to happen in practice? In the worst case, users would currently just see our Hub marketing page instead of their actual Hub instance, which would be confusing but not breaking anything. What do you think?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yeah, let's go with option 1. It's totally an edge case and shouldn't happen, but you never know what we're changing in the future. 😄
New Share Vault section appears in vault detail settings between locking options and vault management. For normal vaults, displays promotional content about Cryptomator Hub with links to documentation and the Hub website. For Hub-managed vaults, shows sharing instructions and provides a direct link to open the vault in Cryptomator Hub.