Update VerifyExistingVault screen design and routing#4002
Conversation
…ting vault keygen Route vault settings single keygen through VerifyExistingVault screen with dedicated string resources for email/password steps and UI refinements. Co-Authored-By: aminsato <Amin.saradar@yahoo.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a two-step "Verify Existing Vault" flow: new route and serializable Route, Compose screen, Hilt ViewModel, UI state/events, repositories-driven password verification, navigation changes from VaultSettings, and localized strings for the new UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as VerifyExistingVaultScreen
participant VM as VerifyExistingVaultViewModel
participant VR as VaultRepository
participant VS as VultiSignerRepository
participant Nav as Navigator
User->>UI: input email -> Next
UI->>VM: onEvent(Next)
VM->>VM: validate email -> advance to Password step
UI->>User: show password input
User->>UI: input password -> Next
UI->>VM: onEvent(Next)
VM->>VR: loadVault(vaultId)
VR-->>VM: vault (or null)
VM->>VS: checkPassword(vault.pubKeyECDSA, password)
VS-->>VM: PasswordCheckResult (Valid / Invalid / Error)
alt Valid
VM->>Nav: navigate(Route.Keygen.PeerDiscovery)
else Invalid or Error
VM->>UI: update password innerState & errorMessage
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: aminsato <Amin.saradar@yahoo.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt`:
- Around line 1-365: The file fails ktfmt formatting; run the Kotlin formatter
on VerifyExistingVaultViewModel.kt (e.g., run ktfmt or the project's gradle
ktfmtFormat/ktfmtApply task or use IDE Kotlin code style -> Reformat) and commit
the formatted file so class VerifyExistingVaultViewModel and related
declarations (functions like observeActiveStepState, collectStepStates,
validateCurrentStep, verifyPasswordAndNavigate) conform to the project's ktfmt
rules and the CI ktfmtCheckMain passes.
- Around line 169-203: collectStepStates recomputes the step map,
inputTextFieldState and hint but does not reset isNextButtonEnabled, so when
activeStep changes the new step can inherit the previous step's enabled state;
update the uiState.update call inside collectStepStates (the block combining
activeStep and steps) to set isNextButtonEnabled = false when writing the new
state (use the same uiState.update invocation that sets stepAndStates,
inputTextFieldState, and textFieldHint) so VerifyExistingVaultStepType
transitions (and associated emailTextFieldState/passwordTextFieldState) always
start with Next disabled.
- Around line 225-236: prev() currently allows changing steps while
verifyPasswordAndNavigate() is still running; add a guard so prev() returns
early if verification is in progress. Specifically, use the ViewModel's uiState
boolean flag that indicates ongoing work (e.g., isVerifying/isBusy) or add one
to the state, set it true at the start of verifyPasswordAndNavigate() and false
on completion/error, and in prev() check that flag before computing nextIndex;
if verification is running, do not navigate or change activeStep. Ensure
references to verifyPasswordAndNavigate(), prev(), uiState, activeStep,
stepAndStates, and navigator.navigate remain consistent.
In
`@app/src/main/java/com/vultisig/wallet/ui/screens/keygen/VerifyExistingVaultScreen.kt`:
- Around line 164-167: The UI currently applies
Modifier.shinedBottom(Theme.v2.colors.alerts.success) for every
non-VerifyExistingVaultStepState.Done state which causes inactive circles to
show the success shine; change the condition so the success shine is applied
only when this particular step is the active/in-progress step (e.g., compare the
step's identifier/index to the current active step or use an isActive boolean
exposed to VerifyExistingVaultScreen) rather than any state != Done, updating
the code that uses shinedBottom in VerifyExistingVaultScreen so only the active
step receives the green highlight.
- Around line 1-219: The file fails ktfmt formatting; run the project's ktfmt
formatter (or apply the project's Kotlin/IDE formatting rules) on
VerifyExistingVaultScreen.kt to fix whitespace, import ordering and line breaks
so ktfmtCheckMain passes; target the top-level Composables
VerifyExistingVaultScreen (both overloads), VerifyExistingVaultSteps,
VerifyExistingVaultStepIcon and the VerifyExistingVaultTags object when
verifying formatting changes. Ensure you commit the reformatted file without
changing behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86067abd-f769-43ff-b679-511ba9e3ecf6
📒 Files selected for processing (15)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/navigation/NavGraph.ktapp/src/main/java/com/vultisig/wallet/ui/navigation/Navigation.ktapp/src/main/java/com/vultisig/wallet/ui/screens/keygen/VerifyExistingVaultScreen.ktapp/src/main/java/com/vultisig/wallet/ui/screens/vault_settings/VaultSettingsViewModel.ktapp/src/main/res/values-de/strings.xmlapp/src/main/res/values-es/strings.xmlapp/src/main/res/values-hr/strings.xmlapp/src/main/res/values-it/strings.xmlapp/src/main/res/values-ko/strings.xmlapp/src/main/res/values-nl/strings.xmlapp/src/main/res/values-pt/strings.xmlapp/src/main/res/values-ru/strings.xmlapp/src/main/res/values-zh-rCN/strings.xmlapp/src/main/res/values/strings.xml
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt (2)
203-208:⚠️ Potential issue | 🟡 MinorReset CTA enabled state when switching steps.
collectStepStates()updates the active field/hint but does not resetisNextButtonEnabled, so Password can inherit Email’s enabled state.Suggested fix
uiState.update { it.copy( stepAndStates = map, inputTextFieldState = textFieldState, textFieldHint = hint, + isNextButtonEnabled = false, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt` around lines 203 - 208, collectStepStates() updates stepAndStates, inputTextFieldState and textFieldHint via uiState.update but doesn't reset isNextButtonEnabled, causing a newly selected step (e.g., Password) to inherit the previous step's enabled state; modify the uiState.update call in VerifyExistingVaultViewModel.collectStepStates() to explicitly set isNextButtonEnabled = false (or compute the correct default) whenever you swap stepAndStates/inputTextFieldState/textFieldHint so the next-button state is reset on step change.
234-245:⚠️ Potential issue | 🟠 MajorPrevent back-step changes while verification is running.
prev()still allows step changes duringverifyPasswordAndNavigate(), so successful verification can navigate forward from a stale UI step.Suggested fix
private fun prev() { + if (uiState.value.isLoading) return viewModelScope.launch { val currentStep = uiState.value.activeStep🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt` around lines 234 - 245, The prev() function allows navigating back while a verification is in progress; guard against this by checking a verification-in-progress flag on uiState before changing steps (e.g., if uiState.value.isVerifying return early), and ensure verifyPasswordAndNavigate() sets and clears that flag on start/finish so the UIState (used by prev(), uiState.update { ... }, and the verifyPasswordAndNavigate() coroutine) prevents step changes during verification; add the boolean to the state, set it true at the start of verifyPasswordAndNavigate() and false on all exits, and make prev() return immediately when that flag is true.
🧹 Nitpick comments (1)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt (1)
87-98: Use sealed UI states instead of mixed flags in one data class.This state model allows invalid combinations (
isLoading+ error + editable state together). For this ViewModel, a sealed UI state hierarchy would align with project rules and make transitions safer.As per coding guidelines, "Use sealed classes for UI state (Loading, Success, Error)" and "use sealed state classes for error states."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt` around lines 87 - 98, Replace the flat VerifyExistingVaultUiState data class with a sealed UI-state hierarchy (sealed class VerifyExistingVaultUiState with subclasses like Loading, Input, Success, Error) so invalid flag combinations (e.g., isLoading + error + editable) cannot occur; move relevant fields into the appropriate subclass variants (e.g., Loading contains no inputs, Input contains inputTextFieldState, textFieldHint, innerState, isPasswordVisible, isNextButtonEnabled, stepAndStates and activeStep belong where appropriate, Error holds UiText errorMessage), update the ViewModel to emit the new sealed subclasses and adjust any consumers to switch/when on VerifyExistingVaultUiState instead of reading multiple boolean flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt`:
- Around line 203-208: collectStepStates() updates stepAndStates,
inputTextFieldState and textFieldHint via uiState.update but doesn't reset
isNextButtonEnabled, causing a newly selected step (e.g., Password) to inherit
the previous step's enabled state; modify the uiState.update call in
VerifyExistingVaultViewModel.collectStepStates() to explicitly set
isNextButtonEnabled = false (or compute the correct default) whenever you swap
stepAndStates/inputTextFieldState/textFieldHint so the next-button state is
reset on step change.
- Around line 234-245: The prev() function allows navigating back while a
verification is in progress; guard against this by checking a
verification-in-progress flag on uiState before changing steps (e.g., if
uiState.value.isVerifying return early), and ensure verifyPasswordAndNavigate()
sets and clears that flag on start/finish so the UIState (used by prev(),
uiState.update { ... }, and the verifyPasswordAndNavigate() coroutine) prevents
step changes during verification; add the boolean to the state, set it true at
the start of verifyPasswordAndNavigate() and false on all exits, and make prev()
return immediately when that flag is true.
---
Nitpick comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt`:
- Around line 87-98: Replace the flat VerifyExistingVaultUiState data class with
a sealed UI-state hierarchy (sealed class VerifyExistingVaultUiState with
subclasses like Loading, Input, Success, Error) so invalid flag combinations
(e.g., isLoading + error + editable) cannot occur; move relevant fields into the
appropriate subclass variants (e.g., Loading contains no inputs, Input contains
inputTextFieldState, textFieldHint, innerState, isPasswordVisible,
isNextButtonEnabled, stepAndStates and activeStep belong where appropriate,
Error holds UiText errorMessage), update the ViewModel to emit the new sealed
subclasses and adjust any consumers to switch/when on VerifyExistingVaultUiState
instead of reading multiple boolean flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34df0556-bd58-4686-ac22-4b2a03eefab2
📒 Files selected for processing (2)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/screens/keygen/VerifyExistingVaultScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/vultisig/wallet/ui/screens/keygen/VerifyExistingVaultScreen.kt
Reset isNextButtonEnabled when active step changes to prevent stale button state, and block Back navigation while password verification is in progress. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt (1)
145-150: Prefer an explicit step list overstepAndStates.keysfor flow order.Back/next currently depend on the map's iteration order. A dedicated step list makes the transition order obvious and keeps navigation independent from how the UI map is built.
♻️ Suggested refactor
+ private val steps = + listOf( + VerifyExistingVaultStepType.Email, + VerifyExistingVaultStepType.Password, + ) + private fun initStepStates() { val stepAndStates = - mapOf( - VerifyExistingVaultStepType.Email to VerifyExistingVaultStepState.InProgress, - VerifyExistingVaultStepType.Password to VerifyExistingVaultStepState.Inactive, - ) + steps.associateWith { step -> + if (step == VerifyExistingVaultStepType.Email) { + VerifyExistingVaultStepState.InProgress + } else { + VerifyExistingVaultStepState.Inactive + } + } uiState.update { it.copy(stepAndStates = stepAndStates) } } @@ private fun collectStepStates() { uiState .map { it.activeStep } .distinctUntilChanged() - .combine(uiState.map { it.stepAndStates.keys }) { activeStep, steps -> + .onEach { activeStep -> val map = steps.associateWith { step -> when { step == activeStep -> VerifyExistingVaultStepState.InProgress step.ordinal < activeStep.ordinal -> VerifyExistingVaultStepState.Done @@ uiState.update { it.copy( stepAndStates = map, inputTextFieldState = textFieldState, textFieldHint = hint, isNextButtonEnabled = isNextButtonEnabled, ) } } .launchIn(viewModelScope) } @@ - val currentStepIndex = uiState.value.stepAndStates.keys.indexOf(currentStep) + val currentStepIndex = steps.indexOf(currentStep) val nextIndex = currentStepIndex - 1 @@ - val newStep = uiState.value.stepAndStates.keys.elementAt(nextIndex) + val newStep = steps[nextIndex] uiState.update { it.copy(activeStep = newStep) } @@ - val currentStepIndex = uiState.value.stepAndStates.keys.indexOf(currentStep) + val currentStepIndex = steps.indexOf(currentStep) val nextIndex = currentStepIndex + 1 - if (nextIndex >= uiState.value.stepAndStates.size) { + if (nextIndex >= steps.size) { verifyPasswordAndNavigate() return@launch } - val newStep = uiState.value.stepAndStates.keys.elementAt(nextIndex) + val newStep = steps[nextIndex] uiState.update { it.copy(activeStep = newStep) }Also applies to: 175-219, 246-253, 264-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt` around lines 145 - 150, The current navigation relies on the map iteration order of stepAndStates (constructed from VerifyExistingVaultStepType keys), which is brittle; introduce an explicit ordered list (e.g., orderedSteps or stepOrder: List<VerifyExistingVaultStepType>) and store it in the UI state alongside stepAndStates, then change all places that iterate stepAndStates.keys or assume map order (including logic in methods handling back/next navigation and any references around verify flow) to use this ordered list for determining previous/next steps; update uiState.update to set both stepAndStates and the new orderedSteps and refactor navigation helpers to use orderedSteps instead of map keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt`:
- Around line 297-300: In VerifyExistingVaultViewModel's onError lambda, stop
using the raw exception text (e.message) for passwordErrorMessage; keep
Timber.e(e, ...) to log details, but map the UI error to a stable localized
fallback (e.g., set passwordErrorMessage to a UiText.StringResource pointing to
a "invalid password" / "unable to verify password" string ID) and retain
passwordInnerState.value = VsTextInputFieldInnerState.Error; ensure no exception
details are shown to the user and only logged via Timber.
---
Nitpick comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt`:
- Around line 145-150: The current navigation relies on the map iteration order
of stepAndStates (constructed from VerifyExistingVaultStepType keys), which is
brittle; introduce an explicit ordered list (e.g., orderedSteps or stepOrder:
List<VerifyExistingVaultStepType>) and store it in the UI state alongside
stepAndStates, then change all places that iterate stepAndStates.keys or assume
map order (including logic in methods handling back/next navigation and any
references around verify flow) to use this ordered list for determining
previous/next steps; update uiState.update to set both stepAndStates and the new
orderedSteps and refactor navigation helpers to use orderedSteps instead of map
keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de2c2251-f97b-4c21-a933-73e0f313a04a
📒 Files selected for processing (1)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt
- Make uiState private MutableStateFlow, expose as StateFlow - Restore @immutable on UiState by moving TextFieldState out - Move VerifyExistingVaultStepType and StepState enums to Screen file to remove Compose UI types (Color, Dp) from ViewModel layer - Apply shinedBottom only for InProgress step, not all non-Done steps - Replace raw exception text with localized string in error handler - Consolidate isNextButtonEnabled into single source of truth via combine Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt (1)
245-249:⚠️ Potential issue | 🟡 MinorUse one localized fallback for unexpected password-check failures.
onErrorcurrently shows the invalid-password copy for any thrown exception, whilePasswordCheckResult.Errorexposesresult.messagedirectly. Unexpected failures shouldn’t either masquerade as bad credentials or leak repository text; add one generic localized resource and use it in both branches.🧩 Suggested fix
viewModelScope.safeLaunch( onError = { e -> Timber.e(e, "Failed to verify vault password") passwordInnerState.value = VsTextInputFieldInnerState.Error - passwordErrorMessage.value = StringResource(R.string.fast_vault_invalid_password) + passwordErrorMessage.value = + StringResource(R.string.verify_existing_vault_unable_to_verify_password) _uiState.update { it.copy(isLoading = false) } } ) { @@ is PasswordCheckResult.Error -> { passwordInnerState.value = VsTextInputFieldInnerState.Error - passwordErrorMessage.value = UiText.DynamicString(result.message) + passwordErrorMessage.value = + StringResource(R.string.verify_existing_vault_unable_to_verify_password) }Also applies to: 282-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt` around lines 245 - 249, The onError lambda and the PasswordCheckResult.Error branch in VerifyExistingVaultViewModel currently surface either the "invalid password" string or the raw repository message; create a single generic localized string (e.g., R.string.fast_vault_password_check_failed) and set passwordErrorMessage.value to StringResource(R.string.fast_vault_password_check_failed) for unexpected failures: in onError replace the current invalid-password resource with the generic resource, and in the PasswordCheckResult.Error branch use the generic resource instead of result.message (or use it when result.message is null/empty), leaving the explicit invalid-password resource only for the PasswordCheckResult.InvalidPassword case; update usages of passwordErrorMessage and any tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/screens/keygen/VerifyExistingVaultScreen.kt`:
- Around line 171-177: The Modifier.shinedBottom is currently only applied when
state == VerifyExistingVaultStepState.InProgress, but per spec it should also
apply for VerifyExistingVaultStepState.Inactive and be omitted only for
VerifyExistingVaultStepState.Done; update the conditional inside
VerifyExistingVaultStepIcon (the .then(...) block) to apply
Modifier.shinedBottom(color = Theme.v2.colors.alerts.success) when state is
InProgress OR Inactive and return plain Modifier only when state is Done,
keeping the same color and placement.
---
Duplicate comments:
In
`@app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.kt`:
- Around line 245-249: The onError lambda and the PasswordCheckResult.Error
branch in VerifyExistingVaultViewModel currently surface either the "invalid
password" string or the raw repository message; create a single generic
localized string (e.g., R.string.fast_vault_password_check_failed) and set
passwordErrorMessage.value to
StringResource(R.string.fast_vault_password_check_failed) for unexpected
failures: in onError replace the current invalid-password resource with the
generic resource, and in the PasswordCheckResult.Error branch use the generic
resource instead of result.message (or use it when result.message is
null/empty), leaving the explicit invalid-password resource only for the
PasswordCheckResult.InvalidPassword case; update usages of passwordErrorMessage
and any tests accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 763ff43b-ac4d-4d37-b1c3-1fc93f2125a4
📒 Files selected for processing (2)
app/src/main/java/com/vultisig/wallet/ui/models/keygen/VerifyExistingVaultViewModel.ktapp/src/main/java/com/vultisig/wallet/ui/screens/keygen/VerifyExistingVaultScreen.kt
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Route.VerifyExistingVaultand wired it into NavGraph so vault settings routes single keygen through the verify existing vault screenVerifyExistingVaultScreenwith dedicated string resources for email/password steps and simplified the button label logicInProgressstep border width from 3dp to 1.5dpverify_existing_vault_email_description,verify_existing_vault_password_title,verify_existing_vault_password_description) across all 10 locale filesCloses #3956
Closes #3957
Test plan
VerifyExistingVaultScreen🤖 Generated with Claude Code
Summary by CodeRabbit