refactor nsec backup #250
refactor nsec backup #250
Conversation
WalkthroughReplaces the Settings → Backup private-key flow with a modal-driven reveal requiring biometric or password verification; adds Modal and InputFake components, four new SVG icons, many style typings, small UI styling tweaks, testId additions, and e2e test updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/NeedsPassword.tsx (1)
32-37: Avoid double-triggering biometrics on clickWith
onClick={handleBiometrics}onCenterScreenand also on theButton, clicking the button will callauthenticateUsertwice (once via the button, once bubbling to the parent div). This can cause duplicated biometric prompts or odd UX.A simple fix is to let the container handle the click and keep the button as visual only:
- <CenterScreen onClick={handleBiometrics}> + <CenterScreen onClick={handleBiometrics}> <LockIcon big /> <Text centered>Unlock with your passkey</Text> - <Button onClick={handleBiometrics} label='Unlock wallet' /> + <Button label='Unlock wallet' />
🧹 Nitpick comments (3)
src/screens/Settings/Backup.tsx (3)
32-50: Clear authentication error on successful unlockRight now
setError('Invalid password')is only called in thecatch, and there’s no corresponding reset on success. If the user enters a wrong password once and then a correct one, the old “Invalid password” message will still be present when they return to the main view.You can clear the error when
getPrivateKeysucceeds:useEffect(() => { const pass = password ? password : defaultPassword getPrivateKey(pass) - .then((privateKey) => { - setNsec(privateKeyToNsec(privateKey)) - }) + .then((privateKey) => { + setError('') + setNsec(privateKeyToNsec(privateKey)) + })If you want to keep backup-related errors independent of password errors, consider splitting them into separate state variables as a follow-up refactor.
83-118: Dialog’s inline password prompt is effectively dead codeBecause the parent only renders
<Dialog />whendialogis true andnsecis truthy, the!nsecconditional insideDialog:{!nsec ? <p>Enter your password</p> : null} {!nsec ? <input type='password' onChange={(e) => setPassword(e.target.value)} /> : null}will never be hit. The password flow is now handled by
NeedsPassword.To reduce confusion and maintenance overhead, consider removing this unused branch entirely:
- {!nsec ? <p>Enter your password</p> : null} - {!nsec ? <input type='password' onChange={(e) => setPassword(e.target.value)} /> : null}
120-161: Private key reveal & copy flow is well-gated; consider optional hardeningThe new flow—masking the key by default, requiring a dialog + explicit confirm to set
showNsec, and only wiringhandleCopyto the “Copy to clipboard” button in theshowNsecbranch—appropriately gates access to the private key and matches the intent of keeping it hidden by default. Based on learnings, this is consistent with the pattern of only exposing sensitive material after explicit user action.If this screen can remain mounted while the user navigates elsewhere, you might optionally clear
nsec/showNsecwhen leaving the page (e.g., in an effect cleanup or on route change) so the key isn’t immediately visible when returning, mirroring the non-persistence approach used for the restore input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/CenterScreen.tsx(1 hunks)src/components/NeedsPassword.tsx(3 hunks)src/components/Shadow.tsx(1 hunks)src/icons/Dont.tsx(1 hunks)src/icons/Safe.tsx(1 hunks)src/icons/X.tsx(1 hunks)src/screens/Settings/Backup.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsxsrc/components/NeedsPassword.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsxsrc/components/NeedsPassword.tsx
🧬 Code graph analysis (2)
src/screens/Settings/Backup.tsx (9)
src/components/CenterScreen.tsx (1)
CenterScreen(9-28)src/components/Shadow.tsx (1)
Shadow(18-55)src/components/FlexCol.tsx (1)
FlexCol(15-40)src/components/Text.tsx (1)
Text(25-79)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Button.tsx (1)
Button(21-63)src/components/Header.tsx (1)
Header(17-53)src/components/NeedsPassword.tsx (1)
NeedsPassword(20-55)src/components/Warning.tsx (1)
WarningBox(12-38)
src/components/NeedsPassword.tsx (1)
src/components/Button.tsx (1)
Button(21-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (6)
src/components/CenterScreen.tsx (1)
19-19: Max-width increase is fineBumping
maxWidthto18remis a safe layout tweak and doesn’t affect behavior.src/components/Shadow.tsx (1)
33-42: Background fallback change is purely visualSwitching the default background to
var(--dark10)only affects theming; no behavioral impact.src/icons/X.tsx (1)
1-11: XIcon component looks goodSelf-contained SVG icon with
currentColorfill is fine and consistent with typical icon patterns.src/icons/Safe.tsx (1)
1-11: SafeIcon SVG is straightforward and reusableStateless SVG icon with
currentColorfill is correctly implemented.src/icons/Dont.tsx (1)
1-18: DontIcon SVG implementation is fineIcon structure (clipPath + single path) is valid and consistent with the other new icons.
src/components/NeedsPassword.tsx (1)
39-49: Password unlock flow wiring looks solidInline password input, error display via
ErrorMessage, and the explicit “Unlock wallet” button correctly route throughonPassword(password)and align with the existing parent handling.
|
Just Backup (no "and privacy" in "Backup and Privacy" in Settings |
|
the impl is not really 1:1 with the design the confirmation modal should just overlay (not change the background), and def be bigger as per design. |
|
also the warnign notification box should be transprent orange and stronger orange I think on the text 
|
Deploying wallet-bitcoin with
|
| Latest commit: |
5572692
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c5a1c1e.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://refactor-nsec-backup.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Warning.tsx (1)
2-35: Remove unusedInfoIconimport and align text color with variant
InfoIconis imported but never used; this triggers the static‑analysis warning and should be removed.- The warning text color is hard‑coded to orange via
color: 'var(--orange)', even whenredorgreenare set. That likely makes red/green variants visually inconsistent with their border/icon color.A minimal refactor would reuse the per‑variant color for text:
-import InfoIcon, { InfoIconDark } from '../icons/Info' +import { InfoIconDark } from '../icons/Info' export default function WarningBox({ green, red, text }: WarningProps) { const backgroundColor = red ? 'var(--redbg)' : green ? 'var(--greenbg)' : 'var(--orangebg)' const borderColor = red ? 'var(--red)' : green ? 'var(--green)' : 'var(--orange)' - const Icon = () => (red ? <ForbidIcon /> : <InfoIconDark />) + const Icon = () => (red ? <ForbidIcon /> : <InfoIconDark />) - const style = { + const style = { backgroundColor, borderRadius: '0.5rem', - color: 'var(--orange)', + color: borderColor, padding: '0.75rem 1rem', width: '100%', }This keeps the new orange styling for the default case while making red/green variants consistent with their semantic color.
🧹 Nitpick comments (7)
src/icons/Ok.tsx (1)
1-10: LGTM! Clean icon implementation.The component follows a clean pattern with inline SVG and
currentColorfill, making it adaptable to different text color contexts. The fixed 16×16 size aligns with the other icons in this set.Optionally, consider adding accessibility attributes (e.g.,
aria-hidden="true"if decorative, orrole="img"witharia-labelif semantic) depending on how this icon is used in the UI.src/components/Button.tsx (1)
36-43: Inline margin change is fine; consider CSS class if spacing growsThe added
style={{ margin: '4px 0' }}cleanly enforces vertical spacing and is safe. If more button-specific spacing rules accumulate, consider moving this to a shared CSS class for easier theming and consistency across variants.src/components/InputFake.tsx (1)
1-20: InputFake is a good fit for read‑only secret display;wrapprop is likely redundantThis component cleanly encapsulates the “fake input” styling for displaying (or masking) the nsec and keeps the actual password input uncontrolled elsewhere, which aligns with your prior secret‑handling approach. Based on
TextSecondary’s implementation, the explicitwrapprop here is probably redundant, sinceTextSecondaryalways enables wrapping internally—you could drop it for a slightly cleaner call site if you like:- <TextSecondary wrap>{text}</TextSecondary> + <TextSecondary>{text}</TextSecondary>Based on learnings, this keeps the displayed secret in a dedicated read‑only component while the actual password remains outside controlled React state.
src/components/Modal.tsx (1)
1-31: Modal overlay positioning and opacity could better match the intended dialog designThe current overlay works but has a few UX quirks:
- Using
position: 'absolute'ties the overlay to the nearest positioned ancestor; on some layouts it may not fully cover the viewport or header.fixedis usually safer for full‑screen modals.- Applying
opacity: 0.95to the outer wrapper also fades the dialog content, not just the backdrop. A semi‑transparent backdrop color typically gives better contrast while keeping the dialog itself fully opaque.- Design feedback mentions a prominent confirmation modal;
maxWidth: 'min(22rem, 90%)'might be on the small side depending on typography.Consider something along these lines:
- const outerStyle = { - top: '0', - left: '0', - zIndex: 21, - opacity: 0.95, - width: '100%', - height: '100%', - display: 'flex', - alignItems: 'center', - justifyContent: 'center', - position: 'absolute' as 'absolute', - backgroundColor: 'var(--ion-background-color)', - } + const outerStyle = { + top: 0, + left: 0, + zIndex: 21, + width: '100%', + height: '100%', + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + position: 'fixed' as const, + backgroundColor: 'rgba(0, 0, 0, 0.4)', // or a dedicated backdrop token + } const innerStyle = { - zIndex: 21, - opacity: 1, + zIndex: 22, padding: '1rem', - maxWidth: 'min(22rem, 90%)', + maxWidth: 'min(26rem, 90%)', borderRadius: '0.5rem', border: '1px solid rgba(251, 251, 251, 0.10)', backgroundColor: 'var(--ion-background-color)', }This should better overlay the existing screen without visually “flattening” the dialog itself and gives you a bit more room to match the design comps.
src/screens/Settings/Backup.tsx (3)
33-36: Share oneerrorstate across backup failures and password errors may confuse messagingThe single
errorstate is now used for:
- Nostr backup failures in
toggleNostrBackup('Backup to Nostr failed'), and- Password validation inside the dialog (
'Invalid password').Because the dialog also renders
<ErrorMessage error={Boolean(error)} text={error} />, a backup error will appear inside the password dialog if the user opens it later, and a password error will persist in the main page’s top‑levelErrorMessageafter closing the dialog.Consider splitting this into two separate pieces of state, e.g.:
- const [error, setError] = useState('') + const [backupError, setBackupError] = useState('') + const [passwordError, setPasswordError] = useState('') // in toggleNostrBackup - setError('Backup to Nostr failed') + setBackupError('Backup to Nostr failed') // in Dialog - <ErrorMessage error={Boolean(error)} text={error} /> + <ErrorMessage error={Boolean(passwordError)} text={passwordError} /> // top-level - <ErrorMessage error={Boolean(error)} text={error} /> + <ErrorMessage error={Boolean(backupError)} text={backupError} />This keeps error messages scoped to their respective flows and avoids leaking unrelated errors into the modal.
Also applies to: 74-88, 90-107
59-68: ClarifyshowPrivateKeyflow and add a simple empty‑password guardFunctionally,
showPrivateKeyworks, but a couple of small tweaks would make it clearer and more robust:
verifyPasswordreturns an nsec string, but the local variable is namedprivateKey, which is misleading.- When
nsecis empty,input.current?.valuecould beundefinedor an empty string; you currently just pass it through and show “Invalid password” on failure.You could address both with:
- const showPrivateKey = async () => { - if (!nsec) { - const privateKey = await verifyPassword(input.current?.value as string) - setError(privateKey ? '' : 'Invalid password') - if (!privateKey) return - setNsec(privateKey) - } + const showPrivateKey = async () => { + if (!nsec) { + const password = (input.current?.value || '') as string + if (!password.trim()) { + setPasswordError('Enter your password') + return + } + + const nsecFromPassword = await verifyPassword(password) + setPasswordError(nsecFromPassword ? '' : 'Invalid password') + if (!nsecFromPassword) return + setNsec(nsecFromPassword) + } setShowNsec(true) setDialog(false) }(Adjust
setPasswordErrorvssetErrorbased on whether you adopt the split‑error suggestion above.)This keeps naming aligned with what’s actually stored and gives users a clearer message when they hit “Confirm” with an empty password.
Also applies to: 90-125
33-39: Confirm that eagerly derivingnsecwithdefaultPasswordmatches your security modelOn mount you derive and store the nsec with:
useEffect(() => { verifyPassword(defaultPassword).then(setNsec) }, [])and later:
- Skip the password prompt in the dialog whenever
nsecis already non‑empty.- Keep
nsecin React state, usingInputFaketo mask or show it based onshowNsec.Given your previous choice to keep sensitive inputs uncontrolled, this is a slightly stronger exposure: the secret is always in component state once the screen mounts (for the default‑password scenario), even before the user requests to view it, and subsequent reveals do not require re‑authentication.
If the product decision is that “default password” sessions don’t need additional verification, this is fine. Otherwise, you might want to:
- Only call
verifyPasswordfromshowPrivateKey, and- Avoid caching
nseclonger than necessary (e.g., clear it when hiding the key or leaving the screen).Just worth double‑checking this against your intended threat model for backups.
Also applies to: 90-99, 141-160
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/Button.tsx(1 hunks)src/components/InputFake.tsx(1 hunks)src/components/Modal.tsx(1 hunks)src/components/Warning.tsx(2 hunks)src/icons/Ok.tsx(1 hunks)src/lib/types.ts(1 hunks)src/screens/Settings/Backup.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/InputFake.tsxsrc/screens/Settings/Backup.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsx
🧬 Code graph analysis (2)
src/components/InputFake.tsx (1)
src/components/Text.tsx (1)
TextSecondary(91-97)
src/components/Warning.tsx (3)
src/icons/Forbid.tsx (1)
ForbidIcon(1-29)src/icons/Info.tsx (1)
InfoIconDark(16-27)src/components/FlexRow.tsx (1)
FlexRow(18-50)
🪛 GitHub Check: test
src/components/Warning.tsx
[warning] 2-2:
'InfoIcon' is defined but never used
[warning] 2-2:
'InfoIcon' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (2)
src/screens/Settings/Backup.tsx (1)
40-51:verifyPasswordis used before initialization—this will throw at runtime
useEffectcallsverifyPassword(defaultPassword)beforeverifyPasswordis defined as aconstarrow function. Becauseconstbindings are not hoisted, this hits the temporal dead zone and will cause a runtime error the first time this component renders.You can fix this either by moving
verifyPasswordabove theuseEffect, or by converting it to a function declaration. For example:- useEffect(() => { - verifyPassword(defaultPassword).then(setNsec) - }, []) - - const verifyPassword = async (password: string): Promise<string> => { + const verifyPassword = async (password: string): Promise<string> => { try { const privateKey = await getPrivateKey(password) return privateKeyToNsec(privateKey) } catch { return '' } } + + useEffect(() => { + verifyPassword(defaultPassword).then(setNsec) + }, [])After this change, the effect can safely invoke
verifyPasswordon mount.⛔ Skipped due to learnings
Learnt from: bordalix Repo: arkade-os/wallet PR: 134 File: src/screens/Settings/Password.tsx:34-39 Timestamp: 2025-08-06T09:53:28.428Z Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.Learnt from: bordalix Repo: arkade-os/wallet PR: 121 File: src/screens/Init/Restore.tsx:58-58 Timestamp: 2025-07-21T18:25:36.391Z Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.src/lib/types.ts (1)
46-64: Based on my comprehensive verification of the codebase:The enum value change for
SettingsOptions.Backupis safe—no persistence or external key usage detected.The search results confirm:
- The old string
'backup and privacy'does not exist anywhere in the codebaseSettingsOptions.Backupis used exclusively for in-memory UI state management: component routing inSettings/Index.tsxand navigation context calls- No localStorage, sessionStorage, or config persistence of these enum values was found
- The enum is not used as a route identifier or serialized configuration key
The enum value serves only as a UI option identifier for conditional component rendering. The concern in the original review comment about breaking lookups or migrations is not applicable here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/screens/Settings/Backup.tsx (1)
97-140: Separate backup vs auth error state to avoid confusing messages in the dialogThe dialog/layout with
Modal,WarningBox, and the copy/view flow looks good and matches the described UX. One minor UX concern: the sameerrorstate is used for both private‑key auth issues (invalid password / biometric failure) and Nostr backup failures, but it’s rendered both inside the dialog (<ErrorMessage>under the password input) and at the top of the screen. That means a “Backup to Nostr failed” message can show up under the password field the next time the dialog opens.Consider splitting this into two states, e.g.
authErrorandbackupError, and wiring:
- Dialog’s
ErrorMessagetoauthError.- Top‑level
ErrorMessagetobackupError(or tobackupError || authErrorif you still want a global banner).That keeps concerns separated and avoids unrelated errors bleeding into the password prompt.
Also applies to: 145-174, 175-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/Settings/Backup.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/screens/Settings/Backup.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (1)
src/screens/Settings/Backup.tsx (1)
1-2: Init + verification logic looks solid; confirm default‑password behaviorNice job keeping
verifyPasswordisolated, returning only nsec and never persisting the raw private key. The hook wiring (including theuseReffor the password input) looks clean.One thing to double‑check: calling
verifyPassword(defaultPassword).then(setNsec)on mount means that wallets still using the default password will only need to click “View private key” → “Confirm”, without entering a password/biometric again. If that shortcut is intentional, this is fine; if you expect a hard auth step for all users, you may want to gate the default‑password resolution behind an explicit check/flag instead of always running it on mount.Also applies to: 19-29, 32-32, 39-42, 44-55
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Warning.tsx(2 hunks)src/test/e2e/nostr.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Warning.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cloudflare Pages: wallet-bitcoin
- GitHub Check: test
🔇 Additional comments (1)
src/test/e2e/nostr.test.ts (1)
34-34: LGTM! Selector updated to match renamed settings option.The change from a broader label match to an exact text match on "backup" correctly reflects the UI rename from "Backup and Privacy" to "Backup" as noted in the PR comments.
Also applies to: 51-51, 67-67
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/screens/Settings/Backup.tsx (1)
63-75: Critical: Invalid passwords still close the dialog and transition to "revealed" state.This is the same issue flagged in previous reviews. When
verifyPasswordreturns an empty string (invalid password), lines 73-74 still execute, closing the dialog and settingshowNsec=true. This shows the "revealed" UI state (with warnings) even though the private key is empty.Additionally, line 66's
.catch(setError)passes anErrorobject tosetError, buterroris typed asstring.Apply this diff to fix both issues:
const showPrivateKey = async () => { if (!nsec) { const password = wallet.lockedByBiometrics - ? await authenticateUser(wallet.passkeyId).catch(setError) + ? await authenticateUser(wallet.passkeyId).catch((err) => { + setError('Authentication failed') + return '' + }) : (input.current?.value as string) + if (!password) return + const privateKey = await verifyPassword(password) - setError(privateKey ? '' : 'Invalid password') - setNsec(privateKey ?? '') + if (!privateKey) { + setError('Invalid password') + return + } + + setError('') + setNsec(privateKey) } + setShowNsec(true) setDialog(false) }This keeps the dialog open with a clear error message on authentication failure, and only transitions to the revealed state when a valid private key is obtained.
🧹 Nitpick comments (2)
src/components/Header.tsx (1)
43-43: Verify header layout after removing<p>elements.Replacing
<p> </p>with a text node ('\u00A0') removes the paragraph element's default margins from the DOM. While this simplifies the structure, the<p>element's box model may have contributed to vertical alignment or spacing of the header elements.Please verify that:
- The header's vertical alignment (back button, title, aux actions) remains consistent
- No visual regressions occur, especially on different screen sizes
Also applies to: 48-48
src/components/Bullet.tsx (1)
10-10: Remove redundant type cast.The
as React.CSSPropertiescast on line 10 is redundant since thestylevariable is already explicitly typed asReact.CSSPropertieson line 2.Apply this diff to remove the redundant cast:
- } as React.CSSProperties + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/components/AlertBox.tsx(1 hunks)src/components/Bullet.tsx(1 hunks)src/components/Checkbox.tsx(1 hunks)src/components/FlexRow.tsx(1 hunks)src/components/Header.tsx(1 hunks)src/components/InputFake.tsx(1 hunks)src/components/Padded.tsx(1 hunks)src/components/Shadow.tsx(2 hunks)src/components/StepBars.tsx(1 hunks)src/components/Strength.tsx(1 hunks)src/components/Title.tsx(1 hunks)src/components/Warning.tsx(2 hunks)src/icons/Logo.tsx(1 hunks)src/icons/Megaphone.tsx(1 hunks)src/screens/Apps/Index.tsx(2 hunks)src/screens/Settings/Backup.tsx(3 hunks)src/screens/Settings/Vtxos.tsx(2 hunks)src/test/e2e/nostr.test.ts(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/Padded.tsx
- src/icons/Logo.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/Shadow.tsx
- src/components/InputFake.tsx
- src/components/Warning.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsx
🧬 Code graph analysis (1)
src/screens/Settings/Backup.tsx (7)
src/providers/wallet.tsx (1)
WalletContext(42-56)src/lib/privateKey.ts (2)
getPrivateKey(40-44)privateKeyToNsec(30-33)src/lib/biometrics.ts (1)
authenticateUser(71-98)src/components/Text.tsx (2)
Text(25-79)TextSecondary(91-97)src/components/Modal.tsx (1)
Modal(1-31)src/components/Shadow.tsx (1)
Shadow(18-55)src/components/InputFake.tsx (1)
InputFake(3-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (13)
src/components/Title.tsx (1)
6-6: LGTM! Good type safety improvement.The explicit
React.CSSPropertiestype annotation improves type safety and aligns with the broader typing refinements across the PR.src/components/AlertBox.tsx (1)
59-59: LGTM!The explicit type annotation improves type safety without affecting runtime behavior.
src/icons/Megaphone.tsx (1)
6-6: LGTM!The explicit type annotation improves type safety without affecting runtime behavior.
src/components/Checkbox.tsx (1)
10-10: LGTM!The explicit type annotation improves type safety without affecting runtime behavior.
src/components/FlexRow.tsx (1)
33-33: LGTM!The explicit type annotation improves type safety without affecting runtime behavior.
src/screens/Settings/Vtxos.tsx (1)
137-137: LGTM!The explicit type annotations in both
BoxandCoinLineimprove type safety without affecting runtime behavior.Also applies to: 180-180
src/components/Bullet.tsx (1)
2-2: LGTM!The explicit type annotation improves type safety.
src/components/StepBars.tsx (1)
6-6: LGTM!The explicit return type annotation improves type safety for the style function without affecting runtime behavior.
src/components/Strength.tsx (1)
36-36: LGTM!The explicit return type annotation improves type safety for the style function without affecting runtime behavior.
src/screens/Apps/Index.tsx (1)
22-22: LGTM! Type safety improvements.Adding explicit
React.CSSPropertiestype annotations improves type safety and maintains consistency with the broader typing refinement pattern across the codebase.Also applies to: 58-58
src/test/e2e/nostr.test.ts (3)
34-34: LGTM! Consistent navigation pattern.Using exact text match
'backup', { exact: true }aligns with the UI changes and provides more reliable test navigation.Also applies to: 51-51, 67-67, 188-188, 193-193
68-70: LGTM! Robust element selection.The modal flow steps ("View private key" → "Confirm") followed by
getByTestId('private-key')is significantly more robust than the previousnth()indexing approach and correctly reflects the new UI flow.
194-196: LGTM! Consistent test pattern.The second test now correctly implements the same modal flow and element selection pattern as the first test, addressing the previous inconsistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/FlexCol.tsx (1)
41-45: Consider avoiding emptydata-testidattributes.When
testIdis not provided,data-testid=""is still rendered on the DOM element. This clutters the HTML and could cause selector issues if code accidentally matches on empty testId.return ( - <div data-testid={testId ?? ''} style={style}> + <div data-testid={testId} style={style}> {children} </div> )Alternatively, conditionally spread the attribute:
return ( <div {...(testId && { 'data-testid': testId })} style={style}> {children} </div> )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/App.tsx(1 hunks)src/components/FlexCol.tsx(3 hunks)src/test/e2e/nostr.test.ts(6 hunks)src/test/e2e/send.test.ts(2 hunks)src/test/e2e/swap.test.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/test/e2e/send.test.tssrc/test/e2e/nostr.test.ts
🧬 Code graph analysis (1)
src/App.tsx (1)
src/components/FlexCol.tsx (1)
FlexCol(16-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (6)
src/test/e2e/send.test.ts (1)
31-31: LGTM! TestId-based selectors improve test stability.Using
getByTestId('tab-wallet')is more resilient to UI text changes and internationalization compared to text-based selectors.Also applies to: 62-62
src/App.tsx (1)
135-151: LGTM! TestId attributes enable robust e2e test selectors.The testId values are consistently named and align with the selectors used across the e2e test suite.
src/test/e2e/swap.test.ts (1)
16-16: LGTM! Consistent migration to testId-based tab navigation.All tab navigation selectors are uniformly updated to use
getByTestId, improving test maintainability across all swap test scenarios.Also applies to: 70-77, 112-112, 137-140, 175-175, 204-207
src/test/e2e/nostr.test.ts (3)
33-34: LGTM! Robust private key extraction using dedicated testId.The migration from fragile
nth()paragraph selectors togetByTestId('private-key')significantly improves test reliability. The consistent modal flow (View private key→Confirm) properly mirrors the new UI design.Also applies to: 38-38, 45-45, 50-51, 55-55, 61-61, 66-70, 74-74, 93-93
109-109: LGTM! Consistent testId-based navigation throughout the test.Tab navigation selectors are uniformly updated to use
getByTestId.Also applies to: 146-146, 152-152, 174-174, 177-177, 182-183
187-196: LGTM! Second test now aligns with the first test's modal flow.The private key retrieval in the second test (
should save swaps to nostr) now correctly uses the same modal confirmation flow andprivate-keytestId as the first test, ensuring consistency.Also applies to: 200-200, 219-219
|
LGTM. We may want to improve the copy but we can do after merge |
* refactor nsec backup * wip * new modal component * wip * authenticate with biometrics * fix tests * fix tests * add testid to tabs * more testid * fix orange
Refactors backup of private key as per new design.
@tiero please review
Summary by CodeRabbit
New Features
New Icons
UI/UX
Style
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.