Context Improvements + Hooks dir #570
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough## Walkthrough
The pull request introduces significant updates to the state management and action handling in the `GatewayContext` and related components. New properties and action types are added to the global state, allowing for improved control over modal visibility and active tabs. Several components transition from local state management to using the global context, streamlining their interfaces and enhancing maintainability. Import paths for types have been updated to reflect a restructured organization, ensuring consistency across the application.
## Changes
| File Path | Change Summary |
|----------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `apps/router/src/context/gateway/GatewayContext.tsx` | - Added new state properties: `showConnectFed`, `walletModalState`, `activeTab`.<br>- Enhanced reducer to handle new action types for state updates. |
| `apps/router/src/gateway-ui/Gateway.tsx` | - Removed local state management for modal visibility and tab selection.<br>- Introduced functions to dispatch actions for state updates. |
| `apps/router/src/gateway-ui/components/federations/FederationsTable.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/gateway-ui/components/walletModal/WalletActionSelector.tsx` | - Updated import path for `WalletModalAction` and `WalletModalType`. |
| `apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx` | - Removed local enum declarations and replaced with imports from a centralized types module. |
| `apps/router/src/gateway-ui/components/walletModal/WalletTypeButtons.tsx` | - Updated import path for `WalletModalType`. |
| `apps/router/src/gateway-ui/components/walletModal/index.tsx` | - Removed `ReceiveProps` interface and associated imports. |
| `apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx` | - Updated import path for `WalletModalState`. |
| `apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/gateway-ui/components/walletModal/send/SendLightning.tsx` | - Removed props interface and simplified component structure. |
| `apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx` | - Removed props interface and transitioned to context-based state management. |
| `apps/router/src/types/gateway.tsx` | - Added new enums (`WalletModalAction`, `WalletModalType`) and interface (`WalletModalState`).<br>- Updated `GatewayAppState` and `GATEWAY_APP_ACTION_TYPE`. |
## Possibly related PRs
- **#522**: The changes in this PR involve the introduction of a centralized state management system, which aligns with the enhancements made in the main PR regarding state management in `GatewayContext.tsx`.
- **#551**: This PR modifies the `connectFederation` method in the GatewayApi class, which is related to the state management changes in the main PR that enhance action handling within the `GatewayContext`.
- **#558**: The introduction of a new authentication context that simplifies password management could relate to the state management enhancements in the main PR, particularly in how the wallet modal state is handled.
- **#560**: The switch to a tabbed interface in the `Gateway` component directly relates to the changes in state management and action handling in the main PR, as it involves managing the active tab state which is part of the new properties added to the context.
- **#563**: This PR addresses issues with single authentication functionality, which may connect to the state management improvements in the main PR, particularly in how authentication states are handled within the context.
- **#565**: The enhancement of configuration handling to read environment variables in addition to the static config.json could relate to the overall state management improvements in the main PR, as it impacts how the application initializes and manages its state based on configuration.🪧 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: 6
🧹 Outside diff range and nitpick comments (13)
apps/router/src/gateway-ui/components/walletModal/send/SendLightning.tsx (1)
Line range hint
11-11: Implement or track lightning payment functionalityThe TODO comment indicates missing core functionality. This should be tracked in the project management system.
Would you like me to create a GitHub issue to track the implementation of the lightning payment functionality?
apps/router/src/gateway-ui/components/walletModal/WalletTypeButtons.tsx (1)
Line range hint
7-11: Consider using GatewayContext directlySince this PR's objective is to move state into context, this component could be simplified by consuming the GatewayContext directly instead of receiving state and handlers via props.
-interface WalletTypeButtonsProps { - type: WalletModalType; - onTypeChange: (type: WalletModalType) => void; -} - -export const WalletTypeButtons: React.FC<WalletTypeButtonsProps> = ({ - type, - onTypeChange, -}) => { +export const WalletTypeButtons: React.FC = () => { + const { walletModalState, setWalletModalState } = useGatewayContext(); + const type = walletModalState.type; + const onTypeChange = (type: WalletModalType) => + setWalletModalState(prev => ({ ...prev, type }));apps/router/src/gateway-ui/components/walletModal/WalletActionSelector.tsx (1)
Line range hint
41-47: Consider optimizing duplicate WalletTypeButtons renderingThe WalletTypeButtons component is rendered identically in both tab panels. Consider lifting it outside the TabPanels to avoid unnecessary re-renders and improve component maintainability.
<Tabs index={action === WalletModalAction.Receive ? 0 : 1} onChange={(index) => onActionChange( index === 0 ? WalletModalAction.Receive : WalletModalAction.Send ) } isFitted variant='enclosed-colored' > <TabList color='gray.500'> <Tab fontWeight='bold' fontSize='lg'> {t('wallet.receive')} </Tab> <Tab fontWeight='bold' fontSize='lg'> {t('wallet.send')} </Tab> </TabList> - <TabPanels> - <TabPanel> - <WalletTypeButtons type={type} onTypeChange={onTypeChange} /> - </TabPanel> - <TabPanel> - <WalletTypeButtons type={type} onTypeChange={onTypeChange} /> - </TabPanel> - </TabPanels> + <WalletTypeButtons type={type} onTypeChange={onTypeChange} /> </Tabs>apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (2)
Line range hint
11-16: Remove unused props from interfaceThe component interface declares
federationsandwalletModalStateprops that are never used in the implementation. ThesetWalletModalStateprop is also defined but never used.interface ReceiveEcashProps { - federations: FederationInfo[]; - walletModalState: WalletModalState; - setWalletModalState: (state: WalletModalState) => void; setShowSelector: (show: boolean) => void; }
Line range hint
26-33: Improve error handling and add input validationTwo critical issues:
- Errors are only logged to console without user feedback
- Raw clipboard text is passed to API without validation, posing a security risk
const handlePaste = async () => { try { const text = await navigator.clipboard.readText(); + if (!text.trim()) { + throw new Error('Empty clipboard'); + } + // Add validation for expected ecash note format setEcashNote(text); const result = await api.receiveEcash(text); setClaimedAmount(result); setShowSelector(false); } catch (err) { console.error('Failed to receive ecash: ', err); + // Add error state and user feedback + setEcashNote(''); } };apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx (2)
Line range hint
27-75: Prevent concurrent peg-out submissionsThe function should be protected against multiple simultaneous submissions which could lead to race conditions.
+ const [isSubmitting, setIsSubmitting] = useState(false); const handlePegOut = async () => { + if (isSubmitting) return; + setIsSubmitting(true); try { // ... existing code ... } catch (error) { // ... existing code ... } finally { + setIsSubmitting(false); } };
Line range hint
89-100: Add loading state to improve UXThe button should indicate when a peg-out is in progress to prevent user confusion.
- <Button onClick={handlePegOut} colorScheme='blue'> + <Button + onClick={handlePegOut} + colorScheme='blue' + isLoading={isSubmitting} + loadingText={t('wallet-modal.send.processing')} + >apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx (1)
Line range hint
27-95: Consider using GatewayContext directly.The component still uses prop drilling for state management. Since the PR's objective is to move state into a context, consider refactoring to use the GatewayContext directly instead of passing state through props.
apps/router/src/gateway-ui/components/walletModal/index.tsx (1)
Line range hint
1-1: Address component organization TODOThe TODO comment indicates these components should be split into separate files. This would improve maintainability and align with the ongoing context refactoring.
Would you like me to help create a plan for splitting these components into separate files?
apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (1)
Line range hint
134-146: Add safety check for federations arrayThe deposit and withdraw buttons automatically select
federations[0]without checking if the array is empty. This could cause runtime errors if no federations are available.Apply this fix:
onClick={() => { + if (federations.length === 0) return; setWalletModalState({ action: WalletModalAction.Receive, type: WalletModalType.Onchain, selectedFederation: federations[0], isOpen: true, }); }}Same fix should be applied to the send button's onClick handler.
Also applies to: 147-159
apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1)
Line range hint
35-43: Fix potential runtime error when mapping over undefined 'federations'If
state.gatewayInfo?.federationsis undefined, calling.mapon it will cause a runtime error. To prevent this, provide a default empty array.Apply this diff to fix the issue:
-{state.gatewayInfo?.federations.map((federation) => ( +{(state.gatewayInfo?.federations || []).map((federation) => (apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx (1)
Line range hint
38-44: Handle undefined 'federationName' in translation functionIf
state.walletModalState.selectedFederationis undefined,federationNamewill be undefined, which may cause issues in the translation function. Consider providing a default value or handling this case.Apply this diff to handle the potential undefined value:
- .federation_name, + .federation_name ?? 'Unknown Federation',apps/router/src/context/gateway/GatewayContext.tsx (1)
Line range hint
77-79: EnsuregatewayIdis Valid to Prevent Potential ErrorsRelying on
location.pathname.split('/')[2]assumes the URL path always contains at least three segments. If the URL structure varies,gatewayIdcould beundefined, leading to errors inuseGatewayConfigand when initializingGatewayApi. Consider adding validation to handle cases wheregatewayIdmay beundefinedor invalid.Proposed code:
const location = useLocation(); - const gatewayId = location.pathname.split('/')[2]; + const pathSegments = location.pathname.split('/'); + const gatewayId = pathSegments[2]; + if (!gatewayId) { + // Handle the error, e.g., redirect or show an error message + } const config = useGatewayConfig(gatewayId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- apps/router/src/context/gateway/GatewayContext.tsx (3 hunks)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/components/federations/FederationsTable.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletActionSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletTypeButtons.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/index.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendLightning.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx (3 hunks)
- apps/router/src/types/gateway.tsx (4 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- apps/router/src/gateway-ui/Gateway.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/router/src/gateway-ui/components/federations/FederationsTable.tsx
🧰 Additional context used
🔇 Additional comments (2)
apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx (1)
20-24: Verify type definitions match previous local declarations.Let's ensure the imported types maintain the same structure as the removed local declarations.
✅ Verification successful
Type definitions have been correctly moved to a central location
The types
WalletModalAction,WalletModalState, andWalletModalTypehave been properly moved from local definitions toapps/router/src/types/gateway.tsxwith their structure preserved:
WalletModalAction: enum withReceiveandSendWalletModalType: enum withEcash,Onchain(and commentedLightning)WalletModalState: interface withisOpen,action,type, andselectedFederationAll components are correctly importing and using these types consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare the structure of the imported types with their previous definitions # Expected: The types should maintain the same structure to avoid breaking changes # Check the imported type definitions ast-grep --pattern 'export (type|enum|interface) WalletModal(Action|Type|State)'Length of output: 82
Script:
#!/bin/bash # Let's try a different approach to find the type definitions # First, let's check the gateway types file fd "gateway.ts" --type f # Then let's use ripgrep to find the type definitions with more context rg "WalletModal(Action|State|Type)" -A 5Length of output: 25351
apps/router/src/gateway-ui/components/walletModal/index.tsx (1)
19-19: Verify context integration is completeThe new Sats import suggests ongoing refactoring to context-based state management, but these components still use prop-based state.
apps/router/src/gateway-ui/components/walletModal/send/SendLightning.tsx
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx
Outdated
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/router/src/context/gateway/GatewayContext.tsx (3 hunks)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/components/federations/FederationsTable.tsx (4 hunks)
- apps/router/src/gateway-ui/components/lightning/LightningCard.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (3 hunks)
- apps/router/src/types/gateway.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx
- apps/router/src/types/gateway.tsx
🧰 Additional context used
🔇 Additional comments (1)
apps/router/src/gateway-ui/components/federations/FederationsTable.tsx (1)
Line range hint
15-107: LGTM!The refactoring simplifies the component by leveraging context-based state management. The dispatch actions and state accesses are correctly implemented.
Kodylow
changed the title
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (3)
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (1)
Line range hint
21-23: Remove hard-coded test values in production code.The contract is using hard-coded test values which could lead to issues in production.
- const incomingContract: IncomingContract = { - commitment: 'test', - ciphertext: 'test', - }; + const incomingContract: IncomingContract = { + commitment: crypto.randomUUID(), + ciphertext: crypto.randomUUID(), + };apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx (1)
Line range hint
59-65: Prevent address creation without federation selectionThe create button should be disabled when no federation is selected to prevent unnecessary API calls.
Apply this diff:
<FederationSelector /> <AmountInput amount={amount} setAmount={setAmount} unit='sats' /> <CreateButton onClick={handleCreateDepositAddress} + disabled={!state.walletModalState.selectedFederation} label={t('wallet-modal.receive.create-peg-in-address')} />apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1)
Line range hint
36-44: Prevent errors when mapping over potentially undefined 'state.gatewayInfo.federations'If
state.gatewayInfo.federationsis undefined, attempting to map over it will result in a runtime error. Provide a default empty array to ensure safe mapping.Apply this diff to handle the undefined case:
- {state.gatewayInfo?.federations.map((federation) => ( + {(state.gatewayInfo?.federations ?? []).map((federation) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- apps/router/src/context/gateway/GatewayContext.tsx (3 hunks)
- apps/router/src/context/hooks.tsx (0 hunks)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/components/federations/FederationsTable.tsx (4 hunks)
- apps/router/src/gateway-ui/components/lightning/LightningCard.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx (2 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx (3 hunks)
- apps/router/src/hooks/gateway/useGateway.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardian.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardianSetup.tsx (1 hunks)
- apps/router/src/hooks/index.tsx (1 hunks)
💤 Files with no reviewable changes (1)
- apps/router/src/context/hooks.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/router/src/context/gateway/GatewayContext.tsx
- apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx
🧰 Additional context used
📓 Learnings (4)
apps/router/src/gateway-ui/components/lightning/LightningCard.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/lightning/LightningCard.tsx:90-91 Timestamp: 2024-10-22T20:27:07.540Z Learning: In the codebase, `gatewayInfo.lightning_mode` is always defined.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/lightning/LightningCard.tsx:34-35 Timestamp: 2024-10-22T20:26:55.760Z Learning: Potential null reference exceptions with `gatewayInfo` are handled within the `useGatewayContext` hook, so in components like `LightningCard.tsx`, explicit null checks for `gatewayInfo` are unnecessary.apps/router/src/hooks/guardian/useGuardian.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.apps/router/src/hooks/guardian/useGuardianSetup.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.apps/router/src/hooks/index.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.
🪛 Biome
apps/router/src/hooks/guardian/useGuardianSetup.tsx
[error] 71-71: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (4)
apps/router/src/hooks/index.tsx (2)
6-11: LGTM: Clean context hook implementation.The
useAppContexthook follows React's best practices for context consumption.
3-4: Verify the existence of exported modules.The new exports align with the PR's context improvements objective.
✅ Verification successful
Exports are valid and modules exist
Both exported modules
useGuardian.tsxanduseGateway.tsxexist at their respective paths, confirming the exports are valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the exported hook modules exist fd --type f "useGuardian.tsx?" "apps/router/src/hooks/guardian" fd --type f "useGateway.tsx?" "apps/router/src/hooks/gateway"Length of output: 217
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (1)
25-26: Previous federation selection issue remains unfixed.The previous review comment about handling undefined federation is still valid. Using an empty string as fallback for federation_id could cause runtime errors.
apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx (1)
25-31: Verify Error Handling in the Catch BlockEnsure that the error object in the
.catchblock hasmessageanderrorproperties. If not, destructuring may fail, and the error handling might not work as expected. Consider adjusting the error handling to account for different error structures.
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/lightning/LightningCard.tsx
Outdated
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (27)
apps/router/src/context/guardian/AdminContext.tsx (2)
Line range hint
7-8: Context provides no value to consumersThe context is created and provided with
null, making it unusable for consumers. Consider implementing a proper context value interface and providing meaningful state/methods.-// export interface AdminContextValue {} -export const AdminContext = createContext(null); +export interface AdminContextValue { + isAdmin: boolean; + // Add other admin-related state/methods +} +export const AdminContext = createContext<AdminContextValue | null>(null);
Line range hint
12-23: Consider implementing as a route guard instead of contextThe current implementation uses context pattern just to block non-admin access, which could be better implemented as a route guard or HOC. This would separate access control from state management concerns.
apps/router/src/guardian-ui/components/setup/screens/setupComplete/SetupComplete.tsx (1)
Line range hint
20-27: Consider making the transition delay configurable and showing progressThe hardcoded 3-second delay might feel arbitrary to users. Consider:
- Making this duration configurable via constants/env
- Adding a progress indicator to show how long until transition
+ const ADMIN_TRANSITION_DELAY_MS = 3000; useEffect(() => { const timer = setTimeout(() => { dispatch({ type: GUARDIAN_APP_ACTION_TYPE.SET_STATUS, payload: GuardianStatus.Admin, }); - }, 3000); + }, ADMIN_TRANSITION_DELAY_MS);apps/router/src/guardian-ui/components/dashboard/tabs/balanceSheet/BalanceSheet.tsx (1)
Line range hint
17-21: Add error state handling for failed API callsThe error is currently only logged to console, leaving users unaware of API failures. Consider displaying an error state in the UI.
useEffect(() => { const fetchBalance = () => { - api.audit().then(setAuditSummary).catch(console.error); + api.audit() + .then(setAuditSummary) + .catch(error => { + console.error(error); + setAuditSummary(undefined); + }); }; fetchBalance();apps/router/src/home/EditServiceModal.tsx (2)
Line range hint
42-47: Add URL normalization before comparisonThe URL comparison might fail for equivalent URLs with different formats (e.g., 'http://example.com' vs 'http://example.com/' vs 'https://example.com'). Consider normalizing URLs before comparison.
+ const normalizeUrl = (url: string) => { + const normalized = new URL(url); + return normalized.origin + normalized.pathname.replace(/\/$/, ''); + }; const serviceExists = Object.values({ ...guardians, ...gateways }).some( - (s) => s.config.baseUrl === configUrl + (s) => normalizeUrl(s.config.baseUrl) === normalizeUrl(configUrl) );
Line range hint
49-89: Implement atomic updates to prevent data lossThe current implementation removes the old service before adding the new one. If adding the new service fails, the old service is lost. Consider reversing the order or implementing a transaction-like pattern.
- // Remove old service - dispatch({ - type: service.type === 'guardian' - ? APP_ACTION_TYPE.REMOVE_GUARDIAN - : APP_ACTION_TYPE.REMOVE_GATEWAY, - payload: service.id, - }); // Add new service const newPayload = { id: newId, [service.type]: { config: { baseUrl: configUrl } }, }; dispatch({ type: service.type === 'guardian' ? APP_ACTION_TYPE.ADD_GUARDIAN : APP_ACTION_TYPE.ADD_GATEWAY, payload: newPayload, } as AppAction); + // Remove old service only after successful addition + dispatch({ + type: service.type === 'guardian' + ? APP_ACTION_TYPE.REMOVE_GUARDIAN + : APP_ACTION_TYPE.REMOVE_GATEWAY, + payload: service.id, + });apps/router/src/guardian-ui/admin/FederationAdmin.tsx (3)
Line range hint
31-41: Consider using Promise.all for parallel API callsThe
fetchDatafunction makes multiple sequential API calls. This could be optimized by running them in parallel usingPromise.all.const fetchData = useCallback(() => { - api.inviteCode().then(setInviteCode).catch(console.error); - api.config().then(setConfig).catch(console.error); - api.apiAnnouncements().then(setSignedApiAnnouncements).catch(console.error); - api - .status() - .then((statusData) => { - setStatus(statusData); - setLatestSession(statusData?.federation?.session_count); - }) - .catch(console.error); + Promise.all([ + api.inviteCode(), + api.config(), + api.apiAnnouncements(), + api.status() + ]).then(([inviteCode, config, announcements, statusData]) => { + setInviteCode(inviteCode); + setConfig(config); + setSignedApiAnnouncements(announcements); + setStatus(statusData); + setLatestSession(statusData?.federation?.session_count); + }).catch(console.error); }, [api]);
Line range hint
43-47: Make polling interval configurableThe 5000ms polling interval is hardcoded. Consider making it configurable through props or environment variables.
+const POLL_INTERVAL = process.env.REACT_APP_FEDERATION_POLL_INTERVAL || 5000; + useEffect(() => { fetchData(); - const interval = setInterval(fetchData, 5000); + const interval = setInterval(fetchData, POLL_INTERVAL); return () => clearInterval(interval); }, [fetchData]);
Line range hint
31-41: Improve error handling beyond console.logCurrent error handling only logs to console. Consider implementing proper error states and user feedback.
+const [error, setError] = useState<Error | null>(null); + const fetchData = useCallback(() => { + setError(null); Promise.all([ api.inviteCode(), api.config(), api.apiAnnouncements(), api.status() ]).then(([inviteCode, config, announcements, statusData]) => { setInviteCode(inviteCode); setConfig(config); setSignedApiAnnouncements(announcements); setStatus(statusData); setLatestSession(statusData?.federation?.session_count); - }).catch(console.error); + }).catch((err) => { + console.error(err); + setError(err); + }); }, [api]);apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx (3)
Line range hint
156-156: Fix invalid onClose handler in ConnectFedModalThe onClose prop expects a function but receives a boolean value
!loading. This will cause a type error and prevent proper modal behavior.-<ConnectFedModal isOpen={loading} onClose={() => !loading} /> +<ConnectFedModal isOpen={loading} onClose={() => setLoading(false)} />
Line range hint
94-102: Improve error handling robustnessThe error handling assumes a specific error structure with
messageanderrorproperties. This could throw if the error has a different shape.-.catch(({ message, error }) => { +.catch((error: unknown) => { console.error(error); - setErrorMsg(t('connect-federation.error-message', { error: message })); + const errorMessage = error instanceof Error ? error.message : String(error); + setErrorMsg(t('connect-federation.error-message', { error: errorMessage })); })
Line range hint
89-103: Prevent potential race conditions in loading stateThe loading state might not be cleared if the component unmounts during the API call, potentially leading to memory leaks and stale state.
const handleConnectFederation = () => { + let mounted = true; setLoading(true); api .connectFederation(connectInfo.trim()) .then((federation) => { + if (!mounted) return; renderConnectedFedCallback(federation); setConnectInfo(''); }) .catch((error) => { + if (!mounted) return; console.error(error); setErrorMsg(t('connect-federation.error-message', { error: message })); }) .finally(() => { + if (!mounted) return; setLoading(false); }); + return () => { + mounted = false; + }; };apps/router/src/home/ConnectServiceModal.tsx (2)
Line range hint
48-71: Add URL validation and handle race conditionsThe current implementation has two potential issues:
- Race condition: The time gap between checking service existence and adding it could allow duplicate services if multiple requests occur simultaneously
- Missing URL validation: The configUrl should be validated before attempting operations
Consider this implementation:
const handleConfirm = useCallback(async () => { setError(null); setIsLoading(true); try { + if (!configUrl.match(/^wss?:\/\/.+:\d+$/)) { + throw new Error('Invalid URL format'); + } + + // Atomic operation using a transaction or lock mechanism + const id = await sha256Hash(configUrl); const exists = await checkServiceExists(configUrl); if (exists) { throw new Error('Service already exists'); } - const id = await sha256Hash(configUrl);
Line range hint
31-34: Consider moving error state to contextThe component mixes local state for errors with context for services. For consistency with the PR's objective of moving state to context, consider moving the error state to the context as well.
apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1)
Line range hint
1-150: Consider transitioning to context-based state managementThe PR objectives mention moving state into context, but this component still uses local state management (useState) for
sitesandcustomMeta. Consider transitioning these to use the global context for consistency with the PR's goals.apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx (1)
25-25: Consider restructuring imports to reduce path depth.The deep import path (
../../../../../hooks) suggests a potential structural issue. Consider moving shared hooks closer to their consumers or implementing a barrel file pattern to simplify imports.apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (3)
Line range hint
40-80: Consider moving local state to context per PR objectives.The component maintains numerous local state variables that could be moved to the context to align with the PR's goal of centralizing state management. This would reduce complexity and prevent potential state synchronization issues.
Line range hint
150-152: Add type safety for network parameter.The network value is cast to Network type without validation, which could lead to runtime errors if invalid values are provided.
- network: network as Network, + network: isValidNetwork(network) ? (network as Network) : defaultNetwork,
Line range hint
90-130: Potential race condition in initialization effects.The separate effects for configGenParams initialization and password synchronization could lead to race conditions. Consider combining these effects or adding proper dependencies.
apps/router/src/guardian-ui/components/dashboard/tabs/FederationTabsCard.tsx (2)
Line range hint
80-97: Improve polling resilience with exponential backoffThe current polling implementation uses a fixed interval which could overwhelm the server under poor network conditions or high load. Consider implementing an exponential backoff strategy for failed requests.
const pollMetaSubmissions = useCallback(async () => { if (!metaModuleId) return; + let retryCount = 0; + const maxRetries = 3; try { const submissions = await api.moduleApiCall<MetaSubmissions>( Number(metaModuleId), ModuleRpc.getSubmissions, DEFAULT_META_KEY ); + retryCount = 0; // Reset on success // ... rest of the implementation } catch (err) { + retryCount++; + const backoffTime = Math.min(1000 * Math.pow(2, retryCount), 10000); + await new Promise(resolve => setTimeout(resolve, backoffTime)); console.warn('Failed to poll for meta submissions', err); + if (retryCount >= maxRetries) { + throw new Error('Max retries exceeded for meta submissions polling'); + } } }, [api, metaModuleId, ourPeer.id]);
Line range hint
92-94: Enhance error handling with user feedbackThe current implementation only logs warnings to console on polling failures. Users should be notified of connectivity issues that could affect their operations.
} catch (err) { + const errorMessage = err instanceof Error ? err.message : 'Unknown error'; + toast({ + title: 'Federation sync error', + description: `Failed to sync federation state: ${errorMessage}`, + status: 'error', + duration: 5000, + }); console.warn('Failed to poll for meta submissions', err); }apps/router/src/guardian-ui/components/dashboard/tabs/meta/proposals/ProposedMetas.tsx (2)
Line range hint
91-106: Add user feedback for failed operations.The
handleClearfunction only logs errors to console, which means users won't know when the operation fails. Consider adding toast notifications or error states to inform users of failures.const handleClear = useCallback(async () => { try { await api.moduleApiCall<{ metaValue: string }[]>( Number(metaModuleId), ModuleRpc.submitMeta, { key: DEFAULT_META_KEY, value: metaToHex(fieldsToMeta([])), // Empty submission } ); + toast({ + title: "Successfully cleared meta edits", + status: "success" + }); } catch (err) { console.error('Failed to clear meta edits', err); + toast({ + title: "Failed to clear meta edits", + status: "error" + }); } }, [api, metaModuleId]);
Potential Race Condition in Approval Logic
The approval process lacks synchronization mechanisms, allowing multiple approvals to occur simultaneously and potentially exceed the threshold.
- File:
apps/router/src/guardian-ui/components/dashboard/tabs/meta/proposals/ProposedMetas.tsx🔗 Analysis chain
Line range hint
186-196: Handle potential race condition in approval threshold check.The approval check
currentApprovals + 1 >= thresholdcould lead to unexpected behavior if multiple users approve simultaneously, as the count might change between check and actual approval. Consider implementing optimistic locking or version checking.
Let me check the implementation details around this approval logic to better understand the potential race condition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other instances of similar threshold checks that might have race conditions rg -A 2 "currentApprovals.*threshold"Length of output: 393
Script:
#!/bin/bash # Check the implementation of approval logic and threshold calculation ast-grep --pattern 'const threshold = $_' # Look for any state management or approval tracking logic rg -A 5 "setCurrentApprovals|currentApprovals.*=" # Check for any existing synchronization mechanisms rg -A 3 "handleApprove|onApprove"Length of output: 4863
apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (4)
43-43: Consider using path aliases to avoid deep relative imports.Deep relative imports with multiple "../" make the codebase fragile to refactoring. Consider configuring path aliases in your TypeScript/webpack config.
Line range hint
60-67: Consider consolidating verification state into a custom hook.Multiple related state variables (myHash, peersWithHash, enteredHashes, verifiedConfigs) could be consolidated into a custom hook like
useVerificationStateto improve maintainability and align with the PR's objective of proper hooks structure.
Line range hint
71-123: Add cleanup function to prevent memory leaks and race conditions.The effect performs async operations without cleanup. If the component unmounts while
assembleHashInfois running, it could lead to memory leaks or state updates on unmounted component.useEffect(() => { + let mounted = true; if ( peers.every( (peer) => peer.status === GuardianServerStatus.VerifiedConfigs ) ) { setVerifiedConfigs(true); } async function assembleHashInfo() { if (peers.length === 0) { - return setError(t('verify-guardians.error')); + if (mounted) setError(t('verify-guardians.error')); + return; } if (ourCurrentId === null) { - return setError(t('verify-guardians.error-peer-id')); + if (mounted) setError(t('verify-guardians.error-peer-id')); + return; } - if (peers[ourCurrentId]) { + if (mounted && peers[ourCurrentId]) { setOurPeerName(peers[ourCurrentId].name); } try { const hashes = await api.getVerifyConfigHash(); + if (!mounted) return; setMyHash(hashes[ourCurrentId]); setPeersWithHash( // ... existing code ... ); if ( peers[ourCurrentId].status === GuardianServerStatus.VerifiedConfigs ) { const otherPeers = Object.entries(peers).filter( ([id]) => id !== ourCurrentId.toString() ); setEnteredHashes( otherPeers.map(([id]) => hashes[id as unknown as number]) ); } } catch (err) { + if (mounted) { setError(formatApiErrorMessage(err)); + } } } assembleHashInfo(); + return () => { + mounted = false; + }; }, [api, peers, ourCurrentId, t]);
Line range hint
332-334: Add accessibility attributes to interactive elements.The Input components lack proper aria-labels for screen readers. Add aria-labels to improve accessibility:
<Input variant='filled' value={value} + aria-label={`${t('verify-guardians.hash-input-label')} ${peer.name}`} placeholder={`${t('verify-guardians.verified-placeholder')}`} onChange={(ev) => handleChangeHash(ev.currentTarget.value, idx)} readOnly={isValid} />Also applies to: 339-341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (31)
- apps/router/src/components/Header.tsx (1 hunks)
- apps/router/src/context/guardian/AdminContext.tsx (1 hunks)
- apps/router/src/context/guardian/GuardianContext.tsx (1 hunks)
- apps/router/src/context/guardian/SetupContext.tsx (2 hunks)
- apps/router/src/gateway-ui/components/ConnectFederationModal.tsx (1 hunks)
- apps/router/src/gateway-ui/components/HeaderWithUnitSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx (1 hunks)
- apps/router/src/guardian-ui/Guardian.tsx (1 hunks)
- apps/router/src/guardian-ui/admin/FederationAdmin.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/danger/DownloadBackup.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/danger/SignApiAnnouncement.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/FederationTabsCard.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/balanceSheet/BalanceSheet.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/proposals/ProposedMetas.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/status/GatewaysStatus.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/RunDKG.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/setupComplete/SetupComplete.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (1 hunks)
- apps/router/src/guardian-ui/setup/FederationSetup.tsx (1 hunks)
- apps/router/src/home/ConnectServiceModal.tsx (1 hunks)
- apps/router/src/home/EditServiceModal.tsx (1 hunks)
- apps/router/src/home/HomePage.tsx (1 hunks)
- apps/router/src/home/RemoveServiceModal.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardian.tsx (1 hunks)
- apps/router/src/hooks/index.tsx (1 hunks)
- apps/router/src/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/router/src/guardian-ui/components/dashboard/danger/SignApiAnnouncement.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx
- apps/router/src/guardian-ui/components/setup/RunDKG.tsx
- apps/router/src/home/HomePage.tsx
🧰 Additional context used
📓 Learnings (7)
apps/router/src/context/guardian/GuardianContext.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.apps/router/src/context/guardian/SetupContext.tsx (3)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/guardian/SetupContext.tsx:71-71 Timestamp: 2024-10-10T20:23:07.421Z Learning: In `apps/router/src/context/guardian/SetupContext.tsx`, the `SET_INITIAL_STATE` action in the reducer returns the current state without modification because handling is performed in the provider.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/guardian/SetupContext.tsx:135-137 Timestamp: 2024-09-27T21:39:41.672Z Learning: In this codebase, the context is only loaded while in `guardian/id`, so `guardianId` will always be defined, and validation for `guardianId` being undefined is unnecessary.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.apps/router/src/guardian-ui/Guardian.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#559 File: apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx:105-109 Timestamp: 2024-10-15T03:04:45.827Z Learning: In `MetaManager.tsx`, the `proposeMetaEdits` function handles the case where `sites` is an empty string.apps/router/src/hooks/guardian/useGuardian.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.apps/router/src/hooks/index.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.apps/router/src/index.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.
🔇 Additional comments (19)
apps/router/src/hooks/index.tsx (1)
3-5: LGTM: Clean hook organizationThe exports follow a clear modular structure, aligning with the PR's objective of proper hooks organization.
apps/router/src/gateway-ui/components/HeaderWithUnitSelector.tsx (1)
4-4: Verify the hook's new location.The import path change aligns with the PR's objective of implementing a proper hooks structure.
✅ Verification successful
The previous commands failed because we assumed a specific directory structure. Let's first locate the files and then analyze their relationships without making path assumptions.
The results show that all imports are using the new path structure, but we need to verify that the hook implementation exists in the new location and has been properly moved from the old location.
Import path change is correct and consistent
The hook exists at
apps/router/src/hooks/gateway/useGateway.tsxand all components are using the updated import paths. No remaining references to the oldcontext/hookspath were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hook exists in the new location and all imports are updated # Test 1: Check if the hook exists in the new location fd "useGatewayContext" src/gateway-ui/hooks # Test 2: Check for any remaining old imports that need updating rg "from '../../context/hooks'" src/gateway-uiLength of output: 261
Script:
#!/bin/bash # Let's try searching from the repository root without assuming directory structure # First, find the correct path to the component fd "HeaderWithUnitSelector.tsx" # Then search for useGatewayContext hook file fd "useGatewayContext" # Search for any imports of useGatewayContext rg "import.*useGatewayContext" # Search for any remaining old context/hooks imports rg "from.*context/hooks"Length of output: 1785
Script:
#!/bin/bash # Let's verify the hook implementation exists in the new location ast-grep --pattern 'export const useGatewayContext' # Also check the old location to ensure it's been moved fd "hooks.ts" apps/router/src/gateway-ui/contextLength of output: 831
apps/router/src/guardian-ui/components/setup/screens/setupComplete/SetupComplete.tsx (1)
9-9: LGTM: Clean transition to hooks-based dispatchThe change to use a dedicated dispatch hook aligns well with the PR's objective of proper hooks structure.
Also applies to: 17-17
apps/router/src/components/Header.tsx (1)
6-6: LGTM! Import path update aligns with hooks restructuring.The change to consolidate hooks into a single directory is consistent with the PR's objective of improving context management.
apps/router/src/home/RemoveServiceModal.tsx (1)
14-14: Verify hooks directory structure.The import path change aligns with the PR's objective of restructuring hooks. Let's verify the consistency of this change across the codebase.
✅ Verification successful
Import path change is consistent and verified
The verification confirms:
- The new hooks directory exists at
apps/router/src/hooks/- No imports remain from the old path
../context/hooks- All
useAppContextimports consistently use the new path../hooksacross components🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify hooks directory structure and import consistency # Test 1: Verify hooks directory exists fd -t d "hooks$" # Test 2: Check for any remaining imports from old path rg "from '\.\./context/hooks'" # Test 3: Verify all useAppContext imports are from new path rg "from '\.\./hooks'" -A 1 | grep "useAppContext"Length of output: 601
apps/router/src/context/guardian/GuardianContext.tsx (2)
Line range hint
1-83: Implementation looks good!The context implementation follows React best practices with proper typing, efficient state management, and clean separation of concerns.
15-15: Verify the updated import path.✅ Verification successful
Let me try a different search strategy to verify the import path and hook existence.
This should help us:
- Find all hooks-related files and directories
- Locate the useGuardianConfig definition and its usage
- Verify files specifically in the router app that use this hook
Import path is correct and hook exists at expected location
The
useGuardianConfighook is defined inapps/router/src/hooks/guardian/useGuardian.tsxand the relative import path../../hooksfromapps/router/src/context/guardian/GuardianContext.tsxcorrectly resolves to this location.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the import path for useGuardianConfig hook exists # and contains the expected exports # Test 1: Check if the hooks file exists at the new path fd --type f "hooks.tsx?" --exec echo "Found hooks file: {}" # Test 2: Verify the useGuardianConfig export exists ast-grep --pattern 'export function useGuardianConfig($_) { $$$ }'Length of output: 130
Script:
#!/bin/bash # Search for hooks directory and files fd hooks --type d fd hooks --type f # Search for useGuardianConfig in all files rg "useGuardianConfig" -B2 -A2 # Search specifically in the router app fd . apps/router/src --type f --exec grep -l "useGuardianConfig" {} \;Length of output: 1840
apps/router/src/guardian-ui/Guardian.tsx (1)
20-23: LGTM! Clean separation of concernsThe granular hooks approach improves code maintainability and follows React best practices.
apps/router/src/guardian-ui/components/dashboard/danger/DownloadBackup.tsx (1)
16-16: LGTM! Import path update aligns with hooks restructuring.The change to import from a dedicated hooks directory follows React best practices for organizing custom hooks.
Let's verify the hook's new location and consistent usage across the codebase:
✅ Verification successful
The previous search failed because we need to understand the repository structure first. These commands will help us:
- Find all files containing the hook
- See the implementation and import context
- Understand the directory structure to locate the hooks directory
Based on the search results, I can now provide a final response:
Import path change is correct and consistent with codebase structure
The hook has been properly moved from the context directory to a dedicated hooks directory at
apps/router/src/hooks/guardian/useGuardian.tsx, and all components are consistently importing it from the new location. The search results show:
- The hook implementation exists in the new location
- Multiple components are using the new import path
- No remaining imports from the old context/hooks location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new hooks directory structure and usage # Test 1: Check if the hook exists in the new location fd "useGuardianAdminApi" "hooks" # Test 2: Ensure no remaining imports from the old location rg "from.*context/hooks.*useGuardianAdminApi" # Test 3: Check for consistent usage of the new import path rg "from.*hooks.*useGuardianAdminApi"Length of output: 211
Script:
#!/bin/bash # Let's try a broader search to understand the structure # First, find all files that might contain our hook rg -l "useGuardianAdminApi" # Then, check the actual implementation and imports rg "useGuardianAdminApi" -A 5 -B 5 # Also search for any files in the apps/router/src directory structure fd . "apps/router/src"Length of output: 35982
apps/router/src/index.tsx (1)
23-23: Verify the new hooks directory structure.The import path change suggests a restructuring of hooks. Let's verify the new structure is properly set up.
✅ Verification successful
Let me try different commands to verify the hooks directory structure and imports.
Import path change is correct and consistent
The new hooks directory structure exists at
apps/router/src/hooks/and all imports ofuseAppContextacross the codebase consistently use the new path structure. No references to the oldcontext/hookspath remain.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hooks directory structure and import consistency # Test 1: Check if the hooks directory exists and contains the expected files echo "Checking hooks directory structure..." fd --type f . -p "*/hooks/" # Test 2: Check for any remaining imports from the old path echo "Checking for potentially inconsistent imports..." rg "from './context/hooks'" --type tsxLength of output: 392
Script:
#!/bin/bash # Check for hooks directory and its contents echo "Checking hooks directory..." fd hooks # Look for useAppContext imports across the codebase echo -e "\nChecking useAppContext imports..." rg "import.*useAppContext" # Look for any files in context/hooks echo -e "\nChecking old context/hooks path..." fd -p "context/hooks"Length of output: 848
apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (1)
21-21: LGTM: Import path update aligns with hooks restructuringThe updated import path reflects the improved hooks organization structure.
apps/router/src/gateway-ui/components/ConnectFederationModal.tsx (1)
18-18: LGTM! Verify hooks directory structure.The import path change aligns with the PR objective of restructuring hooks.
Let's verify the hooks directory structure:
✅ Verification successful
Import path change verified and correct
The hooks have been properly moved to
apps/router/src/hooks/gateway/useGateway.tsxand no references to the oldcontext/hookspath remain in the codebase. The import statement in ConnectFederationModal.tsx correctly reflects this new structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hooks directory structure and imports # Expect: Files moved from context/hooks to hooks directory # Check if hooks directory exists and contains the expected files fd --type f . -p "**/hooks/" # Verify no remaining imports from the old path rg "from '.*context/hooks'"Length of output: 163
Script:
#!/bin/bash # Let's try a different approach to verify the hooks directory structure # Find all files with 'hooks' in the path fd hooks # Check for any remaining imports from context/hooks rg "from.*context/hooks" # Look for the specific hooks being imported rg "export.*useGatewayApi|export.*useGatewayContext|export.*useGatewayInfo"Length of output: 465
apps/router/src/guardian-ui/components/dashboard/tabs/status/GatewaysStatus.tsx (1)
17-17: Verify consistent hook import paths across the codebase.The hook import path has been updated as part of the hooks directory restructuring.
✅ Verification successful
Let me try different commands to verify the hook imports.
The previous commands failed due to unsupported file type flags. Let's try with basic pattern matching and also look for hook-related files to understand the structure better.
Import path change is consistent with codebase patterns
The verification shows that:
- No imports from 'context/hooks' exist in the codebase
- All hook imports follow the relative path pattern from 'hooks' directory
- The changed import in GatewaysStatus.tsx aligns with the established pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old context/hooks path # to ensure consistent migration across the codebase # Test: Search for old import paths. Expect: No results rg "from '.*context/hooks'" --type ts --type tsx # Test: Search for new import paths. Expect: Consistent usage rg "from '.*hooks'" --type ts --type tsxLength of output: 161
Script:
#!/bin/bash # Search for old import paths without file type flags rg "from '.*context/hooks'" # Search for new hook imports without file type flags rg "from '.*hooks'" # List all TypeScript/TSX files that might contain hook imports fd "\.(ts|tsx)$" | grep -i "hook"Length of output: 5230
apps/router/src/context/guardian/SetupContext.tsx (2)
14-18: LGTM!The import changes look good. The switch to
useGuardianApiand the additional hooks imported are appropriate.
126-126: LGTM!The usage of
useGuardianApito get theapiobject is consistent with the import change and looks good.apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1)
6-6: LGTM: Import path simplifiedThe simplified import path aligns with the hooks directory restructuring.
apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx (1)
Line range hint
33-196: Consider moving more state to context per PR objectives.While the import has been updated to use the new hooks directory, the component still heavily relies on local state management (useCallback, useMemo, etc.). Given that the PR's objective is to "move the state into a context", consider moving more of the component's state management into the context.
Let's check the context implementation:
apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (1)
Line range hint
200-220: Improve password security in submission handler.The password is being passed directly to the configuration. Consider hashing it before transmission or using a more secure method of password handling.
apps/router/src/hooks/guardian/useGuardian.tsx (1)
132-139: Duplicate: Potential type mismatch inuseGuardianAdminApiAs previously noted, the
useGuardianAdminApihook returnsguardian.api, which is of typeGuardianApi, but the expected return type isAdminApiInterface. Please verify thatguardian.apiis of the correct type or adjust the return statement to return the appropriate property.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Kodylow
force-pushed
the
gateway-context
branch
from
988b113
to
a0dd327
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (4)
apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1)
Line range hint
36-44: Handle undefined federations in mappingThe mapping over
state.gatewayInfo?.federationscould fail if federations is undefined, and there's no handling for empty state.- {state.gatewayInfo?.federations.map((federation) => ( + {state.gatewayInfo?.federations?.map((federation) => ( <option key={federation.federation_id} value={federation.federation_id} > {federation.config.global.meta.federation_name || federation.federation_id} </option> - ))} + )) || <option value="">No federations available</option>}apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (1)
Line range hint
21-23: Remove test values from production code.The contract is using hard-coded test values which should not be in production code.
- const incomingContract: IncomingContract = { - commitment: 'test', - ciphertext: 'test', - }; + const incomingContract: IncomingContract = { + commitment: crypto.randomUUID(), + ciphertext: crypto.randomUUID(), + };apps/router/src/gateway-ui/components/walletModal/index.tsx (2)
Line range hint
1-1: Address component organization TODOThe TODO comment indicates these components should be split into multiple files. This aligns with the PR's objective of proper organization and would improve maintainability.
Consider:
- Moving each component to its own file under a
componentsdirectory- Creating an index file to re-export them
- Grouping related components (e.g., QRCode-related components together)
Line range hint
98-127: Extract copy functionality into a custom hookThe copy-to-clipboard logic with timeout management should be abstracted into a reusable hook, especially since this pattern might be needed elsewhere in the application.
Create a custom hook:
// hooks/useCopyToClipboard.ts export function useCopyToClipboard(timeout = 2000) { const [copied, setCopied] = React.useState(false); const copy = React.useCallback((onCopy: () => void) => { onCopy(); setCopied(true); setTimeout(() => setCopied(false), timeout); }, [timeout]); return { copied, copy }; }Then simplify the component:
const QRCodePanel: React.FC<{ value: string; onCopy: () => void }> = ({ value, onCopy, }) => { - const [copied, setCopied] = React.useState(false); - - const handleCopy = () => { - onCopy(); - setCopied(true); - setTimeout(() => setCopied(false), 2000); - }; + const { copied, copy } = useCopyToClipboard(); + const handleCopy = () => copy(onCopy);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (51)
- apps/router/src/components/Header.tsx (1 hunks)
- apps/router/src/context/gateway/GatewayContext.tsx (3 hunks)
- apps/router/src/context/guardian/AdminContext.tsx (1 hunks)
- apps/router/src/context/guardian/GuardianContext.tsx (1 hunks)
- apps/router/src/context/guardian/SetupContext.tsx (2 hunks)
- apps/router/src/context/hooks.tsx (0 hunks)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/components/ConnectFederationModal.tsx (1 hunks)
- apps/router/src/gateway-ui/components/HeaderWithUnitSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx (1 hunks)
- apps/router/src/gateway-ui/components/federations/FederationsTable.tsx (4 hunks)
- apps/router/src/gateway-ui/components/lightning/LightningCard.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletActionSelector.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/WalletTypeButtons.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/index.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx (2 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendLightning.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx (3 hunks)
- apps/router/src/guardian-ui/Guardian.tsx (1 hunks)
- apps/router/src/guardian-ui/admin/FederationAdmin.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/danger/DownloadBackup.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/danger/SignApiAnnouncement.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/FederationTabsCard.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/balanceSheet/BalanceSheet.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/proposals/ProposedMetas.tsx (1 hunks)
- apps/router/src/guardian-ui/components/dashboard/tabs/status/GatewaysStatus.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/RunDKG.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (3 hunks)
- apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/setupComplete/SetupComplete.tsx (1 hunks)
- apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (1 hunks)
- apps/router/src/guardian-ui/setup/FederationSetup.tsx (1 hunks)
- apps/router/src/home/ConnectServiceModal.tsx (1 hunks)
- apps/router/src/home/EditServiceModal.tsx (1 hunks)
- apps/router/src/home/HomePage.tsx (1 hunks)
- apps/router/src/home/RemoveServiceModal.tsx (1 hunks)
- apps/router/src/hooks/gateway/useGateway.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardian.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardianSetup.tsx (1 hunks)
- apps/router/src/hooks/index.tsx (1 hunks)
- apps/router/src/index.tsx (1 hunks)
- apps/router/src/types/gateway.tsx (4 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- apps/router/src/gateway-ui/components/lightning/LightningCard.tsx
💤 Files with no reviewable changes (1)
- apps/router/src/context/hooks.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/router/src/guardian-ui/admin/FederationAdmin.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/FederationTabsCard.tsx
🚧 Files skipped from review as they are similar to previous changes (34)
- apps/router/src/components/Header.tsx
- apps/router/src/context/gateway/GatewayContext.tsx
- apps/router/src/context/guardian/AdminContext.tsx
- apps/router/src/context/guardian/GuardianContext.tsx
- apps/router/src/context/guardian/SetupContext.tsx
- apps/router/src/gateway-ui/components/ConnectFederationModal.tsx
- apps/router/src/gateway-ui/components/HeaderWithUnitSelector.tsx
- apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx
- apps/router/src/gateway-ui/components/walletModal/WalletActionSelector.tsx
- apps/router/src/gateway-ui/components/walletModal/WalletTypeButtons.tsx
- apps/router/src/gateway-ui/components/walletModal/send/SendLightning.tsx
- apps/router/src/gateway-ui/components/walletModal/send/SendOnchain.tsx
- apps/router/src/guardian-ui/Guardian.tsx
- apps/router/src/guardian-ui/components/dashboard/danger/DownloadBackup.tsx
- apps/router/src/guardian-ui/components/dashboard/danger/SignApiAnnouncement.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/balanceSheet/BalanceSheet.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/meta/proposals/ProposedMetas.tsx
- apps/router/src/guardian-ui/components/dashboard/tabs/status/GatewaysStatus.tsx
- apps/router/src/guardian-ui/components/setup/RunDKG.tsx
- apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx
- apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx
- apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx
- apps/router/src/guardian-ui/components/setup/screens/setupComplete/SetupComplete.tsx
- apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx
- apps/router/src/guardian-ui/setup/FederationSetup.tsx
- apps/router/src/home/ConnectServiceModal.tsx
- apps/router/src/home/EditServiceModal.tsx
- apps/router/src/home/HomePage.tsx
- apps/router/src/home/RemoveServiceModal.tsx
- apps/router/src/hooks/index.tsx
- apps/router/src/index.tsx
- apps/router/src/types/gateway.tsx
🧰 Additional context used
📓 Learnings (3)
apps/router/src/gateway-ui/components/lightning/LightningCard.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/lightning/LightningCard.tsx:90-91 Timestamp: 2024-10-22T20:27:07.540Z Learning: In the codebase, `gatewayInfo.lightning_mode` is always defined.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/lightning/LightningCard.tsx:34-35 Timestamp: 2024-10-22T20:26:55.760Z Learning: Potential null reference exceptions with `gatewayInfo` are handled within the `useGatewayContext` hook, so in components like `LightningCard.tsx`, explicit null checks for `gatewayInfo` are unnecessary.apps/router/src/hooks/guardian/useGuardian.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.apps/router/src/hooks/guardian/useGuardianSetup.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.
🪛 Biome
apps/router/src/hooks/guardian/useGuardianSetup.tsx
[error] 71-71: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (16)
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (2)
7-10: LGTM: Clean transition to context-based architectureThe removal of props in favor of context aligns well with the PR objectives of moving state into context.
12-12: LGTM: Proper context destructuringThe destructuring provides clean access to state, dispatch, and api from the context.
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (1)
25-26: Federation selection validation issue.The previous review comment about federation selection validation is still valid. The code should validate federation selection before proceeding.
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveOnchain.tsx (2)
19-33: Error handling needs improvementThe current implementation only shows an alert and logs to console, which doesn't provide proper error state management for users.
39-41: Add optional chaining for nested propertiesAccessing nested properties without optional chaining could cause runtime errors.
apps/router/src/gateway-ui/components/walletModal/WalletModal.tsx (1)
66-67: Fix missing declaration for 'Component'.The Component variable issue from the previous review remains unfixed.
apps/router/src/hooks/gateway/useGateway.tsx (5)
43-50: Redundant null check in useGatewayDispatch
52-59: Update error message in useGatewayInfo
63-65: Avoid direct state mutation
109-115: Incorrect error handling in fetchBalances
67-124:⚠️ Potential issueAdd missing dependency to useEffect
The
idfrom useGatewayContext is used in sessionStorage check but not included in the dependencies array. This could lead to stale values if id changes.- }, [state.needsAuth, api, dispatch]); + }, [state.needsAuth, api, dispatch, id]);Likely invalid or redundant comment.
apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (2)
24-28: Previous review comment about balance calculations is still applicable.The concerns about balance calculation robustness, including null checks and error handling, remain valid for these code segments.
Also applies to: 30-34, 41-44, 51-54
133-141:⚠️ Potential issuePrevious review comment about federation selection validation is still applicable.
Additionally, the modal handlers contain duplicated code that differs only in the action type.
Consider extracting the common logic:
const handleModalAction = (action: WalletModalAction) => { dispatch({ type: GATEWAY_APP_ACTION_TYPE.SET_WALLET_MODAL_STATE, payload: { action, type: WalletModalType.Onchain, selectedFederation: state.gatewayInfo?.federations[0] ?? null, showSelector: true, isOpen: true, }, }); };Then use it in both click handlers:
onClick={() => handleModalAction(WalletModalAction.Receive)} onClick={() => handleModalAction(WalletModalAction.Send)}Also applies to: 154-162
apps/router/src/hooks/guardian/useGuardian.tsx (2)
15-18: Verify the necessity ofuseAppGuardianConfigsfunctionThe
useAppGuardianConfigsfunction was previously removed because it was unused. Please ensure that it is now required and used within the codebase to avoid redundant code.
132-139: Potential type mismatch inuseGuardianAdminApiThe
useGuardianAdminApihook returnsguardian.api, which is of typeGuardianApi, but the expected return type isAdminApiInterface. Please verify thatguardian.apiis of the correct type or adjust the return statement to return the appropriate property.apps/router/src/gateway-ui/components/federations/FederationsTable.tsx (1)
Line range hint
18-101: Changes are correctly implemented.The refactoring to use context-based state management is successfully applied, and the code integrates well with the application's global state.
apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx
Outdated
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…lector.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1)
Line range hint
36-43: Add error handling for empty federations listThe component doesn't handle the case where federations array is empty, which could lead to a confusing empty select box.
- {state.gatewayInfo?.federations.map((federation) => ( + {state.gatewayInfo?.federations?.length ? ( + state.gatewayInfo.federations.map((federation) => ( <option key={federation.federation_id} value={federation.federation_id} > {federation.config.global.meta.federation_name || federation.federation_id} </option> + )) + ) : ( + <option value="">No federations available</option> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1 hunks)
🔇 Additional comments (1)
apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx (1)
21-21: Add null check for walletModalState access
apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (2)
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (1)
Line range hint
21-24: Remove test values for contract properties.The contract properties are using hardcoded test values which suggests incomplete implementation.
Replace with actual contract data or document why test values are being used.
apps/router/src/gateway-ui/components/walletModal/index.tsx (1)
Line range hint
1-1: Address the TODO comment about component splitting.Since this PR focuses on improving the codebase structure, now would be a good time to split these shared components into separate files.
Would you like me to help create a plan for splitting these components or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/components/walletCard/WalletCard.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/index.tsx (2 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (1 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveLightning.tsx (3 hunks)
- apps/router/src/gateway-ui/components/walletModal/send/SendEcash.tsx (3 hunks)
- apps/router/src/gateway-ui/hooks/useBalanceCalculations.ts (1 hunks)
- apps/router/src/hooks/gateway/useGateway.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardian.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardianSetup.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (4)
apps/router/src/gateway-ui/Gateway.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx:21-21 Timestamp: 2024-10-25T19:12:34.549Z Learning: In `apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx`, `state.walletModalState` is always initialized in the Gateway context, so null checks for `state.walletModalState` are unnecessary.apps/router/src/gateway-ui/components/walletModal/index.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx:21-21 Timestamp: 2024-10-25T19:12:34.549Z Learning: In `apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx`, `state.walletModalState` is always initialized in the Gateway context, so null checks for `state.walletModalState` are unnecessary.apps/router/src/hooks/guardian/useGuardian.tsx (4)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/hooks/guardian/useGuardian.tsx:15-18 Timestamp: 2024-10-25T19:11:32.141Z Learning: The `useAppGuardianConfigs` function is now used in the codebase and should not be considered redundant.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/guardian-ui/setup/FederationSetup.tsx:23-23 Timestamp: 2024-10-25T19:08:52.542Z Learning: In `apps/router/src/guardian-ui/setup/FederationSetup.tsx`, the hooks `useGuardianSetupApi` and `useGuardianSetupContext` are correctly imported from `'../../hooks'`.apps/router/src/hooks/guardian/useGuardianSetup.tsx (2)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/hooks/guardian/useGuardianSetup.tsx:71-78 Timestamp: 2024-10-25T19:06:33.427Z Learning: In the file `apps/router/src/hooks/guardian/useGuardianSetup.tsx`, the case clause for `GuardianServerStatus.ConsensusRunning` in the function `useHandleSetupServerStatus` is intentionally kept for debugging purposes.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/guardian-ui/setup/FederationSetup.tsx:23-23 Timestamp: 2024-10-25T19:08:52.542Z Learning: In `apps/router/src/guardian-ui/setup/FederationSetup.tsx`, the hooks `useGuardianSetupApi` and `useGuardianSetupContext` are correctly imported from `'../../hooks'`.
🪛 Biome
apps/router/src/hooks/guardian/useGuardianSetup.tsx
[error] 71-71: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (2)
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (1)
22-25:⚠️ Potential issueConsider using a function update for state changes
The spread operator with existing state could lead to race conditions. Consider using a function update pattern.
- dispatch({ - type: GATEWAY_APP_ACTION_TYPE.SET_WALLET_MODAL_STATE, - payload: { ...state.walletModalState, showSelector: false }, - }); + dispatch({ + type: GATEWAY_APP_ACTION_TYPE.SET_WALLET_MODAL_STATE, + payload: (prevState) => ({ ...prevState, showSelector: false }), + });Likely invalid or redundant comment.
apps/router/src/hooks/guardian/useGuardianSetup.tsx (1)
162-176:⚠️ Potential issueExtract polling interval to a constant.
The polling interval is hardcoded. Extract it to a constant for better maintainability.
Apply this diff:
+const CONSENSUS_POLL_INTERVAL_MS = 2000; + useEffect(() => { if (!isPollingConsensus) return; let timeout: ReturnType<typeof setTimeout>; const pollPeers = () => { fetchConsensusState() .catch((err) => { console.warn('Failed to poll for peers', err); }) .finally(() => { - timeout = setTimeout(pollPeers, 2000); + timeout = setTimeout(pollPeers, CONSENSUS_POLL_INTERVAL_MS); }); }; pollPeers(); return () => clearTimeout(timeout); }, [isPollingConsensus, fetchConsensusState]);Likely invalid or redundant comment.
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…iveEcash.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/components/walletModal/index.tsx (2 hunks)
- apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (1 hunks)
- apps/router/src/gateway-ui/hooks/useBalanceCalculations.ts (1 hunks)
- apps/router/src/hooks/gateway/useGateway.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardian.tsx (1 hunks)
- apps/router/src/hooks/guardian/useGuardianSetup.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (4)
apps/router/src/gateway-ui/Gateway.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx:21-21 Timestamp: 2024-10-25T19:12:34.549Z Learning: In `apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx`, `state.walletModalState` is always initialized in the Gateway context, so null checks for `state.walletModalState` are unnecessary.apps/router/src/gateway-ui/components/walletModal/index.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx:21-21 Timestamp: 2024-10-25T19:12:34.549Z Learning: In `apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx`, `state.walletModalState` is always initialized in the Gateway context, so null checks for `state.walletModalState` are unnecessary.apps/router/src/hooks/guardian/useGuardian.tsx (4)
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/hooks/guardian/useGuardian.tsx:15-18 Timestamp: 2024-10-25T19:11:32.141Z Learning: The `useAppGuardianConfigs` function is now used in the codebase and should not be considered redundant.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/guardian-ui/setup/FederationSetup.tsx:23-23 Timestamp: 2024-10-25T19:08:52.542Z Learning: In `apps/router/src/guardian-ui/setup/FederationSetup.tsx`, the hooks `useGuardianSetupApi` and `useGuardianSetupContext` are correctly imported from `'../../hooks'`.apps/router/src/hooks/guardian/useGuardianSetup.tsx (3)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/hooks/guardian/useGuardianSetup.tsx:71-78 Timestamp: 2024-10-25T19:06:33.427Z Learning: In the file `apps/router/src/hooks/guardian/useGuardianSetup.tsx`, the case clause for `GuardianServerStatus.ConsensusRunning` in the function `useHandleSetupServerStatus` is intentionally kept for debugging purposes.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/guardian-ui/setup/FederationSetup.tsx:23-23 Timestamp: 2024-10-25T19:08:52.542Z Learning: In `apps/router/src/guardian-ui/setup/FederationSetup.tsx`, the hooks `useGuardianSetupApi` and `useGuardianSetupContext` are correctly imported from `'../../hooks'`.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.
🪛 Biome
apps/router/src/gateway-ui/hooks/useBalanceCalculations.ts
[error] 31-31: Expected an expression, or an assignment but instead found 'throw'.
Expected an expression, or an assignment here.
(parse)
[error] 33-43: This code is unreachable
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
apps/router/src/hooks/guardian/useGuardianSetup.tsx
[error] 71-71: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (9)
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx (2)
7-10: LGTM: Clean transition to context-based state managementThe removal of props in favor of context aligns well with the PR objectives.
12-12: LGTM: Proper context usageContext destructuring follows best practices.
apps/router/src/gateway-ui/components/walletModal/index.tsx (2)
19-19: LGTM!The Sats type import is correctly used in the AmountInput component.
51-60: Document the distinction between isLoading and isDisabled.While Chakra UI's Button automatically disables during loading, the separate isDisabled prop might be needed for other cases. Consider adding a comment explaining when to use each prop to prevent confusion.
apps/router/src/hooks/guardian/useGuardianSetup.tsx (2)
187-190:⚠️ Potential issueThrow error instead of console.error.
For consistency with error handling in
fetchConsensusState, throw an error whenmyNameis missing.- if (!myName) { - console.error('myName is required to submit configuration'); - return; - } + if (!myName) { + throw new Error('myName is required to submit configuration'); + }Likely invalid or redundant comment.
168-182: 🛠️ Refactor suggestionExtract polling interval to a configuration.
The hardcoded 2000ms polling interval should be configurable to support different environments.
+const CONSENSUS_POLL_INTERVAL = 2000; // ms + useEffect(() => { if (!isPollingConsensus) return; let timeout: ReturnType<typeof setTimeout>; const pollPeers = () => { fetchConsensusState() .catch((err) => { console.warn('Failed to poll for peers', err); }) .finally(() => { - timeout = setTimeout(pollPeers, 2000); + timeout = setTimeout(pollPeers, CONSENSUS_POLL_INTERVAL); }); }; pollPeers(); return () => clearTimeout(timeout); }, [isPollingConsensus, fetchConsensusState]);⛔ Skipped due to learnings
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/hooks/guardian/useGuardianSetup.tsx:71-78 Timestamp: 2024-10-25T19:06:33.427Z Learning: In the file `apps/router/src/hooks/guardian/useGuardianSetup.tsx`, the case clause for `GuardianServerStatus.ConsensusRunning` in the function `useHandleSetupServerStatus` is intentionally kept for debugging purposes.apps/router/src/hooks/gateway/useGateway.tsx (2)
88-94: Ensure robust error handling when casting errors
106-112: Handle errors safely when the error type is unknownapps/router/src/hooks/guardian/useGuardian.tsx (1)
25-30:⚠️ Potential issueHandle potential undefined
guardiansinuseGuardianConfigAccessing
guardians[id]without verifying thatguardiansis defined may result in errors ifguardiansis undefined.Apply this diff to fix the issue:
export const useGuardianConfig = (id: string): GuardianConfig => { const { guardians } = useAppContext(); + if (!guardians) + throw new Error('useGuardianConfig must be used with guardians defined in context'); if (!guardians[id]) throw new Error('useGuardianConfig must be used with a selected guardian'); return guardians[id].config; };⛔ Skipped due to learnings
Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:69-69 Timestamp: 2024-10-10T21:30:04.372Z Learning: In `apps/router/src/context/hooks.tsx`, the function `useGuardianConfig` returns an object with `id` and `config` properties, and its return type matches the interface accordingly.Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/hooks/guardian/useGuardian.tsx:15-18 Timestamp: 2024-10-25T19:11:32.141Z Learning: The `useAppGuardianConfigs` function is now used in the codebase and should not be considered redundant.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/hooks.tsx:51-54 Timestamp: 2024-10-10T21:28:22.154Z Learning: The function `useAppGuardianConfigs` is unused and has been removed.Learnt from: Kodylow PR: fedimint/ui#547 File: apps/router/src/context/guardian/SetupContext.tsx:135-137 Timestamp: 2024-09-27T21:39:41.672Z Learning: In this codebase, the context is only loaded while in `guardian/id`, so `guardianId` will always be defined, and validation for `guardianId` being undefined is unnecessary.
apps/router/src/gateway-ui/components/walletModal/receive/ReceiveEcash.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/router/src/gateway-ui/Gateway.tsx (4 hunks)
- apps/router/src/gateway-ui/hooks/useBalanceCalculations.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/router/src/gateway-ui/Gateway.tsx (1)
Learnt from: Kodylow PR: fedimint/ui#570 File: apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx:21-21 Timestamp: 2024-10-25T19:12:34.549Z Learning: In `apps/router/src/gateway-ui/components/walletModal/FederationSelector.tsx`, `state.walletModalState` is always initialized in the Gateway context, so null checks for `state.walletModalState` are unnecessary.
🔇 Additional comments (1)
apps/router/src/gateway-ui/Gateway.tsx (1)
28-29:⚠️ Potential issueAdd error handling for useLoadGateway
The
useLoadGatewayhook is called without any error handling. Consider wrapping it in a try-catch block or handling potential errors to prevent silent failures.- useLoadGateway(); + try { + useLoadGateway(); + } catch (error) { + dispatch({ + type: GATEWAY_APP_ACTION_TYPE.SET_GATEWAY_ERROR, + payload: error + }); + }Likely invalid or redundant comment.
moves state into context and wormholes it around as needed using proper hooks structure.
Summary by CodeRabbit
Release Notes
New Features
useBalanceCalculations, for optimized balance calculations.Bug Fixes
Refactor
Chores