Add Done button to change-password success screen - #659
Conversation
Return users to Home after a successful password change, using the same bottom button pattern as Unlock wallet. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ghost
left a comment
There was a problem hiding this comment.
Review: ✅ Approved
Scope: Pure UI change — adds a "Done" button on the change-password success screen that navigates to Pages.Wallet. No protocol, crypto, key-handling, or security logic is touched.
What I checked
-
src/components/Success.tsx— NewSuccessDoneButtonexport is a clean, minimal wrapper around the existingButtonsOnBottom+Buttoncomponents. No issues. -
src/screens/Settings/Password.tsx— The ternary{successText ? <SuccessDoneButton .../> : <ButtonsOnBottom>...}correctly replaces the previous{successText ? null : ...}pattern. Navigation targetPages.Walletis the standard root page. Import ofNavigationContext+Pagesfollows the same pattern asInit/Success.tsxand other screens. -
Cross-repo impact: None. No public API, type, proto, or interface changes. The
SuccessDoneButtonexport is internal to the wallet app. No other repo imports fromwallet/src/components/Success. -
Existing consumers of
Success.tsx:Init/Success.tsxandWallet/Notes/Success.tsxuse the default export only — the new named export doesn't affect them. -
No protocol-critical code touched: Password change flow (
saveNewPassword) is unchanged. Key derivation, mnemonic handling, biometrics registration — all untouched. This PR only changes what happens after a successful password change (showing a button instead of nothing). -
Pattern consistency: Matches
Init/Success.tsxwhich does the sameuseContext(NavigationContext)+navigate(Pages.X)pattern inside aButtonsOnBottom.
No issues found.
🤖 Reviewed by Arkana
ghost
left a comment
There was a problem hiding this comment.
Review: ✅ Approved
Scope: Pure UI change — adds a "Done" button on the change-password success screen that navigates to Pages.Wallet. No protocol, crypto, key-handling, or security logic is touched.
What I checked
-
src/components/Success.tsx— NewSuccessDoneButtonexport is a clean, minimal wrapper around the existingButtonsOnBottom+Buttoncomponents. No issues. -
src/screens/Settings/Password.tsx— The ternary{successText ? <SuccessDoneButton .../> : <ButtonsOnBottom>...}correctly replaces the previous{successText ? null : ...}pattern. Navigation targetPages.Walletis the standard root page. Import ofNavigationContext+Pagesfollows the same pattern asInit/Success.tsxand other screens. -
Cross-repo impact: None. No public API, type, proto, or interface changes. The
SuccessDoneButtonexport is internal to the wallet app. No other repo imports fromwallet/src/components/Success. -
Existing consumers of
Success.tsx:Init/Success.tsxandWallet/Notes/Success.tsxuse the default export only — the new named export doesn't affect them. -
No protocol-critical code touched: Password change flow (
saveNewPassword) is unchanged. Key derivation, mnemonic handling, biometrics registration — all untouched. This PR only changes what happens after a successful password change (showing a button instead of nothing). -
Pattern consistency: Matches
Init/Success.tsxwhich does the sameuseContext(NavigationContext)+navigate(Pages.X)pattern inside aButtonsOnBottom.
No issues found.
🤖 Reviewed by Arkana
|
In my opinion this could be done in a simpler way:
Full diff: |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Small, self-contained UI change. Not protocol-critical (no VTXO / signing / forfeit / round / exit surface touched). Behaviour matches the pattern used in src/screens/Wallet/Unlock.tsx:20 (navigate(Pages.Wallet)), and Pages.Wallet is a registered root page (src/providers/navigation.tsx:89), so the navigation target is safe.
Diff review — src/components/Success.tsx:14-20 and src/screens/Settings/Password.tsx:14,21,103-105:
- Correctness:
SuccessDoneButtonrenders<ButtonsOnBottom><Button label='Done' /></ButtonsOnBottom>, which is structurally identical to the fallback branch that was previously hidden on success. The success screen used to have no bottom CTA at all, so this is a pure UX addition — no regression to the other flows, which was the PR's explicit intent. - No new failure modes:
onClickonly callsnavigate(Pages.Wallet); no async, no promise swallowing, no state mutation. The Header'sbackcontrol is still present, so users retain both paths. - Cross-repo: React component internal to
wallet; grep confirmsSuccessDoneButtonis only referenced fromSettings/Password.tsx. No public API/proto/type change. - Danger: passed.
Nits (non-blocking):
- Naming —
SuccessDoneButtonlives inSuccess.tsxbut doesn't render<Success>; it's just a footer button. If this helper is expected to stay Password-only, inlining the 3-line JSX inPassword.tsxavoids the coupling; if it's meant to be reused across other success screens (Init/Success.tsx,Wallet/Send/Success.tsx,Wallet/Receive/Success.tsx,Wallet/Notes/Success.tsx,Apps/Assets/MintSuccess.tsx), those callsites still have their own bespoke CTAs — worth a follow-up decision. - No test added.
src/test/App.test.tsx:226renders the Password settings screen but only asserts navbar visibility. A brief render-and-click test on the success state would lock the new navigation, though not strictly required for a change this size. 'Done'is a hardcoded string — consistent with the rest of the codebase, which has no i18n layer, so fine.
No security, key-handling, or protocol concerns. LGTM once the nits above are considered.
Summary
Pages.Wallet).Context
Developed and tested locally with Cursor on branch
feat/success-done-button-home.Test plan
Sounds good,Go to wallet, etc.)Made with Cursor