-
Notifications
You must be signed in to change notification settings - Fork 13
feat: show add money prompt modal wen zero balance #893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
kushagrasarathe
commented
Jun 5, 2025
- contributes to TASK-11712
- fixes TASK-11568
- also fixes TASK-11888
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new modal component, Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/Home/AddMoneyPromptModal/index.tsx (3)
21-34
: Optimize performance by memoizing the CTA array.The
ctas
array is recreated on every render, which is unnecessary since it only depends on stable functions.Consider memoizing the array:
+import { useMemo } from 'react' const AddMoneyPromptModal: React.FC<AddMoneyPromptModalProps> = ({ visible, onClose }) => { const router = useRouter() const handleAddMoney = () => { router.push('/add-money') onClose() } - const ctas: ActionModalButtonProps[] = [ + const ctas: ActionModalButtonProps[] = useMemo(() => [ { text: 'Add money now', onClick: handleAddMoney, shadowSize: '4', className: 'h-10 md:py-2.5 text-sm !font-bold ', }, { text: "I'll do it later", onClick: onClose, variant: 'transparent', className: 'underline text-sm !font-normal w-full !transform-none !pt-2', }, - ] + ], [handleAddMoney, onClose])
26-26
: Remove trailing whitespace in className.There's unnecessary trailing whitespace in the className string.
- className: 'h-10 md:py-2.5 text-sm !font-bold ', + className: 'h-10 md:py-2.5 text-sm !font-bold',
36-45
: Consider memoizing static JSX elements.The
title
anddescription
elements are recreated on every render but contain static content.+import { useMemo } from 'react' +const title = <h1 className="!text-lg !font-bold text-black">Let's add money to your wallet</h1> +const description = ( + <> + Your wallet is ready! + <br /> + To start sending and spending money + <br /> + you'll need to fund it first. + </> +) const AddMoneyPromptModal: React.FC<AddMoneyPromptModalProps> = ({ visible, onClose }) => { // ... rest of component - const title = <h1 className="!text-lg !font-bold text-black">Let's add money to your wallet</h1> - const description = ( - <> - Your wallet is ready! - <br /> - To start sending and spending money - <br /> - you'll need to fund it first. - </> - )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/assets/peanut/peanutman-waving.svg
is excluded by!**/*.svg
📒 Files selected for processing (5)
src/app/(mobile-ui)/home/page.tsx
(5 hunks)src/assets/peanut/index.ts
(1 hunks)src/components/0_Bruddle/Button.tsx
(1 hunks)src/components/AddMoney/views/NetworkSelection.view.tsx
(1 hunks)src/components/Home/AddMoneyPromptModal/index.tsx
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/Home/AddMoneyPromptModal/index.tsx (1)
src/components/Global/ActionModal/index.tsx (1)
ActionModalButtonProps
(8-11)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (11)
src/components/0_Bruddle/Button.tsx (1)
43-43
: LGTM: Clean styling update for primary-soft variant.The background color change from 'bg-primary-3' to 'bg-white' aligns with the modal design requirements and maintains consistency with the updated UI.
src/assets/peanut/index.ts (1)
10-10
: LGTM: Proper asset export addition.The new
PEANUTMAN_WAVING
export follows the established naming convention and module structure.src/components/AddMoney/views/NetworkSelection.view.tsx (1)
38-38
: LGTM: Layout alignment improvement.The addition of
self-start
ensures proper container alignment in flex/grid layouts, supporting consistent positioning across different usage contexts.src/app/(mobile-ui)/home/page.tsx (5)
26-26
: LGTM: Clean import addition.The import statement for
AddMoneyPromptModal
is properly structured and follows the established import patterns.
44-44
: LGTM: Proper state initialization.The
showAddMoneyPromptModal
state is correctly initialized with a boolean default value.
72-72
: LGTM: Dependency array correctly updated.Adding
addAccount
to the dependency array ensures the effect runs when the function reference changes, which is the correct behavior.
172-173
: LGTM: Modal integration properly implemented.The
AddMoneyPromptModal
is correctly integrated with:
- Proper visibility control via state
- Clean close handler
- Appropriate positioning in the component tree
97-113
:✅ Verification successful
Verify modal display conditions and consider edge cases.
The modal logic looks correct but needs verification for potential edge cases:
- The condition
balance === 0n
uses strict equality with BigInt, which is correct- Session storage prevents repeated prompts within the same session
- The modal is suppressed when iOS PWA modal is active
However, consider these scenarios:
- What happens if
balance
is undefined or null during loading?- Should the modal appear immediately on page load or after a delay?
- Is there a risk of the modal showing inappropriately on subsequent visits?
🏁 Script executed:
#!/bin/bash # Verify that balance handling is consistent throughout the codebase rg -A 3 -B 3 "balance.*===.*0" --type tsx --type tsLength of output: 81
I’m not finding any other
balance === 0n
checks with the previous flags—let’s pull in the file around the effect and retry a wider grep:
🏁 Script executed:
#!/bin/bash # Show the first 200 lines of page.tsx for context sed -n '1,200p' 'src/app/(mobile-ui)/home/page.tsx'Length of output: 8463
🏁 Script executed:
#!/bin/bash # Search for any strict zero-BigInt comparisons across TS/TSX files rg "balance.*===.*0n" -n -A3 -B3 --glob '*.ts' --glob '*.tsx'Length of output: 845
I’m not seeing how
balance
is initialized in the wallet hook—let’s pull in its definition:#!/bin/bash # Locate the wallet hook find src -type f -name '*useWallet*.ts'#!/bin/bash # Show the start of the hook implementation sed -n '1,200p' 'src/hooks/wallet/useWallet.ts'
Let’s locate and inspect the
useWallet
hook to see howbalance
is initialized:
🏁 Script executed:
#!/bin/bash # Find the useWallet hook file HOOK_FILE=$(find src -type f -name 'useWallet.ts' | head -n1) echo "Inspecting hook at: $HOOK_FILE" # Show its first 200 lines to check balance initialization and default value sed -n '1,200p' "$HOOK_FILE"Length of output: 3927
Let’s confirm the initial
balance
value in the Redux slice:
🏁 Script executed:
#!/bin/bash # Inspect initial state in the wallet slice sed -n '1,200p' 'src/redux/slices/wallet-slice.ts'Length of output: 914
Modal logic verified—no hidden edge cases found
balance
always comes from Redux as a string (default"0"
) and is cast toBigInt
, so it can’t beundefined
ornull
at runtime.- The
isFetchingBalance
flag blocks the effect until the on-chain balance fetch resolves (or errors), preventing any immediate flash on page load.- You’re already using
sessionStorage
to suppress repeat prompts within a browser session; it persists across refreshes in the same tab and clears on tab close.No changes required here.
src/components/Home/AddMoneyPromptModal/index.tsx (3)
1-6
: LGTM: Clean imports and proper client directive usage.The imports are well-organized and the
'use client'
directive is correctly placed for a component using Next.js client-side hooks.
8-11
: LGTM: Well-defined interface.The interface is simple, clear, and follows React component prop conventions.
47-62
: LGTM: Well-structured modal implementation.The
ActionModal
usage is comprehensive with appropriate props for customization. The modal properly handles visibility, callbacks, and styling overrides.
className: 'h-10 md:py-2.5 text-sm !font-bold ', | ||
}, | ||
{ | ||
text: "I'll do it later", | ||
onClick: onClose, | ||
variant: 'transparent', | ||
className: 'underline text-sm !font-normal w-full !transform-none !pt-2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider reducing !important usage in CSS classes.
Multiple !important
declarations suggest potential CSS specificity conflicts. This could make the styles brittle and harder to maintain.
Consider reviewing the base styles of ActionModal
to avoid needing so many !important
overrides. If these overrides are necessary, consider documenting why or creating a more specific CSS class for this modal variant.
Also applies to: 52-60
🤖 Prompt for AI Agents
In src/components/Home/AddMoneyPromptModal/index.tsx around lines 26 to 32 and
also lines 52 to 60, there are multiple CSS classes using !important which
indicates specificity conflicts. To fix this, review the base styles of the
ActionModal component to reduce the need for !important overrides. Refactor the
CSS by either increasing selector specificity properly or creating a dedicated
CSS class for this modal variant, and remove unnecessary !important
declarations. If some !important usage is unavoidable, add comments explaining
why to improve maintainability.
const handleAddMoney = () => { | ||
router.push('/add-money') | ||
onClose() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify the add-money route exists.
The navigation logic assumes the /add-money
route exists in the application.
🏁 Script executed:
#!/bin/bash
# Description: Verify that the /add-money route exists in the codebase
# Expected: Find page/route definition for add-money
# Search for add-money route definitions
fd -t f "page" src/app | xargs grep -l "add-money" 2>/dev/null || true
# Search for add-money in routing files
rg -A 3 "add-money" --type tsx --type ts src/
Length of output: 255
Missing /add-money
route
It looks like you only have nested pages under add-money
—no root handler for /add-money
. As a result, calling router.push('/add-money')
will 404.
Found routes:
src/app/(mobile-ui)/add-money/crypto/page.tsx
→/add-money/crypto
src/app/(mobile-ui)/add-money/crypto/direct/page.tsx
→/add-money/crypto/direct
Please either:
- Change
router.push('/add-money')
to point to an existing route (e.g./add-money/crypto
), - Or add
src/app/(mobile-ui)/add-money/page.tsx
so/add-money
is served.
🤖 Prompt for AI Agents
In src/components/Home/AddMoneyPromptModal/index.tsx around lines 16 to 19, the
code navigates to '/add-money' which does not exist as a route, causing a 404
error. To fix this, either update router.push to navigate to an existing route
like '/add-money/crypto' or create a new page component at
src/app/(mobile-ui)/add-money/page.tsx to serve the '/add-money' path.