Add lock wallet confirmation modal and settings toggle #1009
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds a confirmation modal for the lock wallet action to prevent accidental wallet locking, along with a settings toggle to enable or disable this confirmation. The feature is integrated into both the settings page and the app navbar.
Changes:
- Added
showLockWalletConfirmationboolean setting to the jam settings store (defaults to true) - Created a new
LockWalletConfirmDialogcomponent that displays warnings when maker is running or coinjoin is in progress - Integrated the confirmation modal into both the SettingsPage and AppNavbar components with lock wallet flow modifications
- Added translation key for the new settings toggle
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/store/jamSettingsStore.ts | Added showLockWalletConfirmation boolean property with default value of true |
| src/i18n/locales/en/translation.json | Added translation key show_lock_confirmation for the settings toggle label |
| src/components/settings/LockWalletConfirmDialog.tsx | New dialog component displaying lock wallet confirmation with contextual warnings for active maker or coinjoin |
| src/components/settings/SettingsPage.tsx | Integrated confirmation modal, added settings toggle, and modified lock wallet handler to check confirmation setting |
| src/components/layout/AppNavbar.tsx | Integrated confirmation modal and modified lock wallet button to check confirmation setting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const onClickLockWallet = () => { | ||
| if (showLockWalletConfirmation) { | ||
| setShowLockWalletDialog(true) | ||
| } else { | ||
| handleLockWallet() |
There was a problem hiding this comment.
The onClickLockWallet function should be async and should await the call to handleLockWallet() on line 178 to ensure proper error handling. Currently, if handleLockWallet() fails when the confirmation is disabled, the error would be silently ignored. The SettingsPage implementation correctly uses await for this call.
| const onClickLockWallet = () => { | |
| if (showLockWalletConfirmation) { | |
| setShowLockWalletDialog(true) | |
| } else { | |
| handleLockWallet() | |
| const onClickLockWallet = async () => { | |
| if (showLockWalletConfirmation) { | |
| setShowLockWalletDialog(true) | |
| } else { | |
| await handleLockWallet() |
| const handleLockWallet = async () => { | ||
| try { | ||
| setIsLockingWallet(true) | ||
| await onLockWallet(navigate, t) | ||
| } finally { | ||
| setIsLockingWallet(false) | ||
| setShowLockWalletDialog(false) | ||
| } | ||
| } | ||
|
|
||
| const onClickLockWallet = () => { | ||
| if (showLockWalletConfirmation) { | ||
| setShowLockWalletDialog(true) | ||
| } else { | ||
| handleLockWallet() | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between this implementation and the identical logic in SettingsPage.tsx (lines 62-78). Consider extracting the handleLockWallet and onClickLockWallet functions into a shared custom hook (e.g., useWalletLocking) to reduce duplication and ensure consistent behavior across both components.
|
Looks nice! I think the confirmation dialog should only be shown when a service is active (earn, send, sweep) and does not need a dedicated setting. I'd merge it nonetheless - these things can be done in follow-up PRs. What do you think? |
|
agreed, let's merge and iterate you are right on both points: Conditional dialog: The setting is unnecessary we should just check maker_running || coinjoin_in_process and show the dialog only when services are active simpler UX, less configuration. i will create follow-up issus for both: remove the setting and make dialog conditional on service state thanks for the review |
Done. 🧡
Yeah, I will handle it so you can focus on other stuff if you want 🙏 Edit: See #1011 |
2 participants
fixes #944
Added a confirmation modal for locking the wallet to prevent accidental actions. Integrated the modal into the settings page and app navbar, with a toggle in settings to enable/disable it. .