V0.5.0 rc1 #583
V0.5.0 rc1 #583
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/router/src/gateway-ui/components/walletCard/BalancesSummary.tsxOops! Something went wrong! :( ESLint: 8.40.0 Error: Cannot read config file: /apps/router/.eslintrc.js
📝 WalkthroughWalkthroughThe changes in this pull request include the addition of several new React components for wallet management, specifically Changes
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.gitignore(1 hunks)apps/router/src/gateway-ui/components/walletCard/BalancesSummary.tsx(1 hunks)apps/router/src/gateway-ui/components/walletCard/EcashCard.tsx(1 hunks)apps/router/src/gateway-ui/components/walletCard/LightningCard.tsx(1 hunks)apps/router/src/gateway-ui/components/walletCard/OnchainCard.tsx(1 hunks)apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx(1 hunks)apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx(3 hunks)apps/router/src/hooks/gateway/useGateway.tsx(2 hunks)apps/router/src/languages/en.json(1 hunks)apps/router/src/types/gateway.tsx(1 hunks)flake.nix(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Learnings (1)
apps/router/src/gateway-ui/components/walletCard/LightningCard.tsx (1)
Learnt from: Kodylow
PR: fedimint/ui#570
File: apps/router/src/gateway-ui/components/lightning/LightningCard.tsx:90-91
Timestamp: 2024-11-12T07:32:55.886Z
Learning: In the codebase, `gatewayInfo.lightning_mode` is always defined.
🔇 Additional comments (5)
apps/router/src/hooks/gateway/useGateway.tsx (2)
58-65: LGTM: Auth state management looks good
The useEffect correctly manages the auth state based on sessionStorage, with proper dependencies.
124-124: LGTM: Proper cleanup of interval
Added interval cleanup prevents memory leaks when component unmounts.
apps/router/src/languages/en.json (1)
427-427: LGTM!
The new translation key follows the existing conventions and aligns with the UI changes.
apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (1)
10-14: Approved: Refactored WalletCard component utilizes new modular components effectively.
The integration of BalancesSummary, EcashCard, LightningCard, and OnchainCard simplifies the WalletCard component, enhancing readability and maintainability.
apps/router/src/types/gateway.tsx (1)
24-24: Approved: Addition of Lightning to WalletModalType enum aligns with new functionality.
Including Lightning as a valid modal type ensures consistency across the application and supports the new LightningCard component.
| const handleModalOpen = (action: WalletModalAction) => { | ||
| const federations = state.gatewayInfo?.federations; | ||
| if (!federations?.length) return; | ||
|
|
||
| dispatch({ | ||
| type: GATEWAY_APP_ACTION_TYPE.SET_WALLET_MODAL_STATE, | ||
| payload: { | ||
| action, | ||
| type: WalletModalType.Ecash, | ||
| selectedFederation: federations[0], | ||
| showSelector: true, | ||
| isOpen: true, | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <Box p={6} bg='white' borderRadius='lg' borderWidth='1px' mb={4}> | ||
| <Flex justify='space-between' align='center' mb={4}> | ||
| <Text fontSize='xl'>{t('wallet.ecash')}</Text> | ||
| <Flex gap={2}> | ||
| <Button | ||
| leftIcon={<FaArrowDown />} | ||
| colorScheme='blue' | ||
| variant='solid' | ||
| onClick={() => handleModalOpen(WalletModalAction.Receive)} | ||
| > | ||
| {t('wallet.receive')} | ||
| </Button> | ||
| <Button | ||
| leftIcon={<FaArrowUp />} | ||
| variant='outline' | ||
| onClick={() => handleModalOpen(WalletModalAction.Send)} | ||
| > | ||
| {t('wallet.send')} | ||
| </Button> | ||
| </Flex> | ||
| </Flex> | ||
| <Text color='gray.600'> | ||
| Send and receive federated ecash between federation members | ||
| </Text> | ||
| </Box> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated code into a reusable component
The EcashCard component shares significant code with the LightningCard component. To improve maintainability and reduce duplication, consider abstracting the shared logic into a common component.
| const handleModalOpen = (action: WalletModalAction) => { | ||
| const federations = state.gatewayInfo?.federations; | ||
| if (!federations?.length) return; | ||
|
|
||
| dispatch({ | ||
| type: GATEWAY_APP_ACTION_TYPE.SET_WALLET_MODAL_STATE, | ||
| payload: { | ||
| action, | ||
| type: WalletModalType.Lightning, | ||
| selectedFederation: federations[0], | ||
| showSelector: true, | ||
| isOpen: true, | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <Box p={6} bg='white' borderRadius='lg' borderWidth='1px' mb={4}> | ||
| <Flex justify='space-between' align='center' mb={4}> | ||
| <Text fontSize='xl'>{t('wallet.lightning-payments')}</Text> | ||
| <Flex gap={2}> | ||
| <Button | ||
| leftIcon={<FaArrowDown />} | ||
| colorScheme='blue' | ||
| variant='solid' | ||
| onClick={() => handleModalOpen(WalletModalAction.Receive)} | ||
| > | ||
| {t('wallet.receive')} | ||
| </Button> | ||
| <Button | ||
| leftIcon={<FaArrowUp />} | ||
| variant='outline' | ||
| onClick={() => handleModalOpen(WalletModalAction.Send)} | ||
| > | ||
| {t('wallet.send')} | ||
| </Button> | ||
| </Flex> | ||
| </Flex> | ||
| <Text color='gray.600'> | ||
| Send and receive Lightning Network payments through the federation | ||
| </Text> | ||
| </Box> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicated code into a reusable component
The LightningCard component has substantial overlap with the EcashCard component in terms of structure and functionality. Refactoring the common code into a shared component can enhance maintainability and reduce redundancy.
There was a problem hiding this comment.
@alexlwn123, understood. Would you like me to open a GitHub issue to track this refactoring suggestion?
| payload: { | ||
| ...state.walletModalState, | ||
| isOpen: false, | ||
| }, |
There was a problem hiding this comment.
Consider resetting modal state on close
The current implementation only updates isOpen while preserving other state values. This could lead to stale state being shown when the modal is reopened.
payload: {
- ...state.walletModalState,
isOpen: false,
+ action: null,
+ type: null,
+ selectedFederation: null,
},📝 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.
| payload: { | |
| ...state.walletModalState, | |
| isOpen: false, | |
| }, | |
| payload: { | |
| isOpen: false, | |
| action: null, | |
| type: null, | |
| selectedFederation: null, | |
| }, |
| <Button | ||
| leftIcon={<FaArrowDown />} | ||
| colorScheme='blue' | ||
| variant='solid' | ||
| > | ||
| {t('wallet.receive')} | ||
| </Button> | ||
| <Button leftIcon={<FaArrowUp />} variant='outline'> | ||
| {t('wallet.send')} | ||
| </Button> | ||
| </Flex> |
There was a problem hiding this comment.
Add missing onClick handlers for node wallet buttons
The receive and send buttons in the node's onchain wallet section are non-functional.
| const handleModalOpen = (action: WalletModalAction) => { | ||
| const federations = state.gatewayInfo?.federations; | ||
| if (!federations?.length) return; | ||
|
|
There was a problem hiding this comment.
Add error feedback for missing federations
The function silently returns when no federations are available. Consider showing a toast or error message to inform users.
There was a problem hiding this comment.
TODO: ^^^
maybe create a hook that guards the specific operations we're using within the useGatewayContext hook
There was a problem hiding this comment.
@alexlwn123, that's a great idea. Implementing a hook to guard the operations within useGatewayContext can ensure proper handling when federations are missing. Here's an example of how you might create such a hook:
// In useGatewayContext.tsx
export function useGuardedGatewayContext() {
const context = useGatewayContext();
const { state, dispatch } = context;
if (!state.gatewayInfo?.federations?.length) {
// Handle the case when no federations are available
// For example, show an error message or redirect the user
// dispatch an action or set a state to show an error message
}
return { state, dispatch };
}Then, in your component, you can use the guarded context:
const { state, dispatch } = useGuardedGatewayContext();This approach ensures that any component using the context will handle missing federations appropriately.
| function BalancesSummary(): JSX.Element { | ||
| const { t } = useTranslation(); | ||
| const { state } = useGatewayContext(); | ||
| const balanceAmounts = useBalanceCalculations(state.balances ?? undefined); |
There was a problem hiding this comment.
Handle undefined balances more gracefully
The useBalanceCalculations hook receives undefined when state.balances is null, but there's no handling for this case in the component.
Consider adding a loading state or default values:
- const balanceAmounts = useBalanceCalculations(state.balances ?? undefined);
+ const balanceAmounts = useBalanceCalculations(state.balances ?? { ecash: 0, lightning: 0, onchain: 0 });📝 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 balanceAmounts = useBalanceCalculations(state.balances ?? undefined); | |
| const balanceAmounts = useBalanceCalculations(state.balances ?? { ecash: 0, lightning: 0, onchain: 0 }); |
| formattedValue: formatValue( | ||
| balanceAmounts.ecash as MSats, | ||
| state.unit, | ||
| true | ||
| ), | ||
| color: '#FF6384', | ||
| }, | ||
| { | ||
| title: 'Lightning', | ||
| value: balanceAmounts.lightning, | ||
| formattedValue: formatValue( | ||
| balanceAmounts.lightning as MSats, | ||
| state.unit, | ||
| true | ||
| ), | ||
| color: '#36A2EB', | ||
| }, | ||
| { | ||
| title: 'Onchain', | ||
| value: balanceAmounts.onchain, | ||
| formattedValue: formatValue( | ||
| balanceAmounts.onchain as MSats, | ||
| state.unit, | ||
| true | ||
| ), |
There was a problem hiding this comment.
Remove redundant type casting
The MSats type casting is redundant as the values from balanceAmounts should already be of the correct type.
- formattedValue: formatValue(balanceAmounts.ecash as MSats, state.unit, true),
+ formattedValue: formatValue(balanceAmounts.ecash, state.unit, true),Committable suggestion skipped: line range outside the PR's diff.
|
Missing locales after merging applications: |
| </Flex> | ||
| </Flex> | ||
| <Text color='gray.600'> | ||
| Send and receive federated ecash between federation members |
| leftIcon={<FaArrowDown />} | ||
| colorScheme='blue' | ||
| variant='solid' | ||
| > | ||
| {t('wallet.receive')} | ||
| </Button> | ||
| <Button leftIcon={<FaArrowUp />} variant='outline'> | ||
| {t('wallet.send')} | ||
| </Button> |
There was a problem hiding this comment.
TODO: Implement send/receive onchain
| const handleModalOpen = (action: WalletModalAction) => { | ||
| const federations = state.gatewayInfo?.federations; | ||
| if (!federations?.length) return; | ||
|
|
||
| dispatch({ | ||
| type: GATEWAY_APP_ACTION_TYPE.SET_WALLET_MODAL_STATE, | ||
| payload: { | ||
| action, | ||
| type: WalletModalType.Lightning, | ||
| selectedFederation: federations[0], | ||
| showSelector: true, | ||
| isOpen: true, | ||
| }, | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <Box p={6} bg='white' borderRadius='lg' borderWidth='1px' mb={4}> | ||
| <Flex justify='space-between' align='center' mb={4}> | ||
| <Text fontSize='xl'>{t('wallet.lightning-payments')}</Text> | ||
| <Flex gap={2}> | ||
| <Button | ||
| leftIcon={<FaArrowDown />} | ||
| colorScheme='blue' | ||
| variant='solid' | ||
| onClick={() => handleModalOpen(WalletModalAction.Receive)} | ||
| > | ||
| {t('wallet.receive')} | ||
| </Button> | ||
| <Button | ||
| leftIcon={<FaArrowUp />} | ||
| variant='outline' | ||
| onClick={() => handleModalOpen(WalletModalAction.Send)} | ||
| > | ||
| {t('wallet.send')} | ||
| </Button> | ||
| </Flex> | ||
| </Flex> | ||
| <Text color='gray.600'> | ||
| Send and receive Lightning Network payments through the federation | ||
| </Text> | ||
| </Box> | ||
| ); | ||
| }); |
| const handleModalOpen = (action: WalletModalAction) => { | ||
| const federations = state.gatewayInfo?.federations; | ||
| if (!federations?.length) return; | ||
|
|
There was a problem hiding this comment.
TODO: ^^^
maybe create a hook that guards the specific operations we're using within the useGatewayContext hook
Summary by CodeRabbit
New Features
BalancesSummary,EcashCard,LightningCard, andOnchainCardfor enhanced wallet functionality.Bug Fixes
WalletModalcomponent to prevent potential memory leaks.Documentation
.gitignoreto excludetodo.md.Chores
fedimintinput reference inflake.nix.