Hoprd 2.1.5 #636
Hoprd 2.1.5 #636
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Outside diff range and nitpick comments (7)
src/pages/node/channelsOutgoing.tsx (2)
Line range hint
91-134: Consider improving error handling and constants managementA few suggestions to enhance the error handling:
- Consider using template literals instead of string concatenation for error messages
- The timeout value (5 * 60_000) should be extracted as a named constant
- The error handling structure could be flattened using early returns
Here's a suggested refactoring:
+ const CHANNEL_CLOSE_TIMEOUT_MS = 5 * 60_000; // 5 minutes const handleCloseChannels = (channelId: string) => { dispatch( actionsAsync.closeChannelThunk({ apiEndpoint: loginData.apiEndpoint!, apiToken: loginData.apiToken ? loginData.apiToken : '', channelId: channelId, - timeout: 5 * 60_000, + timeout: CHANNEL_CLOSE_TIMEOUT_MS, }), ) .unwrap() .then(() => { handleRefresh(); }) .catch(async (e) => { const isCurrentApiEndpointTheSame = await dispatch( actionsAsync.isCurrentApiEndpointTheSame(loginData.apiEndpoint!), ).unwrap(); if (!isCurrentApiEndpointTheSame) return; if ( e instanceof sdkApiError && e.hoprdErrorPayload?.error?.includes('channel closure time has not elapsed yet, remaining') ) { - let errMsg = `Closing of outgoing channel ${channelId} halted. C${e.hoprdErrorPayload?.error.substring(1)}`; + const errMsg = `Closing of outgoing channel ${channelId} halted. ${e.hoprdErrorPayload?.error}`; sendNotification({ notificationPayload: { source: 'node', name: errMsg, url: null, timeout: null, }, toastPayload: { message: errMsg }, dispatch, }); return; } - let errMsg = `Closing of outgoing channel ${channelId} failed`; - if (e instanceof sdkApiError && e.hoprdErrorPayload?.status) - errMsg = errMsg + `.\n${e.hoprdErrorPayload.status}`; - if (e instanceof sdkApiError && e.hoprdErrorPayload?.error) errMsg = errMsg + `.\n${e.hoprdErrorPayload.error}`; + const errMsg = [ + `Closing of outgoing channel ${channelId} failed`, + e instanceof sdkApiError && e.hoprdErrorPayload?.status ? e.hoprdErrorPayload.status : null, + e instanceof sdkApiError && e.hoprdErrorPayload?.error ? e.hoprdErrorPayload.error : null, + ].filter(Boolean).join('\n');
Line range hint
1-285: Consider architectural improvements for better maintainabilityThe component handles multiple responsibilities and could benefit from some architectural improvements:
- The table data transformation logic in
parsedTableDatacould be extracted to a separate utility function- Error handling could be moved to a custom hook (e.g.,
useChannelOperations)- Consider adding proper TypeScript types for the channel and error payload structures
Example structure:
// types.ts interface Channel { id: string; peerAddress: string; balance: string; status: 'Open' | 'PendingToClose' | 'Closed'; isClosing?: boolean; } // hooks/useChannelOperations.ts const useChannelOperations = () => { const dispatch = useAppDispatch(); const closeChannel = async (channelId: string) => { // Error handling logic here }; return { closeChannel }; }; // utils/tableTransformations.ts const transformChannelsToTableData = ( channels: Record<string, Channel>, getAliasByPeerAddress: (address: string) => string ) => { // Table data transformation logic here };src/pages/node/channelsIncoming.tsx (2)
Line range hint
155-156: Consider moving the timeout value to configurationThe hardcoded timeout value of 120,000ms should be moved to a configuration constant, as indicated by the TODO comment.
+ // In a config file + export const CHANNEL_CLOSE_TIMEOUT = 120_000; // 2 minutes - average closing time is 50s - timeout: 120_000, //TODO: put those values as default to HOPRd SDK, average is 50s + timeout: CHANNEL_CLOSE_TIMEOUT,
Line range hint
183-246: Consider memoizing table data transformationThe table data transformation is complex and runs on every render. Consider using
useMemoto cache the results and improve performance:- const parsedTableData = Object.keys(channelsIncomingObject) + const parsedTableData = useMemo(() => Object.keys(channelsIncomingObject) .map((id, index) => { // ... existing transformation logic ... }) - .filter((elem) => elem !== undefined) as { + .filter((elem) => elem !== undefined), [ + channelsIncomingObject, + nodeAddressToOutgoingChannelLink, + tickets, + aliases + ]) as { id: string; key: string; // ... rest of the type definition ... }[];src/router.tsx (1)
319-324: LGTM! Consider optimizing dispatch sequence.The ticket price fetch is correctly implemented and properly integrated with the existing authentication flow.
Consider using
Promise.allto parallelize the independent data fetches for better performance:await Promise.all([ dispatch(nodeActionsAsync.getInfoThunk({ apiEndpoint, apiToken })), dispatch(nodeActionsAsync.getAddressesThunk({ apiEndpoint, apiToken })), dispatch(nodeActionsAsync.getAliasesThunk({ apiEndpoint, apiToken })), // ... other independent fetches dispatch(nodeActionsAsync.getTicketPriceThunk({ apiEndpoint, apiToken })) ]);src/components/Modal/node/SendMessageModal.tsx (1)
268-268: Consider using optional chaining for safer property access.While the simplified alias handling is good, consider using optional chaining to make the code more defensive against potential runtime errors.
- return Object.keys(aliases).includes(peerId); + return Object.keys(aliases ?? {}).includes(peerId); - if (aliases[peerId]) { - return aliases[peerId]; + if (aliases?.[peerId]) { + return aliases?.[peerId];Also applies to: 274-275
src/store/slices/node/actionsAsync.ts (1)
896-918: LGTM with a suggestion for improved error handling.The thunk implementation follows the established patterns and includes proper state management. Consider improving the error handling to be more specific about error types.
Consider updating the error handling to be more specific:
- } catch (e) { + } catch (e: unknown) { if (e instanceof sdkApiError) { return rejectWithValue(e); } - return rejectWithValue({ status: JSON.stringify(e) }); + return rejectWithValue({ + status: e instanceof Error ? e.message : JSON.stringify(e) + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(1 hunks)src/components/ConnectNode/modal.tsx(1 hunks)src/components/Modal/node/SendMessageModal.tsx(4 hunks)src/components/Modal/node/WithdrawModal.tsx(2 hunks)src/pages/node/channelsIncoming.tsx(1 hunks)src/pages/node/channelsOutgoing.tsx(1 hunks)src/pages/node/peers.tsx(1 hunks)src/router.tsx(1 hunks)src/store/slices/node/actionsAsync.ts(6 hunks)src/store/slices/node/actionsFetching.ts(3 hunks)src/store/slices/node/initialState.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments (19)
src/store/slices/node/actionsFetching.ts (3)
20-20: LGTM: Action creator follows established patterns
The new setTicketPriceFetching action creator is correctly implemented and maintains consistency with existing patterns.
39-39: LGTM: Export follows established patterns
The new action is correctly exported and maintains the alphabetical ordering in the nodeActionsFetching object.
84-86: Verify ticketPrice state structure exists
The reducer case implementation is correct and follows established patterns. However, let's verify that the ticketPrice state structure is properly initialized.
✅ Verification successful
Initial state structure verified successfully
The ticketPrice state structure is properly defined in the initial state with the required isFetching property initialized to false. The implementation is safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ticketPrice state structure in initial state
# Expected: Should find ticketPrice object with isFetching property in initialState
# Search for ticketPrice definition in initialState
rg -A 5 "ticketPrice.*[:=]" src/store/slices/node/initialState.ts
Length of output: 299
src/pages/node/peers.tsx (1)
54-54: LGTM! Simplified alias handling looks good.
The direct access to the aliases state object is a cleaner approach compared to using a selector. The implementation correctly handles the case when an alias exists by combining it with the peerId.
Let's verify that this is the only place where aliases are being accessed in this component:
✅ Verification successful
Let me gather more information to ensure there are no other alias-related patterns we should check:
This will help us verify if there are any other alias-related patterns or state management code that we should consider.
Alias handling implementation is consistent throughout the component
The verification confirms that the alias handling is consistent in the component:
- Direct access to
aliasesstate is used only in thegetAliasByPeerIdfunction - The component correctly fetches aliases data through the Redux store
- No legacy
peerIdToAliasLinkselector references remain - All alias-related patterns follow the new simplified approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to peerIdToAliasLink or other alias-related patterns
# in this component to ensure consistent usage of the new approach
# Test 1: Search for any remaining references to peerIdToAliasLink
rg "peerIdToAliasLink" "src/pages/node/peers.tsx"
# Test 2: Search for other potential alias-related patterns to ensure consistency
ast-grep --pattern 'aliases[$_]' "src/pages/node/peers.tsx"
Length of output: 222
Script:
#!/bin/bash
# Search for any other alias-related patterns in the component
rg -A 2 "alias" "src/pages/node/peers.tsx"
# Search for any state or selector imports related to aliases
rg "import.*alias" "src/pages/node/peers.tsx"
# Look for any Redux-related alias patterns
ast-grep --pattern 'useSelector($$$alias$$$)'
Length of output: 607
src/store/slices/node/initialState.ts (3)
205-208: LGTM: Well-structured ticket price state.
The new ticketPrice property follows Redux best practices for managing async data with proper typing and loading state.
347-350: LGTM: Proper initialization of new state properties.
The initial values for ticketPrice and aliasToPeerId are correctly initialized with safe default values, maintaining consistency with their type definitions.
Also applies to: 357-357
134-135: Verify the migration strategy for the links.aliasToPeerId change.
The inversion of the mapping direction from peerIdToAlias to aliasToPeerId is a breaking change. While this change makes the data structure more intuitive, we should ensure all components are updated and existing data is properly migrated.
Let's verify the components using this mapping:
✅ Verification successful
Migration of peerIdToAlias to aliasToPeerId appears complete and consistent
Based on the codebase analysis:
- The change is only referenced in
initialState.tsandactionsAsync.ts - The migration is properly handled in
actionsAsync.tswhere the new mapping is populated correctly from the aliases data - No other components or selectors are directly accessing this state property, suggesting the mapping is encapsulated within the node slice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old peerIdToAlias property
rg -l 'peerIdToAlias'
# Search for components already updated to use aliasToPeerId
rg -l 'aliasToPeerId'
Length of output: 119
Script:
#!/bin/bash
# Let's examine the actual usage in actionsAsync.ts to understand the migration
rg -A 5 -B 5 'aliasToPeerId|peerIdToAlias' src/store/slices/node/actionsAsync.ts
# Also check for any selectors or reducers that might be handling this state
fd -e ts -e js selector reducer --exec rg -l 'aliasToPeerId|peerIdToAlias' {}
Length of output: 620
Script:
#!/bin/bash
# Let's search for selector and reducer files in a more robust way
fd -e ts -e js . | grep -E '(selector|reducer)' | xargs rg -l 'aliasToPeerId|peerIdToAlias'
# Also check for any components or hooks that might be using this state
rg -C 5 'useSelector.*links'
Length of output: 200
src/components/Modal/node/WithdrawModal.tsx (4)
112-112: LGTM: Good practice to reset transaction state
Resetting the transaction hash before starting a new withdrawal ensures a clean state and prevents confusion if the new transaction fails.
Line range hint 127-146: LGTM: Robust error handling implementation
The error handling implementation is thorough and well-structured:
- Validates current API endpoint before showing errors
- Constructs clear error messages with status and details
- Properly handles SDK API errors
🧰 Tools
🪛 eslint
[error] 120-121: Missing trailing comma.
(comma-dangle)
221-226: Verify Button component's pending state implementation
The loading state has been simplified by using the Button component's pending prop. Let's verify the implementation:
#!/bin/bash
# Check Button component implementation
echo "Checking Button component implementation:"
fd "Button.(tsx|js)" src/future-hopr-lib-components -x cat {}
# Check for other usages of pending prop
echo "Checking other usages of pending prop:"
rg "pending={.*}" -A 2 -B 2117-120: Verify API changes across the codebase
The parameter rename from 'ethereumAddress' to 'address' and addition of a 4-minute timeout warrant verification:
- Ensure all other components using the withdraw functionality are updated
- Verify if 4-minute timeout aligns with typical transaction completion times
src/pages/node/channelsOutgoing.tsx (1)
72-72: LGTM: Simplified alias lookup logic
The direct access to the aliases object is a cleaner approach that reduces complexity while maintaining the same functionality.
src/pages/node/channelsIncoming.tsx (2)
80-82: LGTM: Simplified alias lookup logic
The direct access to aliases[peerId] simplifies the code while maintaining proper null checks. The format alias (nodeAddress) provides good user context.
Line range hint 155-182: LGTM: Robust error handling implementation
The error handling is well-implemented with:
- Type checking using
sdkApiError - Detailed error messages including status and error details
- API endpoint verification to prevent stale errors
src/components/Modal/node/SendMessageModal.tsx (1)
Line range hint 190-194: LGTM! Error handling improvements.
The error message construction is now more concise and properly handles SDK API errors with their payloads.
src/components/ConnectNode/modal.tsx (1)
303-308: Consider potential race conditions with parallel dispatches.
The new getTicketPriceThunk dispatch is correctly placed within the authentication flow. However, since it's dispatched in parallel with other initialization calls, consider:
- Whether the ticket price calculation depends on data from other dispatches (e.g., configuration, metrics)
- If dependencies exist, consider chaining the calls or using Promise.all for better control
Let's verify the dependencies and implementation of the ticket price thunk:
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have clear visibility into the thunk implementation. The results show that:
- The thunk is a simple, independent operation that only requires apiToken and apiEndpoint
- It has proper error handling and state management
- There are no dependencies on other state data
- It maintains its own fetching state and data storage
The parallel dispatch implementation is correct and safe.
The getTicketPriceThunk implementation shows it's completely independent of other state data and only requires the authentication parameters. The thunk:
- Has its own fetching state management
- Properly handles API errors
- Validates the API endpoint matches before updating state
- Stores data in an isolated state slice
No changes needed as there are no race conditions or dependencies to be concerned about.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the thunk implementation and its dependencies
echo "Searching for getTicketPriceThunk implementation..."
rg "getTicketPriceThunk.*=" -A 10
echo "Checking for ticket price state usage..."
rg "ticketPrice.*state" -A 5
echo "Checking for dependencies between ticket price and other state..."
ast-grep --pattern 'getTicketPriceThunk($$$)'
Length of output: 2397
src/store/slices/node/actionsAsync.ts (3)
42-43: LGTM: Type imports are properly structured.
The new ticket price types are correctly imported from the SDK, following the established pattern.
70-70: LGTM: API import follows conventions.
The getTicketPrice API is correctly imported and follows the alphabetical ordering pattern.
1583-1583: LGTM: Export follows conventions.
The getTicketPriceThunk is correctly exported and follows the alphabetical ordering pattern.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Version Updates
@hoprnet/hopr-sdkupdated to version 2.1.9.