Feature: Vault Recovery Enhancements #3821
Conversation
auto restore when bkup exists added new VaultStates
…se dir and restore
dialog fix
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/addvaultwizard/ChooseExistingVaultController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyIsHubVaultController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyExpertSettingsController.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyValidateController.java
Outdated
Show resolved
Hide resolved
remove is hub vault dialog
src/main/java/org/cryptomator/common/recovery/BackupRestorer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/recovery/BackupRestorer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/recovery/BackupRestorer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/recovery/BackupRestorer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/recovery/CryptoFsInitializer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/recovery/MasterkeyService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/recovery/RecoveryDirectory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/addvaultwizard/ChooseExistingVaultController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/mainwindow/VaultDetailMissingVaultController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java
Outdated
Show resolved
Hide resolved
…ectory using Optional
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/common/vaults/VaultListManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyValidateController.java
Outdated
Show resolved
Hide resolved
…OCKED to NEEDS_MIGRATION
…ctor chooseValidVaultDirectory() to return Path instead of File
src/main/java/org/cryptomator/common/recovery/VaultPreparator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyValidateController.java (1)
77-81: Debounce validation to avoid heavy I/O on every keystroke.validateRecoveryKeyAndDetectCombo creates temp dirs, writes files, and scans the vault; doing this per character is expensive.
public void initialize() { recoveryKey.bind(textarea.textProperty()); - textarea.textProperty().addListener(((observable, oldValue, newValue) -> validateRecoveryKey())); + // Debounce validation to reduce I/O churn + var debounce = new javafx.animation.PauseTransition(javafx.util.Duration.millis(350)); + debounce.setOnFinished(_ -> validateRecoveryKey()); + textarea.textProperty().addListener((obs, ov, nv) -> { + debounce.stop(); + debounce.playFromStart(); + }); }
♻️ Duplicate comments (8)
src/main/resources/i18n/strings.properties (1)
132-133: Placeholder text removed — good“Restore the masterkey file instead” replaces the earlier placeholder. Thanks for fixing.
src/main/java/org/cryptomator/common/recovery/VaultPreparator.java (1)
31-36: Fallback display name uses i18n key — goodUsing
defaults.vault.vaultNameresolves prior “use generic vault display name” concerns.src/main/java/org/cryptomator/common/vaults/VaultListManager.java (1)
204-209: Don’t mutate the vault while “determining” state; separate detection from restoration.determineVaultState() copies backups into the vault directory. This mixes pure state detection with side effects and can surprise callers.
Proposed split:
- determineVaultState(Path): pure, no writes.
- attemptAutoRestoreBackups(Path): optional, explicit, called by a higher-level recovery flow or after user consent.
If desired, I can draft the refactor across call sites.
src/main/java/org/cryptomator/common/recovery/MasterkeyService.java (1)
54-54: Add NoSuchElementException to method signature as requested in reviewThe method uses
orElseThrow()which throws aNoSuchElementException. This exception should be declared in the method signature as requested in the previous review comment from infeo.src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyCreationController.java (1)
100-100: Use direct enum comparison instead of equals()You can compare enums directly with
==for better readability and performance.-if (recoverType.get() == RecoveryActionType.SHOW_KEY) { +if (recoverType.get() == RecoveryActionType.SHOW_KEY) {src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java (2)
229-247: Extract vault directory validation to VaultPreparatorThe directory validation logic (checking for data directory) should be in
VaultPreparatoror a dedicated validation utility class, not in the UI controller.This logic is not related to the UI controller. It must be placed inside the recovery
...common.recoverypackage inside a new class.
267-273: Consider path-based checking before creating vault objectIf we compare the vault by path, can't we first check for the path and only if there is no matching entry create/prepare it?
src/main/java/org/cryptomator/ui/convertvault/ConvertVaultModule.java (1)
122-127: Inject the cipher combo property instead of constructing a null-backed onePassing
new SimpleObjectProperty<>(null)risks NPEs downstream and breaks type-safety. Inject the shared, typed@Named("cipherCombo") ObjectProperty<CryptorProvider.Scheme>and forward it.- static FxController provideRecoveryKeyValidateController(@ConvertVaultWindow Vault vault, @ConvertVaultWindow VaultConfig.UnverifiedVaultConfig vaultConfig, @ConvertVaultWindow StringProperty recoveryKey, RecoveryKeyFactory recoveryKeyFactory, MasterkeyFileAccess masterkeyFileAccess) { - return new RecoveryKeyValidateController(vault, vaultConfig, recoveryKey, recoveryKeyFactory, masterkeyFileAccess, new SimpleObjectProperty<>(RecoveryActionType.CONVERT_VAULT), new SimpleObjectProperty<>(null)); + static FxController provideRecoveryKeyValidateController(@ConvertVaultWindow Vault vault, + @ConvertVaultWindow VaultConfig.UnverifiedVaultConfig vaultConfig, + @ConvertVaultWindow StringProperty recoveryKey, + RecoveryKeyFactory recoveryKeyFactory, + MasterkeyFileAccess masterkeyFileAccess, + @Named("cipherCombo") javafx.beans.property.ObjectProperty<org.cryptomator.cryptolib.api.CryptorProvider.Scheme> cipherCombo) { + return new RecoveryKeyValidateController( + vault, vaultConfig, recoveryKey, recoveryKeyFactory, + masterkeyFileAccess, + new javafx.beans.property.SimpleObjectProperty<>(RecoveryActionType.CONVERT_VAULT), + cipherCombo); }Additionally add explicit imports (outside this hunk):
import javafx.beans.property.ObjectProperty; import org.cryptomator.cryptolib.api.CryptorProvider;If
@Named("cipherCombo")isn’t bound in your DI graph for this window scope, I can provide a provider method.
🧹 Nitpick comments (28)
src/main/resources/i18n/strings.properties (3)
401-401: Unify ellipsis style across menu itemsThis uses three dots "..."; elsewhere many entries use the ellipsis character "…". Pick one style and apply consistently (prefer “…” if that’s the project norm).
-main.vaultlist.addVaultBtn.menuItemRecover=Recover Existing Vault... +main.vaultlist.addVaultBtn.menuItemRecover=Recover Existing Vault…
440-442: Use user-friendly wording for missing config“VaultConfig is missing.” is technical. Suggest clearer copy.
-main.vaultDetail.missingVaultConfig.info=VaultConfig is missing. +main.vaultDetail.missingVaultConfig.info=Vault configuration file is missing. +main.vaultDetail.missingVaultConfig.restore=Restore Vault Configuration
524-527: Success messages: align title/message/description semanticsThe vault-config success uses the masterkey’s description text. Consider a dedicated description for the config case to avoid confusion.
+recoveryKey.recover.resetVaultConfigSuccess.description=Your vault configuration was restored. You can try unlocking your vault again.Then reference this description where appropriate in dialogs.
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java (3)
88-96: Prefer enum identity comparisonUse '==' for enums instead of equals().
- if(vault.getState().equals(VaultState.Value.ALL_MISSING)){ + if (vault.getState() == VaultState.Value.ALL_MISSING) {
28-29: Narrow visibility of recoverTypeMake the property private final to avoid unintended external mutation; expose a getter if needed by FXML.
- public ObjectProperty<RecoveryActionType> recoverType; + private final ObjectProperty<RecoveryActionType> recoverType;
49-67: Avoid stale nextScene if recoverType changesnextScene is computed once in the ctor. If recoverType is adjusted before pressing “Recover”, navigation may be stale. Compute the target lazily on click.
- this.nextScene = switch (recoverType.get()) { + // Defer selection to recover() to honor late changes to recoverType + this.nextScene = null; ... };And in recover():
- window.setScene(nextScene.get()); + final Lazy<Scene> target = switch (recoverType.get()) { + case RESTORE_ALL, RESTORE_VAULT_CONFIG -> expertSettingsScene; + case RESTORE_MASTERKEY, RESET_PASSWORD, SHOW_KEY -> resetPasswordScene; + default -> throw new IllegalArgumentException("Unexpected recovery action type: " + recoverType.get()); + }; + window.setScene(target.get());Also applies to: 101-103
src/main/resources/fxml/convertvault_hubtopassword_start.fxml (1)
22-30: Decorative icon: ensure it doesn’t affect focus/ATIf the icon is purely decorative, confirm it’s not focusable and has no accessible text. Defaults are usually fine; just flagging for UX/AT consistency.
src/main/resources/fxml/recoverykey_validate.fxml (1)
18-21: Replace filler label with a neutral spacerA hardcoded “Just some Filler” string (even invisible) is brittle and not localized. Use a Region as a spacer tied to the same managed binding.
- <VBox> - <Label text="Just some Filler" visible="false" managed="${textarea.text.empty}" graphicTextGap="6"> - <graphic> - <FontAwesome5IconView glyph="ANCHOR"/> - </graphic> - </Label> + <VBox> + <Region visible="false" managed="${textarea.text.empty}" minHeight="24"/>src/main/resources/fxml/recoverykey_reset_password.fxml (1)
25-33: Make decorative icon non-focusablePrevents tab focus on the purely decorative group.
- <Group> + <Group focusTraversable="false">src/main/resources/fxml/recoverykey_onboarding.fxml (3)
60-67: Remove unused extra RowConstraints or add the third rowThere are three RowConstraints but only two rows populated.
- <rowConstraints> - <RowConstraints valignment="TOP"/> - <RowConstraints valignment="TOP"/> - <RowConstraints valignment="TOP"/> - </rowConstraints> + <rowConstraints> + <RowConstraints valignment="TOP"/> + <RowConstraints valignment="TOP"/> + </rowConstraints>
32-40: Make decorative icon non-focusableSame rationale as in other recovery FXMLs.
- <Group> + <Group focusTraversable="false">
44-52: fx:id and controller field match; injection will succeed. ThechooseMethodeBoxidentifier is used consistently in both FXML and the controller, so there’s no runtime issue. Renaming it tochooseMethodBoxis purely an optional spelling cleanup—but if you choose to do so, update both the FXML and the controller field.src/main/java/org/cryptomator/common/recovery/VaultPreparator.java (1)
38-44:determineVaultStatereturn value ignoredIf this is meant for side effects only, fine; otherwise, consider logging or acting on the returned state.
- try { - VaultListManager.determineVaultState(vault.getPath()); + try { + var state = VaultListManager.determineVaultState(vault.getPath()); + LOG.debug("Determined vault state: {}", state);src/main/resources/fxml/recoverykey_recover.fxml (1)
25-33: Make decorative icon non-focusableConsistent with other views.
- <Group> + <Group focusTraversable="false">src/main/java/org/cryptomator/ui/mainwindow/VaultDetailMissingVaultController.java (1)
73-84: Optional: reduce redundant property accessMinor readability by caching
vault.get().- if (masterkeyFile != null) { - vault.get().getVaultSettings().path.setValue(masterkeyFile.toPath().toAbsolutePath().getParent()); - recheck(); - } + if (masterkeyFile != null) { + var v = vault.get(); + v.getVaultSettings().path.setValue(masterkeyFile.toPath().toAbsolutePath().getParent()); + recheck(); + }src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java (1)
56-56: Rename chooseMethodeBox → chooseMethodBox.Typo in identifier; rename for clarity and consistency (update FXML fx:id accordingly).
- private VBox chooseMethodeBox; + private VBox chooseMethodBox;Also replace all references in this class and the related FXML.
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyValidateController.java (3)
53-53: Prefer private @FXML fields over public for FXML injection.Encapsulate FXML fields; public is unnecessary.
- public TextArea textarea; + @FXML + private TextArea textarea;
51-51: Drop volatile; UI thread access is sufficient here.isWrongKey is only set/read on the FX thread; volatile is unnecessary noise.
- private volatile boolean isWrongKey; + private boolean isWrongKey;
139-171: Verify state mapping for exceptions during recovery-key validation.CryptoException likely indicates failure to load the generated masterkey (invalid key or corruption), not “valid but scheme couldn’t be determined.” Consider mapping to INVALID, and use WRONG only for a key valid-in-form but belonging to a different vault (you already have that path with checkKeyAgainstVaultConfig).
If you agree, adjust as:
- } catch (CryptoException e) { - LOG.info("Recovery key is valid but crypto scheme couldn't be determined", e); - recoveryKeyState.set(RecoveryKeyState.WRONG); + } catch (CryptoException e) { + LOG.info("Failed to load masterkey from recovery key", e); + recoveryKeyState.set(RecoveryKeyState.INVALID); }src/main/java/org/cryptomator/common/recovery/MasterkeyService.java (2)
44-46: Method signature is misleading - "validate" implies returning a boolean, not throwing exceptionsThe method name
validateRecoveryKeyAndDetectCombosuggests validation that returns a boolean result, but it actually throws exceptions on failure. Consider renaming to better reflect the throwing behavior or the detection focus.-public static CryptorProvider.Scheme validateRecoveryKeyAndDetectCombo(RecoveryKeyFactory recoveryKeyFactory, // - Vault vault, String recoveryKey, // - MasterkeyFileAccess masterkeyFileAccess) throws IOException, CryptoException, NoSuchElementException { +public static CryptorProvider.Scheme detectSchemeWithRecoveryKey(RecoveryKeyFactory recoveryKeyFactory, // + Vault vault, String recoveryKey, // + MasterkeyFileAccess masterkeyFileAccess) throws IOException, CryptoException, NoSuchElementException {
65-68: Consider using file filter to avoid loading directoriesThe current implementation reads all files under the data directory and then filters. Consider using file filtering during the walk operation to avoid traversing unnecessary directories.
try (Stream<Path> paths = Files.walk(vaultPath.resolve(DATA_DIR_NAME))) { Optional<Path> c9rFile = paths // + .filter(Files::isRegularFile) // .filter(p -> p.toString().endsWith(".c9r")) // .filter(p -> !p.endsWith("dir.c9r")) // .findFirst();src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyCreationController.java (2)
148-149: Improve error message for scheme detection failureThe generic IllegalStateException message doesn't provide enough context about why the combo detection failed.
var combo = MasterkeyService.detect(masterkey, vault.getPath()) - .orElseThrow(() -> new IllegalStateException("Could not detect combo for vault path: " + vault.getPath())); + .orElseThrow(() -> new IllegalStateException("Could not detect cipher scheme for vault at: " + vault.getPath() + ". The vault may be corrupted or using an unsupported encryption scheme."));
139-173: Consider extracting recovery logic to a service classThe
restoreWithPassword()method contains complex recovery logic that would be better placed in a dedicated service class for reusability and testing.Consider moving this recovery orchestration logic to a dedicated service class in the
org.cryptomator.common.recoverypackage, similar to howMasterkeyServicehandles master key operations. This would:
- Improve testability
- Enable reuse across different UI contexts
- Separate business logic from UI concerns
- Make the controller more focused on UI interactions
src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java (1)
233-244: Replace do-while loop with clearer control flowThe do-while loop with nullable check makes the control flow harder to follow. Consider a clearer structure.
-File selectedDirectory; - -do { - selectedDirectory = directoryChooser.showDialog(mainWindow); - if (selectedDirectory == null) { - return Optional.empty(); - } - - Path selectedPath = selectedDirectory.toPath(); - if (!Files.isDirectory(selectedPath.resolve(Constants.DATA_DIR_NAME))) { - dialogs.prepareNoDDirectorySelectedDialog(mainWindow).build().showAndWait(); - selectedDirectory = null; - } -} while (selectedDirectory == null); - -return Optional.of(selectedDirectory.toPath()); +while (true) { + File selectedDirectory = directoryChooser.showDialog(mainWindow); + if (selectedDirectory == null) { + return Optional.empty(); + } + + Path selectedPath = selectedDirectory.toPath(); + if (Files.isDirectory(selectedPath.resolve(Constants.DATA_DIR_NAME))) { + return Optional.of(selectedPath); + } + + dialogs.prepareNoDDirectorySelectedDialog(mainWindow).build().showAndWait(); +}src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java (4)
140-145: Clear sensitive data after successWipe recovery key and password fields after successful recovery/reset to reduce exposure in memory/UI.
window.close(); dialogs.prepareRecoverPasswordSuccess(window) // .setTitleKey("recover.recoverVaultConfig.title") // .setMessageKey("recoveryKey.recover.resetVaultConfigSuccess.message") // .build().showAndWait(); + recoveryKey.set(""); + newPasswordController.passwordField.clear(); + newPasswordController.reenterField.clear();LOG.debug("Used recovery key to reset password for {}.", vault.getDisplayablePath()); window.close(); switch (recoverType.get()){ case RESET_PASSWORD -> dialogs.prepareRecoverPasswordSuccess(window).build().showAndWait(); case RESTORE_MASTERKEY -> dialogs.prepareRecoverPasswordSuccess(window).setTitleKey("recover.recoverMasterkey.title").setMessageKey("recoveryKey.recover.resetMasterkeyFileSuccess.message").build().showAndWait(); } + recoveryKey.set(""); + newPasswordController.passwordField.clear(); + newPasswordController.reenterField.clear();Also applies to: 160-166
99-105: Keep button label in sync with recovery type changesIf
recoverTypechanges at runtime, the label won’t update. Add a listener (or bind) to keep it consistent.@FXML public void initialize() { - switch (recoverType.get()) { - case RESTORE_MASTERKEY, RESTORE_ALL -> nextButton.setText(resourceBundle.getString("recoveryKey.recover.recoverBtn")); - case RESET_PASSWORD -> nextButton.setText(resourceBundle.getString("recoveryKey.recover.resetBtn")); - } + updateNextButtonLabel(recoverType.get()); + recoverType.addListener((obs, oldVal, newVal) -> updateNextButtonLabel(newVal)); }Add this helper method (outside this hunk):
private void updateNextButtonLabel(RecoveryActionType type) { switch (type) { case RESTORE_MASTERKEY, RESTORE_ALL -> nextButton.setText(resourceBundle.getString("recoveryKey.recover.recoverBtn")); case RESET_PASSWORD -> nextButton.setText(resourceBundle.getString("recoveryKey.recover.resetBtn")); } }
181-183: Remove redundant onFailed handler inside taskYou set an
onFailedhandler on the task instance later; this constructor handler is overridden and never runs.- public ResetPasswordTask() { - setOnFailed(_ -> LOG.error("Failed to reset password", getException())); - } + public ResetPasswordTask() {}
64-64: Annotate FXML-injected button for clarityPublic fields don’t strictly require
@FXML, but annotating improves clarity and future-proofs if visibility changes.- public Button nextButton; + @FXML public Button nextButton;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
src/main/java/org/cryptomator/common/recovery/MasterkeyService.java(1 hunks)src/main/java/org/cryptomator/common/recovery/RecoveryDirectory.java(1 hunks)src/main/java/org/cryptomator/common/recovery/VaultPreparator.java(1 hunks)src/main/java/org/cryptomator/common/vaults/VaultListManager.java(4 hunks)src/main/java/org/cryptomator/ui/convertvault/ConvertVaultModule.java(3 hunks)src/main/java/org/cryptomator/ui/dialogs/Dialogs.java(4 hunks)src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/ChooseMasterkeyFileController.java(4 hunks)src/main/java/org/cryptomator/ui/mainwindow/VaultDetailMissingVaultController.java(3 hunks)src/main/java/org/cryptomator/ui/mainwindow/VaultListContextMenuController.java(2 hunks)src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java(9 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyCreationController.java(2 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyExpertSettingsController.java(1 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyModule.java(5 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java(1 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java(1 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java(2 hunks)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyValidateController.java(5 hunks)src/main/resources/fxml/convertvault_hubtopassword_convert.fxml(2 hunks)src/main/resources/fxml/convertvault_hubtopassword_start.fxml(2 hunks)src/main/resources/fxml/recoverykey_expert_settings.fxml(1 hunks)src/main/resources/fxml/recoverykey_onboarding.fxml(1 hunks)src/main/resources/fxml/recoverykey_recover.fxml(1 hunks)src/main/resources/fxml/recoverykey_reset_password.fxml(2 hunks)src/main/resources/fxml/recoverykey_validate.fxml(1 hunks)src/main/resources/fxml/vault_detail_missing_vault_config.fxml(1 hunks)src/main/resources/fxml/vault_list.fxml(1 hunks)src/main/resources/i18n/strings.properties(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/resources/fxml/vault_list.fxml
- src/main/resources/fxml/vault_detail_missing_vault_config.fxml
- src/main/java/org/cryptomator/ui/mainwindow/VaultListContextMenuController.java
- src/main/java/org/cryptomator/common/recovery/RecoveryDirectory.java
- src/main/resources/fxml/recoverykey_expert_settings.fxml
- src/main/java/org/cryptomator/ui/dialogs/Dialogs.java
- src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/ChooseMasterkeyFileController.java
- src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyExpertSettingsController.java
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-01-24T13:49:09.640Z
Learnt from: infeo
PR: cryptomator/cryptomator#3699
File: src/main/java/org/cryptomator/ui/convertvault/HubToPasswordConvertController.java:112-112
Timestamp: 2025-01-24T13:49:09.640Z
Learning: In the HubToPasswordConvertController, the convertInternal() method is specifically for vault conversion operations, while UI-related property updates (like setting keyLoader) should be handled in the UI completion handlers, maintaining a clear separation between vault operations and UI state management.
Applied to files:
src/main/java/org/cryptomator/common/vaults/VaultListManager.javasrc/main/resources/fxml/convertvault_hubtopassword_convert.fxmlsrc/main/java/org/cryptomator/ui/convertvault/ConvertVaultModule.java
📚 Learning: 2024-11-21T16:57:58.292Z
Learnt from: infeo
PR: cryptomator/cryptomator#3618
File: src/main/java/org/cryptomator/ui/fxapp/FxApplicationWindows.java:151-165
Timestamp: 2024-11-21T16:57:58.292Z
Learning: In the file `src/main/java/org/cryptomator/ui/fxapp/FxApplicationWindows.java`, when reviewing the method `showDokanySupportEndWindow`, suggestions to add error handling to asynchronous dialog creation (e.g., `CompletableFuture`) may be considered out of scope for certain pull requests.
Applied to files:
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.javasrc/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.javasrc/main/java/org/cryptomator/ui/mainwindow/VaultListController.java
📚 Learning: 2024-12-11T13:52:43.966Z
Learnt from: infeo
PR: cryptomator/cryptomator#3618
File: src/main/java/org/cryptomator/ui/dialogs/SimpleDialogController.java:55-67
Timestamp: 2024-12-11T13:52:43.966Z
Learning: In the `SimpleDialogController` class of the Cryptomator application, the actions passed to the `handleOk()` and `handleCancel()` methods are responsible for their own error handling. Therefore, wrapping these actions with try-catch blocks in the controller is unnecessary.
Applied to files:
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java
📚 Learning: 2024-11-24T16:41:49.825Z
Learnt from: purejava
PR: cryptomator/cryptomator#3311
File: src/main/java/org/cryptomator/ui/vaultoptions/MasterkeyOptionsController.java:52-56
Timestamp: 2024-11-24T16:41:49.825Z
Learning: In the Cryptomator project, null checks for JavaFX FXML injected components in the `initialize` method (e.g., in `MasterkeyOptionsController.java`) are unnecessary, as null values would indicate a severe issue with the JavaFX framework.
Applied to files:
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.javasrc/main/java/org/cryptomator/ui/convertvault/ConvertVaultModule.java
📚 Learning: 2024-11-24T16:47:01.698Z
Learnt from: purejava
PR: cryptomator/cryptomator#3311
File: src/main/java/org/cryptomator/ui/vaultoptions/MasterkeyOptionsController.java:0-0
Timestamp: 2024-11-24T16:47:01.698Z
Learning: In Cryptomator, avoid adding unnecessary complexity to error handling in UI controllers unless it's critical.
Applied to files:
src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java
🧬 Code graph analysis (10)
src/main/java/org/cryptomator/common/recovery/VaultPreparator.java (2)
src/main/java/org/cryptomator/common/settings/VaultSettings.java (1)
VaultSettings(32-167)src/main/java/org/cryptomator/common/vaults/VaultConfigCache.java (1)
VaultConfigCache(18-65)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyModule.java (5)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyCreationController.java (1)
RecoveryKeyScoped(43-198)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java (1)
RecoveryKeyScoped(33-161)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java (1)
RecoveryKeyScoped(20-111)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java (1)
RecoveryKeyScoped(42-202)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyExpertSettingsController.java (1)
RecoveryKeyScoped(25-123)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyValidateController.java (1)
src/main/java/org/cryptomator/common/recovery/MasterkeyService.java (1)
MasterkeyService(30-101)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyCreationController.java (6)
src/main/java/org/cryptomator/common/recovery/CryptoFsInitializer.java (1)
CryptoFsInitializer(15-33)src/main/java/org/cryptomator/common/recovery/MasterkeyService.java (1)
MasterkeyService(30-101)src/main/java/org/cryptomator/common/recovery/RecoveryDirectory.java (1)
RecoveryDirectory(12-56)src/main/java/org/cryptomator/ui/controls/FormattedLabel.java (1)
FormattedLabel(11-80)src/main/java/org/cryptomator/ui/controls/NiceSecurePasswordField.java (1)
NiceSecurePasswordField(14-106)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java (1)
RecoveryKeyScoped(42-202)
src/main/java/org/cryptomator/common/vaults/VaultListManager.java (1)
src/main/java/org/cryptomator/common/recovery/BackupRestorer.java (1)
BackupRestorer(15-53)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java (2)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java (1)
RecoveryKeyScoped(20-111)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyExpertSettingsController.java (1)
RecoveryKeyScoped(25-123)
src/main/java/org/cryptomator/common/recovery/MasterkeyService.java (1)
src/main/java/org/cryptomator/common/recovery/RecoveryDirectory.java (1)
RecoveryDirectory(12-56)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java (4)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyCreationController.java (1)
RecoveryKeyScoped(43-198)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java (1)
RecoveryKeyScoped(33-161)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java (1)
RecoveryKeyScoped(42-202)src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyExpertSettingsController.java (1)
RecoveryKeyScoped(25-123)
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java (4)
src/main/java/org/cryptomator/common/recovery/CryptoFsInitializer.java (1)
CryptoFsInitializer(15-33)src/main/java/org/cryptomator/common/recovery/MasterkeyService.java (1)
MasterkeyService(30-101)src/main/java/org/cryptomator/common/recovery/RecoveryDirectory.java (1)
RecoveryDirectory(12-56)src/main/java/org/cryptomator/ui/changepassword/NewPasswordController.java (1)
NewPasswordController(19-100)
src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java (1)
src/main/java/org/cryptomator/common/recovery/VaultPreparator.java (1)
VaultPreparator(20-54)
🔇 Additional comments (18)
src/main/resources/fxml/convertvault_hubtopassword_convert.fxml (2)
25-33: Two-column header looks goodIcon header integration and spacing are consistent with the new pattern. No functional issues spotted.
34-36: Ensure included controller wiring is correctConfirm that the controller consuming new_password.fxml exposes the expected fields (e.g., password validators) and that no duplicate padding/margins create double spacing with the surrounding VBox.
src/main/resources/i18n/strings.properties (1)
528-569: All newrecover.*keys are in use
Verified that each added key is referenced in Java controllers or FXML. No obsolete keys detected.src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java (1)
49-67: Handled unexpected action types — goodThrowing IllegalArgumentException in the default branch prevents NPEs later in the flow.
src/main/resources/fxml/convertvault_hubtopassword_start.fxml (1)
37-41: Binding valid: HubToPasswordStartController#getValidateController() returns RecoveryKeyValidateControllersrc/main/resources/fxml/recoverykey_validate.fxml (1)
24-39: Good use of managed bindings to avoid layout jumpsThe visibility/managed coupling on status labels is correct and prevents flicker.
src/main/resources/fxml/recoverykey_reset_password.fxml (1)
43-43: No action needed:passwordSufficientAndMatchingProperty()returns a ReadOnlyBooleanProperty, so the FXML binding will update as expected.src/main/resources/fxml/recoverykey_onboarding.fxml (1)
42-74: All referenced i18n keys in recoverykey_onboarding.fxml are present in the property files.src/main/java/org/cryptomator/common/recovery/VaultPreparator.java (2)
46-50: Harden Dropbox heuristic and mount provider selection
- Match path segments case-insensitively instead of substring-based check.
- Confirm WinFspMountProvider exists on classpath rather than relying on a hardcoded class-name string.
- var nameOfWinfspLocalMounter = "org.cryptomator.frontend.fuse.mount.WinFspMountProvider"; - if (SystemUtils.IS_OS_WINDOWS && vaultSettings.path.get().toString().contains("Dropbox") && mountServices.stream().anyMatch(s -> s.getClass().getName().equals(nameOfWinfspLocalMounter))) { + final var nameOfWinfspLocalMounter = "org.cryptomator.frontend.fuse.mount.WinFspMountProvider"; + final var isDropboxPath = SystemUtils.IS_OS_WINDOWS && + java.util.stream.StreamSupport.stream(vaultSettings.path.get().normalize().spliterator(), false) + .anyMatch(p -> p.toString().equalsIgnoreCase("Dropbox")); + final var hasWinFsp = mountServices.stream().anyMatch(s -> s.getClass().getName().equals(nameOfWinfspLocalMounter)); + if (isDropboxPath && hasWinFsp) { vaultSettings.mountService.setValue(nameOfWinfspLocalMounter); }Manually verify that
org.cryptomator.frontend.fuse.mount.WinFspMountProvideris present on the classpath; code search did not locate its declaration.
35-36: i18n key verified
Thedefaults.vault.vaultNameentry is present instrings.properties(line 33) and all localizedstrings_*.propertiesfiles undersrc/main/resources/i18n.src/main/resources/fxml/recoverykey_recover.fxml (1)
40-44: Good: Back usescloseOrReturnand Next is defaultMatches the updated flow for recovery.
src/main/java/org/cryptomator/ui/mainwindow/VaultDetailMissingVaultController.java (2)
36-48: Constructor DI: order and usage look goodNew
RecoveryKeyComponent.Factoryinjection is consistent and stored.
60-71: FXML handler correctly wired
The restore button invault_detail_missing_vault_config.fxmlalready uses onAction="#restoreVaultConfig", so no further changes are needed.src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java (1)
58-60: Remove the FXML injection suggestion for ToggleGroup.methodToggleGroupisn’t annotated with @FXML and no fx:id="methodToggleGroup" exists in any FXML, so manual instantiation is correct.Likely an incorrect or invalid review comment.
src/main/java/org/cryptomator/common/vaults/VaultListManager.java (1)
69-73: Nice utility.isAlreadyAdded() reads cleanly and avoids duplicate entries.
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyModule.java (1)
139-142: LGTM! Good use of property initialization with sensible defaultsThe shortening threshold property is properly initialized with the maximum value, which is a safe default for vault recovery scenarios.
src/main/java/org/cryptomator/ui/convertvault/ConvertVaultModule.java (1)
7-25: LGTM: import additions align with the new recovery flowNew imports look consistent with the RecoveryKey flow wiring. No issues.
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java (1)
4-14: LGTM: imports and static constants usageNew imports and static constants are coherent with the added recovery workflow.
Also applies to: 18-19, 24-33, 35-41, 39-41
| private void initializeLastKnownKeyLoaderIfPossible(VaultSettings vaultSettings, VaultState.Value vaultState, VaultConfigCache wrapper) throws IOException { | ||
| if (vaultSettings.lastKnownKeyLoader.get() != null) { | ||
| return; | ||
| } | ||
|
|
||
| switch (vaultState) { | ||
| case LOCKED -> { | ||
| wrapper.reloadConfig(); | ||
| vaultSettings.lastKnownKeyLoader.set(wrapper.get().getKeyId().getScheme()); | ||
| } | ||
| case NEEDS_MIGRATION -> { | ||
| //for legacy reasons: pre v8 vault do not have a config, but they are in the NEEDS_MIGRATION state | ||
| vaultSettings.lastKnownKeyLoader.set(MasterkeyFileLoadingStrategy.SCHEME); | ||
| } | ||
| case VAULT_CONFIG_MISSING -> { | ||
| //Nothing to do here, since there is no config to read | ||
| } | ||
| default -> { | ||
| vaultSettings.lastKnownKeyLoader.set(wrapper.get().getKeyId().getScheme()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard lastKnownKeyLoader initialization to avoid reading config in “missing” states.
default branch calls wrapper.get() even when config can be absent (MISSING/ALL_MISSING/ERROR), causing IOEs and flipping state to ERROR.
private void initializeLastKnownKeyLoaderIfPossible(VaultSettings vaultSettings, VaultState.Value vaultState, VaultConfigCache wrapper) throws IOException {
if (vaultSettings.lastKnownKeyLoader.get() != null) {
return;
}
switch (vaultState) {
case LOCKED -> {
wrapper.reloadConfig();
vaultSettings.lastKnownKeyLoader.set(wrapper.get().getKeyId().getS
802E
cheme());
}
case NEEDS_MIGRATION -> {
//for legacy reasons: pre v8 vault do not have a config, but they are in the NEEDS_MIGRATION state
vaultSettings.lastKnownKeyLoader.set(MasterkeyFileLoadingStrategy.SCHEME);
}
case VAULT_CONFIG_MISSING -> {
//Nothing to do here, since there is no config to read
}
- default -> {
- vaultSettings.lastKnownKeyLoader.set(wrapper.get().getKeyId().getScheme());
- }
+ case MISSING, ALL_MISSING, ERROR, PROCESSING -> {
+ // no config available or not safe to load
+ }
+ default -> {
+ if (Files.exists(vaultSettings.path.get().resolve(VAULTCONFIG_FILENAME))) {
+ try {
+ wrapper.reloadConfig();
+ vaultSettings.lastKnownKeyLoader.set(wrapper.get().getKeyId().getScheme());
+ } catch (IOException e) {
+ LOG.debug("Unable to load config for {}", vaultSettings.path.get(), e);
+ }
+ }
+ }
}
}There was a problem hiding this comment.
@mindmonk this should be implemented
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!
| VaultState.Value structureResult = checkDirStructure(pathToVault); | ||
|
|
||
| if (structureResult == LOCKED || structureResult == NEEDS_MIGRATION) { | ||
| return structureResult; | ||
| } | ||
|
|
||
| Path pathToVaultConfig = pathToVault.resolve(VAULTCONFIG_FILENAME); | ||
| Path pathToMasterkey = pathToVault.resolve(MASTERKEY_FILENAME); | ||
|
|
||
| if (!Files.exists(pathToVaultConfig)) { | ||
| BackupRestorer.restoreIfBackupPresent(pathToVault, VAULTCONFIG_FILENAME); | ||
| } | ||
| if (!Files.exists(pathToMasterkey)) { | ||
| BackupRestorer.restoreIfBackupPresent(pathToVault, MASTERKEY_FILENAME); | ||
| } | ||
|
|
||
| boolean hasConfig = Files.exists(pathToVaultConfig); | ||
|
|
||
| if (!hasConfig && !Files.exists(pathToMasterkey)) { | ||
| return ALL_MISSING; | ||
| } | ||
| if (!hasConfig) { | ||
| return VAULT_CONFIG_MISSING; | ||
| } | ||
|
|
||
| return structureResult; | ||
| } |
There was a problem hiding this comment.
Recompute structure after potential restoration; current result can be stale.
structureResult is computed before backup restoration; returning it after files are restored can yield an incorrect state.
VaultState.Value structureResult = checkDirStructure(pathToVault);
if (structureResult == LOCKED || structureResult == NEEDS_MIGRATION) {
return structureResult;
}
Path pathToVaultConfig = pathToVault.resolve(VAULTCONFIG_FILENAME);
Path pathToMasterkey = pathToVault.resolve(MASTERKEY_FILENAME);
if (!Files.exists(pathToVaultConfig)) {
BackupRestorer.restoreIfBackupPresent(pathToVault, VAULTCONFIG_FILENAME);
}
if (!Files.exists(pathToMasterkey)) {
BackupRestorer.restoreIfBackupPresent(pathToVault, MASTERKEY_FILENAME);
}
boolean hasConfig = Files.exists(pathToVaultConfig);
if (!hasConfig && !Files.exists(pathToMasterkey)) {
return ALL_MISSING;
}
if (!hasConfig) {
return VAULT_CONFIG_MISSING;
}
- return structureResult;
+ // Files may have been restored; re-evaluate structure:
+ return checkDirStructure(pathToVault);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| VaultState.Value structureResult = checkDirStructure(pathToVault); | |
| if (structureResult == LOCKED || structureResult == NEEDS_MIGRATION) { | |
| return structureResult; | |
| } | |
| Path pathToVaultConfig = pathToVault.resolve(VAULTCONFIG_FILENAME); | |
| Path pathToMasterkey = pathToVault.resolve(MASTERKEY_FILENAME); | |
| if (!Files.exists(pathToVaultConfig)) { | |
| BackupRestorer.restoreIfBackupPresent(pathToVault, VAULTCONFIG_FILENAME); | |
| } | |
| if (!Files.exists(pathToMasterkey)) { | |
| BackupRestorer.restoreIfBackupPresent(pathToVault, MASTERKEY_FILENAME); | |
| } | |
| boolean hasConfig = Files.exists(pathToVaultConfig); | |
| if (!hasConfig && !Files.exists(pathToMasterkey)) { | |
| return ALL_MISSING; | |
| } | |
| if (!hasConfig) { | |
| return VAULT_CONFIG_MISSING; | |
| } | |
| return structureResult; | |
| } | |
| VaultState.Value structureResult = checkDirStructure(pathToVault); | |
| if (structureResult == LOCKED || structureResult == NEEDS_MIGRATION) { | |
| return structureResult; | |
| } | |
| Path pathToVaultConfig = pathToVault.resolve(VAULTCONFIG_FILENAME); | |
| Path pathToMasterkey = pathToVault.resolve(MASTERKEY_FILENAME); | |
| if (!Files.exists(pathToVaultConfig)) { | |
| BackupRestorer.restoreIfBackupPresent(pathToVault, VAULTCONFIG_FILENAME); | |
| } | |
| if (!Files.exists(pathToMasterkey)) { | |
| BackupRestorer.restoreIfBackupPresent(pathToVault, MASTERKEY_FILENAME); | |
| } | |
| boolean hasConfig = Files.exists(pathToVaultConfig); | |
| if (!hasConfig && !Files.exists(pathToMasterkey)) { | |
| return ALL_MISSING; | |
| } | |
| if (!hasConfig) { | |
| return VAULT_CONFIG_MISSING; | |
| } | |
| // Files may have been restored; re-evaluate directory structure: | |
| return checkDirStructure(pathToVault); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/common/vaults/VaultListManager.java around
lines 195-221, the method computes structureResult before attempting
BackupRestorer.restoreIfBackupPresent, then returns the earlier result which may
be stale after restoration; update the flow so that backups are restored first
(or, after calling restoreIfBackupPresent for VAULTCONFIG and MASTERKEY, call
checkDirStructure(pathToVault) again to recompute structureResult) and then base
the subsequent checks/returns on the recomputed value (also recompute any
dependent booleans like hasConfig if needed) so the returned VaultState reflects
the post-restore state.
There was a problem hiding this comment.
@mindmonk I follow the argument of the agent. This should be implemented and tested.
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!
src/main/java/org/cryptomator/ui/mainwindow/VaultDetailMissingVaultController.java
Show resolved
Hide resolved
| switch (preparedVault.getState()) { | ||
| case VAULT_CONFIG_MISSING -> recoveryKeyWindow.create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_VAULT_CONFIG)).showOnboardingDialogWindow(); | ||
| case ALL_MISSING -> recoveryKeyWindow.create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_ALL)).showOnboardingDialogWindow(); | ||
| case LOCKED, NEEDS_MIGRATION -> { | ||
| vaultListManager.addVault(preparedVault); | ||
| dialogs.prepareRecoveryVaultAdded(mainWindow, preparedVault.getDisplayName()).setOkAction(Stage::close).build().showAndWait(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for unexpected vault states
The switch statement doesn't handle all possible vault states. Add a default case to handle unexpected states gracefully.
switch (preparedVault.getState()) {
case VAULT_CONFIG_MISSING -> recoveryKeyWindow.create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_VAULT_CONFIG)).showOnboardingDialogWindow();
case ALL_MISSING -> recoveryKeyWindow.create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_ALL)).showOnboardingDialogWindow();
case LOCKED, NEEDS_MIGRATION -> {
vaultListManager.addVault(preparedVault);
dialogs.prepareRecoveryVaultAdded(mainWindow, preparedVault.getDisplayName()).setOkAction(Stage::close).build().showAndWait();
}
+ default -> {
+ LOG.warn("Unexpected vault state during recovery: {}", preparedVault.getState());
+ dialogs.prepareErrorDialog(mainWindow)
+ .setMessageKey("recovery.unexpectedState")
+ .build().showAndWait();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (preparedVault.getState()) { | |
| case VAULT_CONFIG_MISSING -> recoveryKeyWindow.create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_VAULT_CONFIG)).showOnboardingDialogWindow(); | |
| case ALL_MISSING -> recoveryKeyWindow.create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_ALL)).showOnboardingDialogWindow(); | |
| case LOCKED, NEEDS_MIGRATION -> { | |
| vaultListManager.addVault(preparedVault); | |
| dialogs.prepareRecoveryVaultAdded(mainWindow, preparedVault.getDisplayName()).setOkAction(Stage::close).build().showAndWait(); | |
| } | |
| } | |
| switch (preparedVault.getState()) { | |
| case VAULT_CONFIG_MISSING -> | |
| recoveryKeyWindow | |
| .create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_VAULT_CONFIG)) | |
| .showOnboardingDialogWindow(); | |
| case ALL_MISSING -> | |
| recoveryKeyWindow | |
| .create(preparedVault, mainWindow, new SimpleObjectProperty<>(RecoveryActionType.RESTORE_ALL)) | |
| .showOnboardingDialogWindow(); | |
| case LOCKED, NEEDS_MIGRATION -> { | |
| vaultListManager.addVault(preparedVault); | |
| dialogs | |
| .prepareRecoveryVaultAdded(mainWindow, preparedVault.getDisplayName()) | |
| .setOkAction(Stage::close) | |
| .build() | |
| .showAndWait(); | |
| } | |
| default -> { | |
| LOG.warn("Unexpected vault state during recovery: {}", preparedVault.getState()); | |
| dialogs | |
| .prepareErrorDialog(mainWindow) | |
| .setMessageKey("recovery.unexpectedState") | |
| .build() | |
| .showAndWait(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/cryptomator/ui/mainwindow/VaultListController.java around
lines 278-286, the switch on preparedVault.getState() lacks a default branch;
add a default case that handles unexpected states by logging the unexpected
state (including preparedVault id/displayName and state) and presenting a
user-facing error dialog (using the existing dialogs/dialogs.prepareError or
similar) so the user is informed and the condition is not silently ignored;
ensure the default branch does not alter application invariants (do not add the
vault or proceed with recovery) and return/exit the switch after handling.
| ResourceBundle resourceBundle) { | ||
| this.window = window; | ||
| this.vault = vault; | ||
| window.setTitle(resourceBundle.getString("recover.recoverVaultConfig.title")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid setting the window title twice; remove the unconditional constructor title.
initialize() already sets the title based on recoverType. The unconditional set here can briefly show the wrong title and is redundant.
- window.setTitle(resourceBundle.getString("recover.recoverVaultConfig.title"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| window.setTitle(resourceBundle.getString("recover.recoverVaultConfig.title")); |
🤖 Prompt for AI Agents
In
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java
around line 71, the constructor unconditionally calls
window.setTitle(resourceBundle.getString("recover.recoverVaultConfig.title"))
which duplicates and conflicts with the title set later in initialize() based on
recoverType; remove this unconditional setTitle call from the constructor so
only initialize() sets the window title dynamically.
There was a problem hiding this comment.
@mindmonk what is the reason for setting the title already here?
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!
| messageLabel.textProperty().bind(Bindings.createStringBinding(() -> | ||
| vault.getState() == VaultState.Value.ALL_MISSING | ||
| ? resourceBundle.getString("recover.onBoarding.allMissing.intro") | ||
| : resourceBundle.getString("recover.onBoarding.intro"))); | ||
|
|
There was a problem hiding this comment.
Bind messageLabel to vault.stateProperty() so it updates when the state changes.
The binding misses observable dependencies; it won’t react to state changes.
- messageLabel.textProperty().bind(Bindings.createStringBinding(() ->
- vault.getState() == VaultState.Value.ALL_MISSING
- ? resourceBundle.getString("recover.onBoarding.allMissing.intro")
- : resourceBundle.getString("recover.onBoarding.intro")));
+ messageLabel.textProperty().bind(Bindings.createStringBinding(() ->
+ vault.getState() == VaultState.Value.ALL_MISSING
+ ? resourceBundle.getString("recover.onBoarding.allMissing.intro")
+ : resourceBundle.getString("recover.onBoarding.intro"),
+ vault.stateProperty()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| messageLabel.textProperty().bind(Bindings.createStringBinding(() -> | |
| vault.getState() == VaultState.Value.ALL_MISSING | |
| ? resourceBundle.getString("recover.onBoarding.allMissing.intro") | |
| : resourceBundle.getString("recover.onBoarding.intro"))); | |
| messageLabel.textProperty().bind(Bindings.createStringBinding(() -> | |
| vault.getState() == VaultState.Value.ALL_MISSING | |
| ? resourceBundle.getString("recover.onBoarding.allMissing.intro") | |
| : resourceBundle.getString("recover.onBoarding.intro"), | |
| vault.stateProperty())); |
🤖 Prompt for AI Agents
In
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyOnboardingController.java
around lines 102 to 106, the StringBinding for messageLabel is created without
any observable dependencies so it won’t update when the vault state changes; fix
it by passing the vault.stateProperty() as a dependency to
Bindings.createStringBinding (e.g. Bindings.createStringBinding(() -> { ... },
vault.stateProperty())) so the binding will re-evaluate when the state changes.
There was a problem hiding this comment.
@mindmonk we don't need a binding here, since in the onboarding screen we can safely assume that nothing changes. So drop the whole binding and set the messageLabel directly.
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!
| @FXML | ||
| public void restorePassword() { | ||
| try (RecoveryDirectory recoveryDirectory = RecoveryDirectory.create(vault.getPath())) { | ||
| Path recoveryPath = recoveryDirectory.getRecoveryPath(); | ||
| MasterkeyService.recoverFromRecoveryKey(recoveryKey.get(), recoveryKeyFactory, recoveryPath, newPasswordController.passwordField.getCharacters()); | ||
|
|
||
| try (Masterkey masterkey = MasterkeyService.load(masterkeyFileAccess, recoveryPath.resolve(MASTERKEY_FILENAME), newPasswordController.passwordField.getCharacters())) { | ||
| CryptoFsInitializer.init(recoveryPath, masterkey, shorteningThreshold.get(), cipherCombo.get()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Null-safe scheme handling before initializing CryptoFS
cipherCombo.get() may be null here, which would cause a failure in CryptoFsInitializer.init. Detect and set the scheme if missing, using the already-loaded masterkey.
- try (Masterkey masterkey = MasterkeyService.load(masterkeyFileAccess, recoveryPath.resolve(MASTERKEY_FILENAME), newPasswordController.passwordField.getCharacters())) {
- CryptoFsInitializer.init(recoveryPath, masterkey, shorteningThreshold.get(), cipherCombo.get());
- }
+ try (Masterkey masterkey = MasterkeyService.load(masterkeyFileAccess, recoveryPath.resolve(MASTERKEY_FILENAME), newPasswordController.passwordField.getCharacters())) {
+ CryptorProvider.Scheme scheme = cipherCombo.get();
+ if (scheme == null) {
+ java.util.Optional<CryptorProvider.Scheme> detected = MasterkeyService.detect(masterkey, vault.getPath());
+ if (detected.isEmpty()) {
+ throw new IllegalStateException("Unable to detect Crypto scheme from *.c9r");
+ }
+ scheme = detected.get();
+ cipherCombo.set(scheme);
+ }
+ CryptoFsInitializer.init(recoveryPath, masterkey, shorteningThreshold.get(), scheme);
+ }Also widen the catch to surface a user-friendly error:
- } catch (IOException | CryptoException e) {
+ } catch (IOException | CryptoException | IllegalStateException e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @FXML | |
| public void restorePassword() { | |
| try (RecoveryDirectory recoveryDirectory = RecoveryDirectory.create(vault.getPath())) { | |
| Path recoveryPath = recoveryDirectory.getRecoveryPath(); | |
| MasterkeyService.recoverFromRecoveryKey(recoveryKey.get(), recoveryKeyFactory, recoveryPath, newPasswordController.passwordField.getCharacters()); | |
| try (Masterkey masterkey = MasterkeyService.load(masterkeyFileAccess, recoveryPath.resolve(MASTERKEY_FILENAME), newPasswordController.passwordField.getCharacters())) { | |
| CryptoFsInitializer.init(recoveryPath, masterkey, shorteningThreshold.get(), cipherCombo.get()); | |
| } | |
| @FXML | |
| public void restorePassword() { | |
| try (RecoveryDirectory recoveryDirectory = RecoveryDirectory.create(vault.getPath())) { | |
| Path recoveryPath = recoveryDirectory.getRecoveryPath(); | |
| MasterkeyService.recoverFromRecoveryKey( | |
| recoveryKey.get(), | |
| recoveryKeyFactory, | |
| recoveryPath, | |
| newPasswordController.passwordField.getCharacters() | |
| ); | |
| try (Masterkey masterkey = MasterkeyService.load( | |
| masterkeyFileAccess, | |
| recoveryPath.resolve(MASTERKEY_FILENAME), | |
| newPasswordController.passwordField.getCharacters() | |
| )) { | |
| // Null-safe scheme handling | |
| CryptorProvider.Scheme scheme = cipherCombo.get(); | |
| if (scheme == null) { | |
| java.util.Optional<CryptorProvider.Scheme> detected = MasterkeyService.detect(masterkey, vault.getPath()); | |
| if (detected.isEmpty()) { | |
| throw new IllegalStateException("Unable to detect Crypto scheme from *.c9r"); | |
| } | |
| scheme = detected.get(); | |
| cipherCombo.set(scheme); | |
| } | |
| CryptoFsInitializer.init(recoveryPath, masterkey, shorteningThreshold.get(), scheme); | |
| } | |
| } catch (IOException | CryptoException | IllegalStateException e) { | |
| // existing error handling… | |
| dialogs.prepareRecoverPasswordError(window, owner, resourceBundle) | |
| .withException(e) | |
| .build() | |
| .showAndWait(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyResetPasswordController.java
around lines 124 to 133, handle a potential null cipher scheme and broaden error
handling: after loading the Masterkey, if cipherCombo.get() returns null,
derive/set the cipher scheme from the loaded masterkey (use its stored
scheme/version) before calling CryptoFsInitializer.init; then widen the
surrounding catch to catch Exception (or a broader relevant superclass) and
convert it into a clear user-facing error message/log entry so failures during
init surface as a friendly error rather than an uncaught null-related crash.
| <buttons> | ||
| <Button text="%generic.button.cancel" ButtonBar.buttonData="CANCEL_CLOSE" cancelButton="true" onAction="#close"/> | ||
| <Button fx:id="cancelButton" ButtonBar.buttonData="BACK_PREVIOUS" cancelButton="true" onAction="#closeOrReturn"/> | ||
| <Button text="%generic.button.next" ButtonBar.buttonData="NEXT_FORWARD" defaultButton="true" onAction="#recover" disable="${!controller.validateController.recoveryKeyCorrect}"/> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify nested property binding
Confirm validateController.recoveryKeyCorrect is exposed and observable.
🏁 Script executed:
#!/bin/bash
rg -nP 'class\s+RecoveryKeyRecoverController\b' -C3 src/main/java
rg -n 'validateController\s*\.recoveryKeyCorrect' -C2 src/main/java
rg -n 'void\s+closeOrReturn\s*\(' -C2 src/main/javaLength of output: 1466
🏁 Script executed:
#!/bin/bash
set -e
echo "=== fx:include occurrences in recoverykey_recover.fxml ==="
rg -n "fx:include" -C3 src/main/resources/fxml/recoverykey_recover.fxml || echo "none"
echo "=== validateController occurrences in recoverykey_recover.fxml ==="
rg -n "validateController" -C3 src/main/resources/fxml/recoverykey_recover.fxml || echo "none"
echo "=== ValidateController class definitions ==="
rg -nP "class\s+\w*Validate\w*Controller" -C3 src/main/java || echo "none"
echo "=== recoveryKeyCorrect occurrences in code ==="
rg -n "recoveryKeyCorrect" -C3 src || echo "none"
echo "=== recoveryKeyCorrectProperty occurrences in code ==="
rg -n "recoveryKeyCorrectProperty" -C3 src || echo "none"Length of output: 8970
🏁 Script executed:
#!/bin/bash
set -e
echo "=== @FXML field declarations in RecoveryKeyRecoverController ==="
rg -n "@FXML" -C3 src/main/java/org/cryptomator/ui/recoverykey/RecoveryKeyRecoverController.java || echo "none"Length of output: 952
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' src/main/resources/fxml/recoverykey_recover.fxmlLength of output: 1671
Fix nested property binding path
The FXML uses controller.validateController.recoveryKeyCorrect, but the parent controller only exposes @FXML RecoveryKeyValidateController recoveryKeyValidateController. Update the binding to:
disable="${!controller.recoveryKeyValidateController.recoveryKeyCorrect}"so it matches the injected field and hooks into the recoveryKeyCorrectProperty().
🤖 Prompt for AI Agents
In src/main/resources/fxml/recoverykey_recover.fxml around line 43, the disable
binding reference
D73A
s controller.validateController.recoveryKeyCorrect but the
parent controller injects @FXML RecoveryKeyValidateController
recoveryKeyValidateController; change the binding to use
controller.recoveryKeyValidateController.recoveryKeyCorrect so it matches the
injected field name and resolves to the recoveryKeyCorrectProperty() for proper
nested property binding.
| <ButtonBar buttonMinWidth="120" buttonOrder="+CI"> | ||
| <buttons> | ||
| <Button text="%generic.button.back" ButtonBar.buttonData="CANCEL_CLOSE" cancelButton="true" onAction="#close"/> | ||
| <Button fx:id="nextButton" ButtonBar.buttonData="FINISH" defaultButton="true" disable="${!controller.passwordSufficientAndMatching}" onAction="#next"/> | ||
| </buttons> |
There was a problem hiding this comment.
Localize the FINISH button text and align Back semantics with other recovery views
- The FINISH button lacks a text resource; this will fall back to platform defaults and break i18n.
- The Back button still calls
#close; other recovery views use#closeOrReturnwithBACK_PREVIOUS.
Apply:
- <ButtonBar buttonMinWidth="120" buttonOrder="+CI">
+ <ButtonBar buttonMinWidth="120" buttonOrder="+BI">
...
- <Button text="%generic.button.back" ButtonBar.buttonData="CANCEL_CLOSE" cancelButton="true" onAction="#close"/>
- <Button fx:id="nextButton" ButtonBar.buttonData="FINISH" defaultButton="true" disable="${!controller.passwordSufficientAndMatching}" onAction="#next"/>
+ <Button text="%generic.button.back" ButtonBar.buttonData="BACK_PREVIOUS" cancelButton="true" onAction="#closeOrReturn"/>
+ <Button fx:id="nextButton" text="%generic.button.finish"
+ ButtonBar.buttonData="FINISH" defaultButton="true"
+ disable="${!controller.passwordSufficientAndMatching}" onAction="#next"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ButtonBar buttonMinWidth="120" buttonOrder="+CI"> | |
| <buttons> | |
| <Button text="%generic.button.back" ButtonBar.buttonData="CANCEL_CLOSE" cancelButton="true" onAction="#close"/> | |
| <Button fx:id="nextButton" ButtonBar.buttonData="FINISH" defaultButton="true" disable="${!controller.passwordSufficientAndMatching}" onAction="#next"/> | |
| </buttons> | |
| <ButtonBar buttonMinWidth="120" buttonOrder="+BI"> | |
| <buttons> | |
| - <Button text="%generic.button.back" ButtonBar.buttonData="CANCEL_CLOSE" cancelButton="true" onAction="#close"/> | |
| <Button text="%generic.button.back" | |
| ButtonBar.buttonData="BACK_PREVIOUS" | |
| cancelButton="true" | |
| onAction="#closeOrReturn"/> | |
| <Button fx:id="nextButton" | |
| text="%generic.button.finish" | |
| ButtonBar.buttonData="FINISH" | |
| defaultButton="true" | |
| disable="${!controller.passwordSufficientAndMatching}" | |
| onAction="#next"/> | |
| </buttons> |
🤖 Prompt for AI Agents
In src/main/resources/fxml/recoverykey_reset_password.fxml lines 40-44, the
FINISH button is missing a localized text resource and the Back button uses the
wrong action; change the nextButton to use a localized text key (e.g.
text="%generic.button.finish") instead of relying on platform defaults, and
change the Back button's onAction from "#close" to "#closeOrReturn" and its
ButtonBar.buttonData to the BACK_PREVIOUS semantic used by other recovery views
so it behaves consistently.
| main.vaultlist.contextMenu.reveal=Reveal Drive | ||
| main.vaultlist.addVaultBtn.menuItemNew=Create New Vault... | ||
| main.vaultlist.addVaultBtn.menuItemExisting=Open Existing Vault... | ||
| main.vaultlist.addVaultBtn.menuItemRecover=Recover Existing Vault... |
There was a problem hiding this comment.
Why not use the ellipsis? Currently three periods are used.
| main.vaultDetail.missingVaultConfig.info=VaultConfig is missing. | ||
| main.vaultDetail.missingVaultConfig.restore=Restore VaultConfig |
There was a problem hiding this comment.
Don't write VaultConfig, it would become an official term. Use "vault config" or "vault configuration" as used elsewhere.
| ##Add Existing Vault without recovery - Dialog | ||
| recover.existing.title=Vault Added | ||
| recover.existing.message=The vault was added successfully | ||
| recover.existing.description=Your vault "%s" has been added to your vault list. No recovery process was started. |
There was a problem hiding this comment.
- Your vault "%s" has been added to the vault list. No recovery process was started.
+ Your vault "%s" has been added to the vault list. No recovery process was necessary.| ##Vault Already Exists - Dialog | ||
| recover.alreadyExists.title=Vault Already Exists | ||
| recover.alreadyExists.message=This vault has already been added | ||
| recover.alreadyExists.description=Your vault "%s" is already present in your vault list and was therefore not added again. |
There was a problem hiding this comment.
Can we mark the vault directly in the vault list in the main windo instead of showing an end dialog?
| recover.onBoarding.intro=Make sure to check the following: | ||
| recover.onBoarding.allMissing.intro=If your vault is a Hub vault, the vault owner can restore the file for you. Otherwise: | ||
| recover.onBoarding.intro.ensure=Ensure all files are completely synced. | ||
| recover.onBoarding.affirmation=I have read and understood the above information |
There was a problem hiding this comment.
I would use a more formal wording:
If this vault is managed by Cryptomator Hub, the vault owner must restore it for you.
Before proceeding, please confirm that:
* All files are fully synced.
* You have the vault recovery key available.
[x] I have read and understood these requirements.
|
|
||
| ###Vault Config Missing | ||
| recover.onBoarding.intro.recoveryKey=You will need the recovery key, a new vault password and possibly some expert settings. | ||
| recover.onBoarding.intro.password=You will need the vault password and possibly some expert settings. |
There was a problem hiding this comment.
You have the vault password available and know if expert settings were applied
This PR enhances the recovery functionality. The following is an overview of the key modifications and improvements:
1. Recovery Utility
A new utility class has been introduced with methods to detect the encryption scheme (using a .c9r file), restore missing configuration and master key files from backups, and perform recovery tasks.
Cipher Scheme Detection: Uses a provided master key and a sample file to determine if the vault employs one of the supported cipher schemes (e.g., SIV_GCM or SIV_CTRMAC).
Backup Restoration: Automatically searches for backup files for missing vault configuration or master key files and restores them if available.
Recovery Directory Handling: Creates a temporary recovery directory, generates a new master key file using the recovery key, initialises the crypto file system, moves the recovered files back to the vault directory, and then cleans up the temporary directory.
2. Enhancements to Vault and VaultState
In addition to the previous status values, new states have been added for missing vault configuration (VAULT_CONFIG_MISSING) and for scenarios where both the configuration and master key are missing (ALL_MISSING).
3. UI/UX Enhancements and Recovery Key Workflow
New Controllers and Dialogs:
Several new UI components have been added under the org.cryptomator.ui.recoverykey package to support the recovery key flow:
4. Module and Dependency Injection Updates
Dependency Injection:
Modules such as ConvertVaultModule, RecoveryKeyModule, and MainWindowModule have been updated to integrate the new recovery key components into the DI graph.