refactor: add useLockedInReferenda #1608
Conversation
plus using two lock shapes
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (5)
packages/extension-polkagate/src/hooks/useLockedInReferenda.ts (1)
12-18: Consider documenting the 'N/A' special case in refId.The
refIdproperty allows 'N/A' as a special case. Adding JSDoc comments would help explain when and why this special case occurs.interface Lock { classId: BN; endBlock: BN; locked: string; + /** Reference ID of the lock. 'N/A' indicates [document the special case here] */ refId: BN | 'N/A'; total: BN; }packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx (1)
Line range hint
82-89: Simplify nested ternary expressions for better readability.The status text logic using nested ternaries could be made more maintainable.
- <Typography fontSize='12px' fontWeight={500} textAlign='right'> - {api && unlockableAmount && !unlockableAmount.isZero() - ? `${api.createType('Balance', unlockableAmount).toHuman()} can be unlocked` - : delegatedBalance && !delegatedBalance.isZero() - ? t('Locked as delegated') - : timeToUnlock === null ? '' : timeToUnlock - } - </Typography> + <Typography fontSize='12px' fontWeight={500} textAlign='right'> + {getStatusText()} + </Typography> // Add this function above the component or in a separate utils file +const getStatusText = () => { + if (api && unlockableAmount && !unlockableAmount.isZero()) { + return `${api.createType('Balance', unlockableAmount).toHuman()} can be unlocked`; + } + + if (delegatedBalance && !delegatedBalance.isZero()) { + return t('Locked as delegated'); + } + + return timeToUnlock === null ? '' : timeToUnlock; +};packages/extension-polkagate/src/hooks/useAccountLocks.ts (3)
Line range hint
156-157: Remove eslint-disable comment for floating promise.The code disables eslint warning for floating promises. This could lead to unhandled promise rejections.
Apply this fix to properly handle the promise:
- // eslint-disable-next-line @typescript-eslint/no-floating-promises - getLockClass(); + void getLockClass().catch((error) => { + console.error('Failed to get lock class:', error); + setInfo(undefined); + });
Line range hint
89-155: Consider optimizing state updates in getLockClass function.The function performs multiple async operations and state updates. Consider:
- Implementing batch queries where possible
- Memoizing expensive computations
- Adding loading states for better UX
Consider implementing the following optimizations:
- Use
combineLatestor similar RxJS operators for handling multiple observables- Implement request caching for frequently accessed data
- Add loading states to handle async operations more gracefully
Line range hint
165-186: Optimize memory usage in useMemo dependency array.The current dependency array includes the entire API object, which could cause unnecessary re-renders.
Consider extracting only the required API methods:
- }, [api, chain?.genesisHash, info, currentBlock, palletVote, notExpired]); + }, [ + api?.genesisHash.toString(), + chain?.genesisHash, + info, + currentBlock, + palletVote, + notExpired + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx (3 hunks)
- packages/extension-polkagate/src/hooks/index.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useAccountLocks.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useLockedInReferenda.ts (1 hunks)
- packages/extension-polkagate/src/popup/account/unlock/LockedInReferenda.tsx (4 hunks)
🔇 Additional comments (11)
packages/extension-polkagate/src/hooks/useLockedInReferenda.ts (3)
1-10: LGTM! Imports and headers are properly structured.The file includes appropriate copyright notice, license information, and well-organized imports.
41-43:⚠️ Potential issueAdd unlockableAmount to useMemo dependencies.
The
hasDescriptioncomputation depends onunlockableAmountbut it's missing from the dependency array.const hasDescription = useMemo(() => (unlockableAmount && !unlockableAmount.isZero()) || (delegatedBalance && !delegatedBalance.isZero()) || timeToUnlock - , [delegatedBalance, timeToUnlock, unlockableAmount]); + , [delegatedBalance, timeToUnlock, unlockableAmount]);Likely invalid or redundant comment.
32-37: Add error handling for hook failures.The hook calls multiple external hooks but doesn't handle potential failures. Consider wrapping the calls in try-catch blocks and providing fallback values.
Let's check if these hooks have error handling:
Consider implementing a custom error handling hook that can be reused across all blockchain-related hooks.
packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx (2)
8-14: LGTM! Clean import consolidation.The consolidation of multiple hooks into
useLockedInReferendareduces complexity while maintaining functionality. The addition of lock icons enhances the UI feedback.
100-103: LGTM! Clear and intuitive icon feedback.The conditional icon rendering provides clear visual feedback about the lock state, enhancing the user experience.
packages/extension-polkagate/src/hooks/useAccountLocks.ts (2)
10-10: LGTM: Type import addition improves type safety.The addition of
ITupletype import enhances type safety for tuple structures used in vote casting operations.
Line range hint
28-82: Consider enhancing error handling in getLocks function.The function uses console.error for unhandled cases, which might not be sufficient for production error handling. Consider implementing proper error reporting or recovery mechanisms.
Let's check for other similar error handling patterns:
Consider implementing a more robust error handling strategy:
- console.error(`Unable to handle ${accountVote.type}`); + const error = new Error(`Unhandled vote type: ${accountVote.type}`); + // Either throw the error or handle it through a proper error reporting mechanism + throw error;packages/extension-polkagate/src/hooks/index.ts (1)
64-64: LGTM! The export follows the established patterns.The new export for
useLockedInReferendais correctly placed alphabetically and follows the same export pattern as other hooks in the file.Let's verify that the hook implementation exists:
✅ Verification successful
Verified: The hook implementation exists and follows React hook conventions
The
useLockedInReferendahook is properly implemented as a React hook function that takes address and refreshNeeded parameters, and uses other hooks likeuseHasDelegatedanduseAccountLocksinternally.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the useLockedInReferenda hook implementation exists # Test: Check if the hook file exists and exports a default function fd -t f "useLockedInReferenda.ts" packages/extension-polkagate/src/hooks/ --exec rg -l "export default"Length of output: 166
Script:
#!/bin/bash # Let's verify the hook implementation to ensure it's a proper React hook ast-grep --pattern 'export default function useLockedInReferenda' -l typescriptLength of output: 426
Script:
#!/bin/bash # Let's try a simpler pattern to verify the hook implementation rg "function useLockedInReferenda" -A 2 packages/extension-polkagate/src/hooks/useLockedInReferenda.tsLength of output: 403
packages/extension-polkagate/src/popup/account/unlock/LockedInReferenda.tsx (3)
11-11: Importing FontAwesome iconsThe
faLockandfaUnlockAlticons are correctly imported from@fortawesome/free-solid-svg-icons.
19-19: ImportinguseLockedInReferendahookThe
useLockedInReferendahook is properly imported from the hooks directory.
39-39: Destructuring values fromuseLockedInReferendahookThe hook is invoked with
addressandrefresh, and the returned values are correctly destructured.
packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/popup/account/unlock/LockedInReferenda.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx (2)
32-32: Well-structured hook consolidation!The consolidation of multiple hooks into
useLockedInReferendaimproves code organization and maintainability. This is a good example of the custom hooks pattern in React.Consider documenting the hook's interface in a separate types file to make it more maintainable and reusable across the application.
92-92: Consider extracting the icon selection logic.While the conditional icon rendering works well, consider extracting it to a constant for improved readability:
+ const lockIcon = unlockableAmount && !unlockableAmount.isZero() ? faUnlockAlt : faLock; <FontAwesomeIcon color={isDisable ? theme.palette.action.disabledBackground : theme.palette.secondary.light} - icon={ unlockableAmount && !unlockableAmount.isZero() ? faUnlockAlt : faLock} + icon={lockIcon} shake={shake} style={{ height: '25px' }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx (3 hunks)
- packages/extension-polkagate/src/hooks/useAccountLocks.ts (3 hunks)
- packages/extension-polkagate/src/hooks/useCurrentBlockNumber.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useHasDelegated.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useLockedInReferenda.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useTimeToUnlock.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/extension-polkagate/src/hooks/useAccountLocks.ts
- packages/extension-polkagate/src/hooks/useLockedInReferenda.ts
🔇 Additional comments (7)
packages/extension-polkagate/src/hooks/useHasDelegated.ts (3)
17-24: Well-structured state management.The addition of
fetchedForstate effectively prevents redundant API calls when the address hasn't changed, improving performance.
26-31: Clean refresh handling implementation.The refresh logic properly resets all relevant state variables, ensuring a fresh fetch cycle.
66-69: Clean implementation following React hooks best practices.The effect cleanup and return value handling are well-implemented, following React hooks patterns.
packages/extension-polkagate/src/fullscreen/accountDetails/components/LockedInReferendaFS.tsx (2)
8-14: LGTM! Import changes align with the refactoring.The imports have been properly updated to support the new consolidated hook pattern and animation functionality.
33-33: Great improvement in animation handling!The use of
useAnimateOncehook addresses the previous review comment about animation logic extraction. This is a cleaner and more reusable approach.packages/extension-polkagate/src/hooks/useTimeToUnlock.tsx (2)
14-14: LGTM: Hook signature updated correctly.The addition of
delegatedBalanceparameter is well-typed and maintains backward compatibility with nullable/undefined values.Also applies to: 16-16
54-55: LGTM: Consistent state initialization.The initialization of
unlockableAmountandtotalLockedtoBN_ZEROwhenreferendaLocksis null, and toundefinedwhenreferendaLocksis undefined, maintains proper state consistency.Also applies to: 63-64
plus using two lock shapes
Summary by CodeRabbit
New Features
useLockedInReferenda, to manage and provide information about locked referenda.LockedInReferendacomponent to utilize the new hook, streamlining state management and enhancing visual feedback with conditional icons.Bug Fixes
useCurrentBlockNumberanduseHasDelegatedhooks to prevent unhandled promise rejections.Documentation