-
Notifications
You must be signed in to change notification settings - Fork 8
chore: Remove debug console logs and warnings, and adjust formatting for token address and amount display. #19
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
…for token address and amount display.
WalkthroughThis PR removes debug logging statements from the SentInvoice component, streamlines error handling by removing the no-invoices warning, adds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
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 |
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: 0
🧹 Nitpick comments (1)
frontend/src/page/SentInvoice.jsx (1)
342-342: The dependency addition is correct, but consider memoizinguseTokenList's return value.Adding
tokensto the dependency array is necessary since it's used within the effect (viagetTokenInfoat line 275). Thetokensreference should remain stable across renders becauseuseTokenList's effect only depends onchainId, so excessive re-renders are unlikely.However,
useTokenListinfrontend/src/hooks/useTokenList.jsreturns a new object{ tokens, loading, error }on every call. While the destructuredtokensarray itself is stable whenchainIddoesn't change, wrapping the return value inuseMemowould make referential stability explicit:export function useTokenList(chainId) { const [tokens, setTokens] = useState([]); const [loading, setLoading] = useState(false); const [error, setError] = useState(null); useEffect(() => { // ... existing fetch logic }, [chainId]); return useMemo(() => ({ tokens, loading, error }), [tokens, loading, error]); }This ensures that consumers like
SentInvoiceget a stable object reference when their dependencies haven't changed, improving code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/page/SentInvoice.jsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/page/SentInvoice.jsx (2)
frontend/src/page/ReceivedInvoice.jsx (2)
drawerState(96-99)fee(78-78)frontend/src/page/Treasure.jsx (1)
fee(21-21)
🔇 Additional comments (3)
frontend/src/page/SentInvoice.jsx (3)
176-176: Good cleanup: Debug logging removed.The removal of
console.log("Raw invoices data:", res)is appropriate. Debug logging should not be present in production code.
179-179: Good cleanup: Unnecessary warning removed.The removal of
console.warn("No invoices found.")is appropriate. An empty invoice list is a normal state and doesn't warrant a console warning.
889-894: LGTM: Formatting improvements for readability.The multi-line formatting of the token address display (lines 889-894) and total amount calculation (lines 979-984) improves code readability without changing any logic.
Also applies to: 979-984
Zahnentferner
left a comment
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.
These console messages are actually useful. So, I will close this PR without merging.
Summary of changes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.