new nudge (aka red dot) provider #251
Conversation
WalkthroughIntroduces a centralized nudge system: NudgeProvider now exposes nudges/addNudge/addPasswordNudge/removeNudge; UI components and icons render red-dot nudges; NudgeProvider is moved outermost; wallet startup triggers password nudges; new NudgeComponent and AlertBox/AlertText exports; tests and testIDs added. Changes
Sequence Diagram(s)sequenceDiagram
participant WalletProv as Wallet Provider
participant NudgeProv as Nudge Provider
participant App as App / UI
participant User as User
WalletProv->>WalletProv: initialize and compute balance
alt balance ≥ minSatsToNudge
WalletProv->>NudgeProv: addPasswordNudge()
NudgeProv->>NudgeProv: add nudge to repo and rebuild nudges map
NudgeProv-->>App: context value updates (nudges, APIs)
end
App->>App: consume NudgeContext\npass `nudge` props to icons/menu\nrender NudgeComponent where present
User->>App: click dismiss on NudgeComponent
App->>NudgeProv: removeNudge(nudge)
NudgeProv->>NudgeProv: remove from repo and rebuild nudges map
NudgeProv-->>App: context updates -> UI re-renders
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
f34c1bb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e7e1a616.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://nudge-provider.wallet-bitcoin.pages.dev |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/icons/Apps.tsx (1)
1-11: Consider using theme variable for the nudge circle colorThe nudge logic is fine, but here the circle uses a literal
'red'whileRedDotIconand other components usevar(--red). For visual/theming consistency you might want to align this tofill='var(--red)'.src/icons/Wallet.tsx (1)
1-13: Align nudge circle color with theme variablesFunctionally this is fine, but the nudge dot uses a hard‑coded
'red'. For consistency withRedDotIconand theming, consider switching tofill='var(--red)'.src/index.tsx (1)
30-57: Provider reordering with NudgeProvider outermost looks safeWrapping the existing provider stack in
NudgeProviderkeeps their relative order and matches howNudgeContextis consumed inAppandMenu; there’s no apparent dependency that would be broken by this move. Note that commenting outReact.StrictModedoes change dev‑time behaviour (no more double effects), so just make sure that’s intentional.src/providers/nudge.tsx (2)
26-26: Consider more robust equality comparison.The
flat()function usesJSON.stringifyfor nudge equality, which is sensitive to property order. If object properties are serialized in different orders, identical nudges may not be recognized as equal.Consider using a stable serialization approach or a dedicated equality function:
const flat = (nudge: Nudge) => { // Sort arrays to ensure consistent serialization return JSON.stringify({ options: [...nudge.options].sort(), pages: [...nudge.pages].sort(), tabs: [...nudge.tabs].sort(), text: nudge.text, }) }Or use a deep equality library for more reliable comparison.
38-59: Consider consolidating state updates for consistency.Both
addNudgeandremoveNudgecallupdateNudgesbeforesetRepo, which could theoretically lead to inconsistency if state updates don't complete synchronously. While React typically batches updates, using a single state update or functional updates would be more robust.Example using a single state object:
const [state, setState] = useState<{ repo: Nudge[]; nudges: Record<string, Nudge[]> }>({ repo: [], nudges: {}, }) const addNudge = (nudge: Nudge) => { setState(prev => { if (prev.repo.find((n) => flat(n) === flat(nudge))) return prev const newRepo = [...prev.repo, nudge] return { repo: newRepo, nudges: buildNudgesMap(newRepo) } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/App.tsx(3 hunks)src/components/AlertBox.tsx(5 hunks)src/components/Menu.tsx(3 hunks)src/components/Nudge.tsx(1 hunks)src/icons/Apps.tsx(2 hunks)src/icons/RedDot.tsx(1 hunks)src/icons/Settings.tsx(2 hunks)src/icons/Wallet.tsx(2 hunks)src/index.tsx(1 hunks)src/lib/constants.ts(1 hunks)src/providers/nudge.tsx(1 hunks)src/providers/wallet.tsx(3 hunks)src/screens/Settings/Password.tsx(4 hunks)src/screens/Wallet/Index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/Wallet/Index.tsxsrc/providers/wallet.tsx
📚 Learning: 2025-08-17T11:03:40.893Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 142
File: src/screens/Wallet/Unlock.tsx:18-27
Timestamp: 2025-08-17T11:03:40.893Z
Learning: In the Arkade wallet, the default password ("noah") is intentionally used as part of a progressive security model. New users can onboard without password friction using the default password, and once they accumulate 10,000+ sats, the app nudges them to set a custom password. This is a deliberate UX design decision to reduce onboarding barriers while encouraging security practices when users have meaningful value at stake.
Applied to files:
src/providers/wallet.tsxsrc/lib/constants.ts
📚 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/Password.tsx
🧬 Code graph analysis (7)
src/components/Menu.tsx (2)
src/providers/nudge.tsx (1)
NudgeContext(12-20)src/icons/RedDot.tsx (1)
RedDotIcon(1-10)
src/screens/Wallet/Index.tsx (2)
src/lib/constants.ts (1)
psaMessage(10-10)src/components/AlertBox.tsx (1)
InfoBox(55-68)
src/providers/wallet.tsx (2)
src/providers/nudge.tsx (1)
NudgeContext(12-20)src/lib/constants.ts (1)
minSatsToNudge(8-8)
src/App.tsx (5)
src/providers/nudge.tsx (1)
NudgeContext(12-20)src/icons/Wallet.tsx (1)
WalletIcon(1-13)src/components/FlexCol.tsx (1)
FlexCol(15-40)src/icons/Apps.tsx (1)
AppsIcon(1-13)src/icons/Settings.tsx (1)
SettingsIcon(1-13)
src/screens/Settings/Password.tsx (2)
src/providers/nudge.tsx (1)
NudgeContext(12-20)src/components/Nudge.tsx (1)
NudgeComponent(7-19)
src/components/Nudge.tsx (4)
src/providers/nudge.tsx (2)
Nudge(5-10)NudgeContext(12-20)src/components/AlertBox.tsx (2)
AlertBox(14-45)AlertText(47-53)src/icons/RedDot.tsx (1)
RedDotIconAnimated(12-21)src/components/FlexCol.tsx (1)
FlexCol(15-40)
src/index.tsx (2)
src/providers/nudge.tsx (1)
NudgeProvider(22-62)src/App.tsx (1)
App(36-160)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages: wallet-bitcoin
🔇 Additional comments (11)
src/components/AlertBox.tsx (2)
14-53: AlertBox/AlertText public exports look consistent and safeThe new exports, click/dismiss wiring, and styling logic are all straightforward and type‑safe; DOMPurify usage and InfoBox integration remain intact. No issues from my side here.
55-60: InfoBox and CloseIcon tweaks are low‑risk UI refinementsReducing InfoBox font size and adding a pointer cursor on the close SVG are purely visual changes and match the intent of a dismissible alert; no behavioural or safety concerns.
Also applies to: 70-82
src/screens/Wallet/Index.tsx (1)
55-55: PSA InfoBox rendering is straightforward and safeConditional rendering based solely on
psaMessageand delegating HTML sanitization toInfoBoxlooks correct; no edge‑case or safety issues here.src/App.tsx (1)
32-42: NudgeContext consumption and icon wiring are correctUsing
nudges[Tabs.X]?.length > 0with optional chaining is safe against missing keys, and passing the resulting boolean into the tab icons cleanly reflects nudge state without risking runtime errors. The integration withNudgeProviderinindex.tsxlooks coherent.Also applies to: 136-151
src/components/Menu.tsx (1)
4-9: Menu nudge indicator integration is soundConsuming
nudgesfromNudgeContextand rendering<RedDotIcon />whennudges[option]?.length > 0is type‑safe and aligns with the nudge model (options keyed bySettingsOptions). No correctness issues here.Also applies to: 16-19, 22-43
src/icons/RedDot.tsx (1)
1-21: RedDot icon components are simple and robustBoth static and animated red dot SVGs are self‑contained and side‑effect free; dependency on CSS variables is safe and failure‑tolerant. Looks good.
src/lib/constants.ts (2)
8-8: LGTM! Threshold aligns with progressive security model.The reduction to 10,000 sats correctly implements the nudge threshold described in the project learnings, where users are encouraged to set a password once they have meaningful value at stake.
Based on learnings, the 10,000 sats threshold is part of the intentional progressive security UX design.
10-10: LGTM! Default PSA message added.The default message provides helpful guidance to users about support resources.
src/components/Nudge.tsx (1)
7-19: LGTM! Clean nudge component implementation.The component correctly integrates with NudgeContext for dismissal, uses appropriate UI components (AlertBox, AlertText, FlexCol), and displays the animated red dot icon.
src/screens/Settings/Password.tsx (1)
86-96: LGTM! Proper nudge integration in password settings.The nudge is correctly retrieved from context using the SettingsOptions.Password key and conditionally rendered before the error message. The optional chaining safely handles missing nudges.
src/icons/Settings.tsx (1)
1-10: LGTM! Red dot indicator added to settings icon.The optional nudge prop correctly controls the visibility of the red circle badge, providing a visual indicator when nudges are present.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/providers/wallet.tsx (1)
90-99: Add error handling to prevent unhandled promise rejections.The promise chain for
calcBatchLifetimeMsandcalcNextRolloverlacks error handling. If either promise rejects, it results in an unhandled rejection. The balance check effect on line 109 uses.catch(() => {})for this purpose.Apply this diff to add error handling:
useEffect(() => { if (!initialized || !vtxos || !svcWallet) return const allVtxos = [...vtxos.spendable, ...vtxos.spent] calcBatchLifetimeMs(aspInfo, allVtxos).then((batchLifetimeMs) => { const thresholdMs = Math.floor((batchLifetimeMs * maxPercentage) / 100) calcNextRollover(vtxos.spendable, svcWallet, aspInfo).then((nextRollover) => { updateWallet((prev) => ({ ...prev, nextRollover, thresholdMs })) }) - }) + }).catch((err) => consoleError(err, 'Error computing rollover thresholds')) }, [initialized, vtxos, svcWallet, aspInfo])Alternatively, consider using
async/awaitwith try/catch for better readability:useEffect(() => { if (!initialized || !vtxos || !svcWallet) return - const allVtxos = [...vtxos.spendable, ...vtxos.spent] - calcBatchLifetimeMs(aspInfo, allVtxos).then((batchLifetimeMs) => { - const thresholdMs = Math.floor((batchLifetimeMs * maxPercentage) / 100) - calcNextRollover(vtxos.spendable, svcWallet, aspInfo).then((nextRollover) => { - updateWallet((prev) => ({ ...prev, nextRollover, thresholdMs })) + const computeThresholds = async () => { + try { + const allVtxos = [...vtxos.spendable, ...vtxos.spent] + const batchLifetimeMs = await calcBatchLifetimeMs(aspInfo, allVtxos) + const thresholdMs = Math.floor((batchLifetimeMs * maxPercentage) / 100) + const nextRollover = await calcNextRollover(vtxos.spendable, svcWallet, aspInfo) + updateWallet((prev) => ({ ...prev, nextRollover, thresholdMs })) + } catch (err) { + consoleError(err, 'Error computing rollover thresholds') + } + } + computeThresholds() - }) - }) }, [initialized, vtxos, svcWallet, aspInfo])
🧹 Nitpick comments (1)
src/providers/wallet.tsx (1)
101-110: Consider addingaddPasswordNudgeto the dependency array.The
addPasswordNudgefunction is called inside the effect but is not listed in the dependency array. This violates the React hooks exhaustive-deps rule and could cause stale closure issues if the function reference changes.If
addPasswordNudgeis stable (wrapped inuseCallbackin the provider), you can suppress the lint warning with a comment. Otherwise, add it to the dependencies:useEffect(() => { if (!initialized || !vtxos || !svcWallet) return svcWallet .getBalance() .then((balance) => { if (balance.total >= minSatsToNudge) addPasswordNudge() }) .catch(() => {}) - }, [initialized, vtxos, svcWallet, aspInfo]) + }, [initialized, vtxos, svcWallet, aspInfo, addPasswordNudge])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/wallet.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/providers/wallet.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/providers/wallet.tsx
🧬 Code graph analysis (1)
src/providers/wallet.tsx (3)
src/providers/nudge.tsx (1)
NudgeContext(12-20)src/lib/wallet.ts (2)
calcBatchLifetimeMs(37-48)calcNextRollover(21-35)src/lib/constants.ts (2)
maxPercentage(9-9)minSatsToNudge(8-8)
⏰ 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/providers/wallet.tsx (1)
18-19: LGTM!The new imports for
maxPercentage,minSatsToNudge, andNudgeContextare correctly added to support the nudge functionality.
|
an we enable it for things like Push Notifications and Nostr backup? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/e2e/nudge.test.ts (2)
18-23: Selectors relying on.nth()and generic tags are brittleThe following selectors are tightly coupled to DOM structure and ordering:
- Lines 18–23:
page.locator('div').filter({ hasText: /^Copy address$/ }).nth(5).click()- Line 23:
page.getByRole('img').nth(4).click()for triggering copy- Line 37:
page.getByTestId('transactions-list').locator('div').first().click()These will break easily if:
- Additional "Copy address" labels are added or order changes
- Icons/images are reordered
- Extra wrapper
<div>s are introduced inside the transactions listPrefer more semantic and targeted locators, for example:
getByRole('button', { name: 'Copy address' })or a dedicateddata-testidfor the copy action- A more specific role/name for the control that copies the address instead of
img.nth(4)- Within
transactions-list, target the clickable transaction row more explicitly (e.g., adata-testidon the row, or a role likerow/listitem), rather than the first genericdivThis should make the test more resilient to future UI changes.
Happy to propose concrete locator patterns once the underlying markup/roles or test IDs are known.
Also applies to: 23-23, 37-37
25-27: Small robustness and clarity improvements in faucet + clipboard assertionsA few optional cleanups that could make failures clearer and the test a bit leaner:
- Lines 25–27:
expect(boardingAddress).toBeDefined()followed bytoBeTruthy()are redundant; a singletoBeTruthy()(or stronger, a regex check for address format) is enough.- Lines 29–33:
exec('nigiri faucet ...')is fire-and-forget. If the CLI is missing or fails, the test will eventually time out on UI waits but without surfacing the underlying error. Wrappingexecin a smallpromisifyhelper andawait‑ing it (or at least logging errors in its callback) would make failures easier to diagnose.- Line 31:
await page.waitForSelector('text=Payment received!')is followed byexpect(...).toBeVisible(), which already has built-in waiting; you can rely on theexpectalone.These aren’t blockers but would improve the test’s signal and maintainability.
If you decide to tighten the faucet handling, please confirm the exact
nigiriCLI behavior and exit codes for your environment, as those can differ across versions.Also applies to: 29-33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/TransactionsList.tsx(1 hunks)src/test/e2e/nudge.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/e2e/nudge.test.ts (1)
src/test/e2e/utils.ts (1)
readClipboard(3-16)
⏰ 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/components/TransactionsList.tsx (1)
114-121: Stable test hook for transactions list looks goodAdding
data-testid='transactions-list'on the container is a clean, non-invasive way to drive E2E tests and matches its usage innudge.test.ts. No behavioral impact on the component.
@tiero yes, the Nudge provider is the outer most provider, which means every other provider will be able to register nudges into it, for whatever reason they want (push notifications, Nostr backup, soon to expiry, etc). The Nudge to be registered has the following interface: Which marks which Tabs and SettingsOptions (in setting's menu) should have the red dot. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/e2e/nudge.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/nudge.test.ts
🧬 Code graph analysis (1)
src/test/e2e/nudge.test.ts (1)
src/test/e2e/utils.ts (1)
readClipboard(3-16)
⏰ 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). (1)
- GitHub Check: test
| // faucet | ||
| exec(`nigiri faucet ${boardingAddress} 0.0021`) | ||
| await page.waitForSelector('text=Payment received!') | ||
| await expect(page.getByText('Payment received!')).toBeVisible() | ||
| await expect(page.getByText('SATS received successfully')).toBeVisible() |
There was a problem hiding this comment.
Add error handling for the faucet command.
The exec call on line 30 runs asynchronously without error handling or completion verification. If the nigiri faucet command fails (e.g., nigiri not installed, network issue, or invalid address), the test will hang indefinitely waiting for the "Payment received!" selector.
Wrap exec in a promise with error handling:
- // faucet
- exec(`nigiri faucet ${boardingAddress} 0.0021`)
- await page.waitForSelector('text=Payment received!')
+ // faucet
+ await new Promise<void>((resolve, reject) => {
+ exec(`nigiri faucet ${boardingAddress} 0.0021`, (error, stdout, stderr) => {
+ if (error) {
+ reject(new Error(`Faucet command failed: ${error.message}`))
+ return
+ }
+ if (stderr) {
+ console.warn('Faucet stderr:', stderr)
+ }
+ resolve()
+ })
+ })
+ await page.waitForSelector('text=Payment received!')📝 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.
| // faucet | |
| exec(`nigiri faucet ${boardingAddress} 0.0021`) | |
| await page.waitForSelector('text=Payment received!') | |
| await expect(page.getByText('Payment received!')).toBeVisible() | |
| await expect(page.getByText('SATS received successfully')).toBeVisible() | |
| // faucet | |
| await new Promise<void>((resolve, reject) => { | |
| exec(`nigiri faucet ${boardingAddress} 0.0021`, (error, stdout, stderr) => { | |
| if (error) { | |
| reject(new Error(`Faucet command failed: ${error.message}`)) | |
| return | |
| } | |
| if (stderr) { | |
| console.warn('Faucet stderr:', stderr) | |
| } | |
| resolve() | |
| }) | |
| }) | |
| await page.waitForSelector('text=Payment received!') | |
| await expect(page.getByText('Payment received!')).toBeVisible() | |
| await expect(page.getByText('SATS received successfully')).toBeVisible() |
🤖 Prompt for AI Agents
In src/test/e2e/nudge.test.ts around lines 29 to 33, the exec call for `nigiri
faucet` is fired without awaiting completion or handling errors which can cause
the test to hang; change it to a promise-based exec (or promisify
child_process.exec), await the command, capture stdout/stderr and check the exit
code, and if the command errors or exits non-zero reject/throw to fail the test;
also add a reasonable timeout and optional retry/logging so failures surface
immediately instead of waiting for the selector.
|
Ok tested the branch deploy and don't see the red dots yet |
@tiero for now the only action adding a nudge is having a total balance > 100_000 sats and not having a defined password. Are you testing it with those conditions? |
Screen.Recording.2025-12-01.at.14.47.09.mov |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/screens/wallet/send.test.tsx (1)
83-83: Prefer role-based query for the “Continue” button (optional)The updated expectation is fine and matches the new UI copy. For slightly more robust and accessibility-aligned testing, consider asserting by role instead of raw text:
expect(screen.getByRole('button', { name: /continue/i })).toBeInTheDocument()This will keep the intent (a button labeled “Continue”) even if surrounding layout or nearby text changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/providers/wallet.tsx(3 hunks)src/test/screens/wallet/send.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/providers/wallet.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 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/screens/wallet/send.test.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
|
Lets add Notifications already ( will be useful when we have #204) and Nostr backup feature |
|
@tiero let's see what we need to show on those new notifications and react then? |
|
Driving
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/providers/nudge.tsx (3)
29-38: Consider treatingnudgesas derived state fromrepoinstead of maintaining both manually.Right now
updateNudgesis called manually fromaddNudge/removeNudge. That couples each mutator to the derivation logic and makes future mutations easy to get wrong.A more robust pattern is:
- Keep only
repoin state.- Use
useMemooruseEffectto derivenudgesfromrepo.Conceptually:
-import { ReactNode, createContext, useState } from 'react' +import { ReactNode, createContext, useMemo, useState } from 'react' @@ - const [repo, setRepo] = useState<Nudge[]>([]) - const [nudges, setNudges] = useState<Record<string, Nudge[]>>({}) + const [repo, setRepo] = useState<Nudge[]>([]) @@ - const updateNudges = (repo: Nudge[]) => { - const map: Record<string, Nudge[]> = {} - repo.forEach((nudge) => { - nudge.options.forEach((option) => (map[option] = [...(map[option] || []), nudge])) - nudge.pages.forEach((page) => (map[page] = [...(map[page] || []), nudge])) - nudge.tabs.forEach((tab) => (map[tab] = [...(map[tab] || []), nudge])) - }) - setNudges(map) - } + const nudges = useMemo(() => { + const map: Record<string, Nudge[]> = {} + repo.forEach((nudge) => { + nudge.options.forEach((option) => (map[option] = [...(map[option] || []), nudge])) + nudge.pages.forEach((page) => (map[page] = [...(map[page] || []), nudge])) + nudge.tabs.forEach((tab) => (map[tab] = [...(map[tab] || []), nudge])) + }) + return map + }, [repo])This removes the risk of forgetting to call
updateNudgeswheneverrepochanges and simplifies the mutator functions.
29-30: Re-evaluate JSON-stringify–based identity for nudges.Using
JSON.stringifyinflatfor equality checks works but has trade-offs:
- It’s relatively expensive if the number of nudges grows.
- It couples identity to property order and full structural equality, which can be brittle if callers construct equivalent
Nudgeobjects with different key insertion orders.- For
removeNudge, you already pass around the same object instance from context in most flows, so reference equality would likely suffice.If you don’t need structural equality, consider:
- Using reference equality where possible (
n === nudge).- Or assigning a stable
idfield at creation time and comparing on that.Example minimal change if identity-by-reference is enough:
- const flat = (nudge: Nudge) => JSON.stringify(nudge) @@ - if (repo.find((n) => flat(n) === flat(nudge))) return // already exists + if (repo.includes(nudge)) return // already exists @@ - if (!repo.find((n) => flat(n) === flat(nudge))) return // not found - const filtered = repo.filter((n) => flat(n) !== flat(nudge)) + if (!repo.includes(nudge)) return // not found + const filtered = repo.filter((n) => n !== nudge)If you expect structurally identical but distinct instances, adding an explicit
idis safer than relying onJSON.stringify.Also applies to: 41-62
6-10: Context shape and provider value are clear; consider documentingtextssemantics.The
Nudgetype andNudgeContextAPI are straightforward and flexible. One small suggestion: sincetextsis an array (vs. a singletext), it may help future contributors if you document whether these are intended as multiple bullet points, paragraphs, or ordered messages so UI components render them consistently.Also applies to: 13-22, 64-68
src/components/Menu.tsx (1)
4-9: Menu nudge integration looks correct and idiomatic.Using
NudgeContextto gateRedDotIcononnudges[option]?.length > 0is type-safe given theRecord<string, Nudge[]>map, and the nestedFlexRowkeeps the existing layout while adding the red dot before the arrow. No functional issues spotted.If you want a tiny readability tweak, you could replace the ternary with
nudges[option]?.length && <RedDotIcon />, but that’s purely stylistic.Also applies to: 17-18, 37-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Menu.tsx(3 hunks)src/components/Nudge.tsx(1 hunks)src/icons/RedDot.tsx(1 hunks)src/providers/nudge.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Nudge.tsx
- src/icons/RedDot.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/providers/nudge.tsx (1)
src/lib/constants.ts (1)
minSatsToNudge(8-8)
src/components/Menu.tsx (4)
src/providers/nudge.tsx (1)
NudgeContext(13-23)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/icons/RedDot.tsx (1)
RedDotIcon(1-10)src/icons/Arrow.tsx (1)
ArrowIcon(1-7)
⏰ 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/providers/nudge.tsx (1)
47-55: Password nudge definition looks coherent and localized.The
addPasswordNudgehelper cleanly encapsulates the advanced/password options, settings page/tab, and the multi-linetextscontent, which should make it easy to reuse and evolve as you add more nudge types (e.g., push, Nostr backup). No issues from a correctness standpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/App.tsx (1)
138-138: Optional: factor out per‑tab nudge checks for readabilityThe three
nudges[Tabs.*]?.length > 0checks are correct but a bit repetitive. You could optionally precompute them to make the JSX slightly cleaner.For example, near the top of the component (after getting
nudges):const walletHasNudge = nudges[Tabs.Wallet]?.length > 0 const appsHasNudge = nudges[Tabs.Apps]?.length > 0 const settingsHasNudge = nudges[Tabs.Settings]?.length > 0Then update the icon props:
- <WalletIcon nudge={nudges[Tabs.Wallet]?.length > 0} /> + <WalletIcon nudge={walletHasNudge} /> ... - <AppsIcon nudge={nudges[Tabs.Apps]?.length > 0} /> + <AppsIcon nudge={appsHasNudge} /> ... - <SettingsIcon nudge={nudges[Tabs.Settings]?.length > 0} /> + <SettingsIcon nudge={settingsHasNudge} />Purely cosmetic; no behavior change.
Also applies to: 144-144, 150-150
src/components/AlertBox.tsx (1)
70-83:CloseIconpointer styling improves click affordanceAdding an explicit
cursor: 'pointer'on the SVG is a nice UX improvement and is correctly guarded byonClickbeing required in the props. If you want a bit more type safety, you could annotatestyleasReact.CSSProperties, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsx(3 hunks)src/components/AlertBox.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/App.tsx (5)
src/providers/nudge.tsx (1)
NudgeContext(13-23)src/icons/Wallet.tsx (1)
WalletIcon(1-13)src/components/FlexCol.tsx (1)
FlexCol(16-46)src/icons/Apps.tsx (1)
AppsIcon(1-13)src/icons/Settings.tsx (1)
SettingsIcon(1-13)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (4)
src/App.tsx (1)
32-41: NudgeContext wiring in App looks solidImporting and consuming
NudgeContextat the App root to readnudgesis correct and keeps the tab icons in sync with nudge state. The defaultnudges: {}value also makes this safe if the provider is ever missing in tests or storybook.src/components/AlertBox.tsx (3)
3-5: Imports forFlexRowandMegaphoneIconlook appropriateUsing
FlexRowfor layout and a dedicatedMegaphoneIconfor alerts/nudges fits the component’s responsibilities and keeps icon semantics clear. No issues here.
14-53: ExportingAlertBoxandAlertTextas shared primitives makes sensePromoting these to named exports is a good move for reusability (e.g., by the new nudge components). The props remain simple and flexible (
iconasReact.ReactNode, optionalonClick/onDismiss), and the layout viaFlexRowis straightforward. No functional concerns from this API exposure.
55-67:InfoBoxstyling/icon tweak is safeSwitching to
MegaphoneIconand slightly reducing the font size to 12px preserves structure and still wraps content inAlertBoxwith sanitized HTML. Behavior and security posture withDOMPurify.sanitizeremain intact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/icons/RedDot.tsx (2)
1-18: Consider removing the negative margin positioning hack.The negative
marginLeftis a fragile positioning approach that may cause layout issues in different contexts. Additionally, the circle is slightly off-center in the viewBox (positioned at 10,10 in a 21×21 viewBox rather than 10.5,10.5).Apply this diff to center the circle and remove the negative margin:
export default function RedDotIcon() { - const style = { - marginLeft: '-0.5rem', - } return ( <svg fill='none' width='1rem' height='1rem' - style={style} viewBox='0 0 21 21' data-testid='red-dot-icon' xmlns='http://www.w3.org/2000/svg' > - <circle cx='10' cy='10' r='3' fill='var(--red)' /> + <circle cx='10.5' cy='10.5' r='3' fill='var(--red)' /> </svg> ) }If the negative margin is intentional for specific alignment needs, consider documenting the reason or using a more explicit positioning approach via the parent component's CSS.
20-29: Add test ID for consistency and consider CSS variable fallbacks.The animated variant lacks a
data-testidattribute, creating inconsistency withRedDotIcon. Additionally, both components rely on CSS variables without fallback values, which could result in invisible dots if the variables are undefined.Apply this diff to add the test ID and fallback values:
export function RedDotIconAnimated() { const style = { animation: 'var(--animation-pulse)', } return ( - <svg width='14' height='14' viewBox='0 0 35 35' fill='none' style={style} xmlns='http://www.w3.org/2000/svg'> - <circle cx='17.5' cy='17.5' r='17.5' fill='var(--red)' /> + <svg width='14' height='14' viewBox='0 0 35 35' fill='none' style={style} data-testid='red-dot-icon-animated' xmlns='http://www.w3.org/2000/svg'> + <circle cx='17.5' cy='17.5' r='17.5' fill='var(--red, #ff0000)' /> </svg> ) }Also consider adding the same fallback to
RedDotIcon(line 15):- <circle cx='10.5' cy='10.5' r='3' fill='var(--red)' /> + <circle cx='10.5' cy='10.5' r='3' fill='var(--red, #ff0000)' />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ExpandAddresses.tsx(2 hunks)src/icons/RedDot.tsx(1 hunks)src/icons/Settings.tsx(2 hunks)src/test/e2e/nudge.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/icons/Settings.tsx
- src/test/e2e/nudge.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T15:58:17.245Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 142
File: src/lib/bip21.ts:7-13
Timestamp: 2025-08-28T15:58:17.245Z
Learning: In src/lib/bip21.ts, the encodeBip21 function is used by the wallet which always has an arkAddress available to pass to the function, while decodeBip21 is used to parse received possible BIP21 URIs which may not contain arkAddress. This explains why arkAddress is required in encodeBip21 but optional in the Bip21Decoded interface.
Applied to files:
src/components/ExpandAddresses.tsx
🧬 Code graph analysis (1)
src/components/ExpandAddresses.tsx (1)
src/components/FlexRow.tsx (1)
FlexRow(18-50)
⏰ 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/components/ExpandAddresses.tsx (1)
46-72: LGTM! Clean test identifier implementation.The addition of test identifiers to the address lines is well-implemented:
- The
testIdprop is correctly typed as optional- The pattern
address-line-${testId}provides consistent, descriptive test selectors- Each address type (bip21, btc, ark, lightning) receives a meaningful identifier
This will facilitate targeted testing of individual address lines.
|
@coderabbitai you can now review the test |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1766 @bordalix, I've reviewed the e2e nudge test. Overall, it provides good coverage of the nudge feature flow. Here are my observations and suggestions: ✅ Strengths
🔍 Issues & Suggestions1. Race condition with faucet command (Critical)// Line 23: exec is fire-and-forget
exec(`nigiri faucet ${boardingAddress} 0.0021`)
await page.waitForSelector('text=Payment received!')The await new Promise<void>((resolve, reject) => {
exec(`nigiri faucet ${boardingAddress} 0.0021`, (error) => {
if (error) reject(error)
else resolve()
})
})2. Hard-coded timeout (Best Practice)// Line 22: Avoid hard-coded delays
await page.waitForTimeout(500)Replace with a more robust wait strategy or remove if the subsequent clipboard read is sufficient. 3. Redundant settings tab click// Line 26
await page.getByTestId('tab-settings').click()
// Line 29: duplicate click
await page.getByTestId('tab-settings').click()The second click appears unnecessary. If it's intentional (e.g., to re-trigger navigation), please add a comment explaining why. 4. Missing error handlingConsider adding error handling for the faucet command and potentially a timeout: try {
await new Promise<void>((resolve, reject) => {
exec(`nigiri faucet ${boardingAddress} 0.0021`, (error) => {
if (error) reject(error)
else resolve()
})
})
} catch (error) {
console.error('Faucet command failed:', error)
throw error
}5. Address validation could be more specificexpect(boardingAddress).toBeDefined()
expect(boardingAddress).toBeTruthy()Consider validating the address format (e.g., starts with expected prefix for the network). 6. Test amount verificationThe test name says "balance > 100_000 sats" and you're fauceting 0.0021 BTC = 210,000 sats. This is correct, but consider adding a comment or constant to make the relationship clear: const minSatsToNudge = 100_000
const faucetAmount = 0.0021 // 210,000 sats (> minSatsToNudge)Minor NoteThe test doesn't include cleanup (e.g., deleting the wallet), which might be fine for e2e tests that run in isolated environments, but consider if this is intentional. Would you like me to suggest a refactored version of the test addressing these issues? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/providers/nudge.tsx (1)
31-39: Consider consolidating state updates to avoid secondary renders.Calling
setNudgesinsideupdateNudges, which is itself called from withinsetRepocallbacks, triggers two state updates and two renders for each add/remove operation. For small nudge counts this is acceptable, but it could be optimized by derivingnudgesfromrepousinguseMemoinstead of maintaining separate state.Optional refactor using
useMemo:const [repo, setRepo] = useState<Nudge[]>([]) const nudges = useMemo(() => { const map: Record<string, Nudge[]> = {} repo.forEach((nudge) => { nudge.options.forEach((option) => (map[option] = [...(map[option] || []), nudge])) nudge.pages.forEach((page) => (map[page] = [...(map[page] || []), nudge])) nudge.tabs.forEach((tab) => (map[tab] = [...(map[tab] || []), nudge])) }) return map }, [repo])Then remove
setNudgesandupdateNudgescalls fromaddNudgeandremoveNudge.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/nudge.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/providers/nudge.tsx (1)
src/lib/constants.ts (1)
minSatsToNudge(8-8)
🪛 GitHub Check: test
src/providers/nudge.tsx
[warning] 26-26:
'repo' is assigned a value but never used
[warning] 26-26:
'repo' is assigned a value 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 (6)
src/providers/nudge.tsx (6)
13-23: LGTM!The context definition provides a clean API surface with appropriate default values for nudge management.
26-27: Static analysis false positive.The warning that
repois unused is a false positive—it's correctly used within the functional state update callbacks inaddNudgeandremoveNudge.
41-48: LGTM! Past review addressed.The functional state update pattern correctly prevents the stale-state bug identified in the previous review. All state updates now use the latest
prevvalue.
50-58: LGTM!The
addPasswordNudgehelper provides a convenient API for creating password-related nudges with appropriate targeting (settings tab, settings page, password and advanced options).
60-67: LGTM! Past review addressed.The functional state update pattern correctly prevents the stale-state bug identified in the previous review.
69-74: LGTM!The provider correctly exposes the nudge management API to consumers throughout the application.
| export type Nudge = { | ||
| options: SettingsOptions[] | ||
| pages: Pages[] | ||
| tabs: Tabs[] | ||
| texts: string[] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of nudge.texts or nudge.text in the codebase
rg -n 'nudge\.texts?(?:\[|\.| )' --type tsx --type ts -A 2 -B 2Repository: arkade-os/wallet
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for usage of nudge.texts or nudge.text in the codebase
rg -n 'nudge\.texts?' -g '*.tsx' -g '*.ts' -A 2 -B 2Repository: arkade-os/wallet
Length of output: 415
The Nudge type field texts: string[] is correctly implemented but conflicts with PR objectives.
The codebase actually uses nudge.texts as an array (see src/components/Nudge.tsx:14 where it's mapped over). However, the PR objectives document states the field should be text: string. Clarify which design is intended—if multiple text lines are needed (current implementation), update the PR objectives; if only a single text is required, refactor the type to text: string and update the consuming component accordingly.
🤖 Prompt for AI Agents
In src/providers/nudge.tsx around lines 6–11, the Nudge type currently defines
texts: string[] which conflicts with the PR objectives that require a single
text string; change the type to text: string and then update all consuming code
(notably src/components/Nudge.tsx where nudge.texts is currently mapped) to use
nudge.text (remove the map/array handling), update any places that construct
Nudge objects to provide a single text property, and adjust related tests and
documentation accordingly so the model and usage are consistent.
| const [repo, setRepo] = useState<Nudge[]>([]) | ||
| const [nudges, setNudges] = useState<Record<string, Nudge[]>>({}) | ||
|
|
||
| const flat = (nudge: Nudge) => JSON.stringify(nudge) |
There was a problem hiding this comment.
Replace JSON.stringify deduplication with a more robust approach.
Using JSON.stringify for object equality is fragile because property order in JavaScript objects is not guaranteed to be consistent. Two Nudge objects with identical content but different construction order will produce different strings, allowing duplicates.
Consider a more reliable approach:
- const flat = (nudge: Nudge) => JSON.stringify(nudge)
+ const flat = (nudge: Nudge) => {
+ return [
+ nudge.options.slice().sort().join(','),
+ nudge.pages.slice().sort().join(','),
+ nudge.tabs.slice().sort().join(','),
+ nudge.texts.slice().sort().join(','),
+ ].join('|')
+ }Alternatively, implement a proper deep-equality check or use a library like lodash.isequal.
📝 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.
| const flat = (nudge: Nudge) => JSON.stringify(nudge) | |
| const flat = (nudge: Nudge) => { | |
| return [ | |
| nudge.options.slice().sort().join(','), | |
| nudge.pages.slice().sort().join(','), | |
| nudge.tabs.slice().sort().join(','), | |
| nudge.texts.slice().sort().join(','), | |
| ].join('|') | |
| } |
🤖 Prompt for AI Agents
In src/providers/nudge.tsx around line 29, the flat function uses
JSON.stringify(nudge) for deduplication which is brittle due to property order
variance; replace it with a robust approach such as implementing a
stable/canonical serializer that recursively sorts object keys before
stringifying (or simply use a deep-equality check via lodash.isequal to detect
duplicates), and update dedup logic to use that canonical key or isEqual
comparisons; if you choose lodash.isequal, add the dependency/import and use it
where duplicates are filtered, otherwise implement a small helper that sorts
keys recursively then JSON.stringify to produce consistent keys.
|
replaced by #296 |
Summary by CodeRabbit
New Features
UI
Tests
✏️ Tip: You can customize this high-level summary in your review settings.