feat: support NFTs and Uniques #1564
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces comprehensive support for Non-Fungible Tokens (NFTs) across various components of the application. Key updates include the addition of new components for displaying NFTs, such as Changes
Assessment against linked issues
Possibly related PRs
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: 21
🧹 Outside diff range and nitpick comments (34)
packages/extension-polkagate/src/components/SVG/NFT.tsx (2)
4-4: Consider removing the ESLint disable comment.The ESLint disable comment for
react/jsx-max-props-per-linesuggests that there might be lines with too many props. Instead of disabling the rule, consider formatting the code to comply with the rule. This will improve code readability and consistency.
14-21: LGTM: Well-implemented NFTIcon component.The component is correctly implemented as a functional component with default prop values. The SVG structure looks appropriate for an icon.
Consider adding a
role="img"attribute to the<svg>element and anaria-labelfor better accessibility.You can improve accessibility by adding these attributes to the SVG:
- <svg fill='none' height={height} viewBox='0 0 42 36' width={width} xmlns='http://www.w3.org/2000/svg'> + <svg fill='none' height={height} viewBox='0 0 42 36' width={width} xmlns='http://www.w3.org/2000/svg' role="img" aria-label="NFT Icon">packages/extension-polkagate/src/fullscreen/nft/components/ItemFullScreenModal.tsx (1)
1-10: Consider addressing the eslint warning instead of disabling it.The eslint disable comment on line 4 suggests that there might be an opportunity to improve code formatting. Consider addressing the underlying issue that triggers the
react/jsx-max-props-per-linewarning instead of disabling it. This could involve breaking down props into multiple lines for better readability.The imports look good and appropriate for the component's functionality. The custom type
FullscreenNftModalPropsis correctly imported from a local file, which is a good practice for type management.packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts (1)
38-40: LGTM: Retry-related constants are well-defined.The retry-related constants are well-defined and seem appropriate for the use case. Setting
MAX_RETRY_ATTEMPTSto the length ofIPFS_GATEWAYSensures all gateways are tried. The backoff times provide a good balance between quick retries and avoiding overwhelming the system.Consider adding a brief comment explaining the purpose of these constants and how they will be used in the retry mechanism. For example:
// Retry mechanism constants // MAX_RETRY_ATTEMPTS ensures we try all available IPFS gateways export const MAX_RETRY_ATTEMPTS = IPFS_GATEWAYS.length; // Initial and maximum backoff times for exponential backoff strategy export const INITIAL_BACKOFF_TIME = 1000; // 1 second export const MAX_BACKOFF_TIME = 10000; // 10 secondspackages/extension-polkagate/src/fullscreen/nft/components/ItemsList.tsx (2)
4-4: Consider removing the ESLint disable commentThe ESLint disable comment for
react/jsx-max-props-per-linesuggests that there might be instances of excessive prop spreading in the component. Instead of disabling the rule, consider refactoring the code to comply with it. This can improve readability and maintainability.You could:
- Break down components with many props into smaller, more focused components.
- Use object destructuring to pass multiple related props as a single object.
- If the rule is too restrictive for your team's preferences, consider adjusting it in the ESLint configuration instead of disabling it in the file.
29-50: Item rendering and empty states are well-implementedThe logic for rendering items and handling empty states is clear and comprehensive. The use of separate messages for different empty states (no NFTs owned vs. no items matching filter) improves the user experience.
Consider adding
aria-liveattributes to the empty state messages to improve accessibility:- <Typography fontSize='16px' fontWeight={400}> + <Typography fontSize='16px' fontWeight={400} aria-live="polite"> {t('You do not own any NFTs/Uniques')}! </Typography>This change would ensure that screen readers announce the message when it appears, improving the experience for users with visual impairments.
packages/extension-polkagate/src/components/InputFilter.tsx (1)
45-45: LGTM:disabledattribute correctly applied to Input component.The
disabledattribute has been properly added to theInputcomponent, allowing the input field to be disabled based on the prop value. This implementation is consistent with React and HTML standards.For consistency with other boolean props in this component (like
autoFocus), consider using the shorthand syntax when the prop name matches the attribute name:- disabled={disabled} + disabled={disabled || undefined}This change ensures that the
disabledattribute is only added when the prop istrue, maintaining consistency with React's handling of boolean attributes.packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (4)
15-19: LGTM: Well-defined FilterSectionProps interface with a minor suggestionThe
FilterSectionPropsinterface is well-structured for a filter component handling both unique items and NFTs. The use of React's state management types is appropriate.For consistency, consider using
ItemInformation[]for thesetItemsToShowtype instead ofItemInformation[] | null | undefined. This would align with the types ofmyUniquesDetailsandmyNFTsDetails.- setItemsToShow: React.Dispatch<React.SetStateAction<ItemInformation[] | null | undefined>>; + setItemsToShow: React.Dispatch<React.SetStateAction<ItemInformation[] | undefined>>;
30-37: LGTM: Comprehensive ItemMetadata interface with a suggestionThe
ItemMetadatainterface is well-structured and covers common metadata fields for NFTs. The use of optional properties allows for flexibility in the metadata structure.Consider adding a generic type parameter to allow for custom attribute types. This would make the interface more flexible for different NFT standards or custom implementations:
export interface ItemMetadata<T = Attribute> { // ... other properties attributes: T[] | undefined; // ... other properties }This change would allow users of the interface to define custom attribute types while maintaining compatibility with the default
Attributetype.
46-51: LGTM: Comprehensive ItemAvatarProp interface with a minor suggestionThe
ItemAvatarPropinterface is well-defined, covering necessary properties for an avatar component with flexible sizing options and user interaction.Consider using a more specific type for the
imageproperty instead ofstring | null | undefined. For example:image: string | null;This change would eliminate the
undefinedcase, making it clearer that the image should always be either a valid string or explicitly null.
53-57: LGTM: Well-structured FullscreenNftModalProps interface with a suggestionThe
FullscreenNftModalPropsinterface is well-defined, covering necessary properties for a modal component displaying an NFT image.Consider adding an optional
titleproperty to the interface. This would allow for a more descriptive modal header, enhancing the user experience:export interface FullscreenNftModalProps { image: string | null | undefined; onClose: () => void; open: boolean; title?: string; }This addition would make the component more flexible and reusable across different contexts.
packages/extension-polkagate/src/fullscreen/nft/components/ItemAvatar.tsx (4)
4-4: Consider removing the eslint disable comment.The presence of this comment suggests that some lines might have too many props, potentially affecting readability. Instead of disabling the rule, consider breaking down complex components or spreading props to improve code clarity.
20-21: LGTM: Grid container setup with theme-aware styling.The Grid container is well-structured with appropriate styling. The use of theme-aware background color is a good practice.
For consistency, consider using template literals for the borderRadius value:
-sx={{ bgcolor: isDarkMode ? ALT_NFT_BGCOLOR_DARK : ALT_NFT_BGCOLOR_LIGHT, borderRadius: '10px 10px 5px 5px', height, overflow: 'hidden', width }} +sx={{ bgcolor: isDarkMode ? ALT_NFT_BGCOLOR_DARK : ALT_NFT_BGCOLOR_LIGHT, borderRadius: `10px 10px 5px 5px`, height, overflow: 'hidden', width }}
22-50: LGTM: Image rendering with optional fullscreen functionality.The conditional rendering of the Avatar and fullscreen button is well-implemented. The styling and positioning of the fullscreen button are appropriate.
For improved accessibility, consider adding an aria-label to the fullscreen button:
<IconButton onClick={onFullscreen} + aria-label="View fullscreen" sx={{ // ... (existing styles) }} > <OpenInFullIcon /> </IconButton>
51-57: LGTM: Fallback NFTIcon rendering.The fallback logic for rendering an NFTIcon when no image is provided is well-implemented. Using the theme's primary background color for the icon ensures visual consistency.
For improved readability, consider using object destructuring for the theme color:
-<NFTIcon - color={theme.palette.backgroundFL.primary} - height={70} - width={70} -/> +const { backgroundFL: { primary: iconColor } } = theme.palette; +<NFTIcon + color={iconColor} + height={70} + width={70} +/>packages/extension-polkagate/src/fullscreen/nft/components/Item.tsx (1)
19-24: Consider simplifying thedisplayNftlogic for better readability.The current implementation of
displayNftis correct but could be more explicit:const displayNft = useMemo(() => (itemDetail || (itemDetail === null && itemInformation?.data)), [itemInformation?.data, itemDetail]);Consider refactoring it to make the logic more clear:
const displayNft = useMemo(() => { return itemDetail !== undefined || (itemDetail === null && !!itemInformation?.data); }, [itemInformation?.data, itemDetail]);This change makes the condition more explicit and easier to understand at a glance.
packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (1)
1-17: Consider addressing the eslint warning instead of disabling it.The file structure and imports look good. However, there's an eslint disable comment for
react/jsx-max-props-per-line. It's generally better to address the underlying issue rather than disabling the rule. Consider formatting the code to comply with the rule or adjusting your eslint configuration if this is a project-wide preference.packages/extension-polkagate/src/fullscreen/governance/FullScreenHeader.tsx (2)
21-21: LGTM! Consider improving readability.The addition of 'nft' to the
pageproperty is consistent with the PR objectives and correctly implements support for NFT routing.For improved readability, consider using a type alias for the
pageproperty. This would make it easier to maintain and extend in the future. For example:type PageType = 'governance' | 'manageIdentity' | 'send' | 'stake' | 'socialRecovery' | 'accountDetails' | 'proxyManagement' | 'nft'; interface Props { page?: PageType; // ... other properties }
68-69: LGTM! Consider improving consistency.The addition of the 'nft' case in the
onAccountChangefunction correctly implements the routing for the new NFT page, aligning with the PR objectives.For consistency with other cases in the switch statement, consider removing the trailing slash from the NFT route. This would make it consistent with cases like 'send'. For example:
case 'nft': return onAction(`/nft/${selectedAddress}`);packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FullScreenAccountMenu.tsx (2)
67-69: LGTM: New callback for NFT Album navigation.The
onNFTAlbumcallback is well-implemented and consistent with other navigation functions. It correctly checks for the address before navigation.Consider adding a comment explaining the purpose of this function for better code documentation:
// Navigate to the NFT Album page for the current address const onNFTAlbum = useCallback(() => { address && onAction(`/nft/${address}`); }, [address, onAction]);
121-136: LGTM: New MenuItem for NFT Album.The new MenuItem for the NFT Album is well-implemented and consistent with other menu items. It correctly uses the NFTIcon, applies conditional styling, and handles disabling based on chain support.
For consistency with other menu items, consider moving the
disabledprop to a new line:<MenuItem disabled={isDisable(NFT_CHAINS)} iconComponent={ <NFTIcon color={ isDisable(NFT_CHAINS) ? theme.palette.text.disabled : theme.palette.text.primary } height={24} width={24} /> } onClick={onNFTAlbum} text={t('NFT Album')} withHoverEffect />packages/extension-polkagate/src/partials/AccountMenu.tsx (2)
80-83: LGTM: onNFTAlbum callback addedThe new
onNFTAlbumcallback function is consistent with the PR objectives, opening a window to the NFT album associated with the user's address.Consider adding error handling to provide feedback to the user if the window fails to open. For example:
const onNFTAlbum = useCallback(() => { if (address) { windowOpen(`/nft/${address}`) .catch((error) => { console.error('Failed to open NFT album:', error); // TODO: Add user-facing error notification }); } }, [address]);
140-155: LGTM: NFT Album MenuItem addedThe new MenuItem for the NFT Album is consistent with the PR objectives, providing a user interface element to access the NFT album. It correctly uses the imported NFTIcon and is conditionally disabled based on the
isDisabledfunction that checks againstNFT_CHAINS.For consistency with other MenuItems, consider extracting the icon color logic into a variable:
const nftIconColor = isDisabled(NFT_CHAINS) ? theme.palette.text.disabled : theme.palette.text.primary; // Then in the NFTIcon component: color={nftIconColor}This approach is used in other MenuItems and would improve readability and maintainability.
packages/extension-polkagate/src/util/constants.tsx (1)
111-114: LGTM! Consider adding a comment for clarity.The addition of the
NFT_CHAINSconstant is appropriate and aligns well with the PR objectives of adding NFT support. It correctly uses the existing genesis hash constants for Statemine and Statemint, which are known to support NFT functionality.Consider adding a brief comment above the constant to explain its purpose, for example:
// Chains that support NFT functionality export const NFT_CHAINS = [ STATEMINE_GENESIS_HASH, STATEMINT_GENESIS_HASH ];This would enhance code readability and make it easier for other developers to understand the constant's role in the application.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (1)
Line range hint
1-265: Summary of changes and alignment with PR objectivesThe changes in this file primarily focus on removing the
ActionContextdependency and disabling the settings functionality. While these modifications streamline the component, they don't directly contribute to the PR's main objective of adding NFT support.Consider the following suggestions to improve the alignment with the PR objectives and overall code quality:
- If the removal of settings functionality is intentional, document this decision in the code or PR description to provide context for future developers.
- Evaluate whether this component needs any modifications to support NFT display or interactions. If so, consider adding TODO comments or placeholder functions for future NFT-related features.
- Update the component's props or internal logic to accommodate potential NFT data that might be passed down from parent components.
- If the changes in this file are part of a larger refactoring effort to prepare for NFT integration, consider adding a comment explaining this context.
These suggestions will help clarify the purpose of the current changes and set the stage for future NFT-related enhancements.
packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (1)
257-272: LGTM: NFT Album TaskButton added correctly.The NFT Album TaskButton is well-implemented and consistent with other TaskButtons. It correctly uses the
nftDisablestate and theonNFTAlbumcallback.For consistency with other TaskButtons, consider adding a
showprop set totrue. While it doesn't affect functionality, it maintains consistency across all TaskButtons:<TaskButton disabled={nftDisable} icon={ <NFTIcon color={ nftDisable ? theme.palette.text.disabled : theme.palette.text.primary} height={24} width={24} /> } onClick={onNFTAlbum} secondaryIconType='page' text={t('NFT Album')} + show={true} />packages/extension-ui/src/Popup/index.tsx (1)
342-342: LGTM: NFT route added correctly with a suggestionThe new route for the NFTAlbum component is correctly implemented, following the existing pattern for route definitions. The use of the ':address' parameter in the path is appropriate for displaying NFTs specific to a user's address.
Consider adding a comment above this route to indicate that it's for the NFT feature, similar to comments present for other feature routes. This would enhance code readability and maintainability.
+ {/* NFT Album */} <Route path='/nft/:address'>{wrapWithErrorBoundary(<NFTAlbum />, 'nft-album')}</Route>packages/extension/public/locales/en/translation.json (1)
1406-1424: New NFT-related translations look good, with minor suggestions for improvement.The new translations for NFT and Unique item functionality are well-structured and consistent with the existing translations. However, I have a few minor suggestions for improvement:
- Line 1417: Consider removing the trailing space in "🔍 Search in nfts " for consistency.
- Line 1418: "Don't have data" could be rephrased to "No data available" for better clarity.
- Line 1424: Consider breaking the long sentence into two for better readability: "On the NFT / Unique Album page, you can view all of your created or owned NFT/unique items."
Consider applying these minor changes to enhance consistency and clarity:
- "🔍 Search in nfts ": "", + "🔍 Search in nfts": "", - "Don't have data": "", + "No data available": "", - "On NFT / Unique Album page you can watch all of your created or owned NFT/unique items.": "" + "On the NFT / Unique Album page, you can view all of your created or owned NFT/unique items.": ""packages/extension-polkagate/src/fullscreen/nft/components/FilterItems.tsx (3)
95-98: Remove redundant dependency inuseEffectIncluding both
filteredItemsandfilteredItems?.lengthis unnecessary. Changes tofilteredItemswill cover changes in its length.useEffect(() => { setItemsToShow(filteredItems); - }, [filteredItems, filteredItems?.length, setItemsToShow]); + }, [filteredItems, setItemsToShow]);
103-103: Enhance internationalization for item countConcatenating strings can lead to translation issues. Use interpolation in your translation strings.
<Typography> - {t('Items')}{` (${filteredItems.length})`} + {t('Items ({{count}})', { count: filteredItems.length })} </Typography>
115-115: Correct capitalization in placeholder textStandardize the term "NFTs" by using uppercase letters for consistency.
placeholder={t('🔍 Search in nfts ')} + // Update to: placeholder={t('🔍 Search in NFTs')}packages/extension-polkagate/src/fullscreen/nft/index.tsx (3)
70-72: Enhance error handling for fetching NFT dataCurrently, errors during the fetching of NFTs and uniques are logged to the console. Consider implementing user-friendly error handling to inform the user if data fetching fails. This could involve updating the UI to display an error message or a notification to enhance user experience.
82-83: Improve error handling for metadata fetchingWhen fetching item metadata, errors are caught and logged to the console. To provide better user feedback, consider handling these errors by updating the UI or notifying the user if metadata cannot be retrieved.
129-135: Ensure consistent button stylingThe
PButtoncomponent has inline styling that sets its width to '280px'. To maintain consistency and responsiveness across the application, consider using theme spacing or CSS classes instead of fixed widths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
- packages/extension-base/src/defaults.ts (1 hunks)
- packages/extension-polkagate/src/components/InputFilter.tsx (2 hunks)
- packages/extension-polkagate/src/components/SVG/NFT.tsx (1 hunks)
- packages/extension-polkagate/src/components/index.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountSetting.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (4 hunks)
- packages/extension-polkagate/src/fullscreen/governance/FullScreenHeader.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (2 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FullScreenAccountMenu.tsx (4 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/FilterItems.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/Item.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/ItemAvatar.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/ItemFullScreenModal.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/ItemsList.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/index.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/util.ts (1 hunks)
- packages/extension-polkagate/src/partials/AccountMenu.tsx (3 hunks)
- packages/extension-polkagate/src/themes/dark.ts (1 hunks)
- packages/extension-polkagate/src/themes/light.ts (1 hunks)
- packages/extension-polkagate/src/util/constants.tsx (1 hunks)
- packages/extension-ui/src/Popup/index.tsx (2 hunks)
- packages/extension/public/locales/en/translation.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountSetting.tsx
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/fullscreen/nft/utils/util.ts
[error] 221-221: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (47)
packages/extension-polkagate/src/components/SVG/NFT.tsx (3)
8-12: LGTM: Well-defined Props interface.The
Propsinterface is correctly defined with appropriate types for each property. Making all props optional allows for flexible usage of the component.
23-23: LGTM: Correct component export.The component is correctly exported as the default export, which is appropriate for a single component in a file.
1-23: Summary: NFTIcon component aligns with PR objectives.This new component introduces an NFT icon, which directly supports the PR objective of adding NFT functionality to the PolkaGate extension. The implementation is clean, follows React best practices, and provides flexibility through optional props with default values.
To fully meet the PR objectives, ensure that this NFTIcon component is properly integrated into the new NFT-related pages and buttons mentioned in the PR description.
packages/extension-base/src/defaults.ts (1)
15-15: LGTM: Addition of NFT path aligns with PR objectives.The addition of '/nft/' to the
START_WITH_PATHarray is consistent with the PR's objective of adding NFT support to the PolkaGate extension. This change allows the application to recognize NFT-related paths, which is necessary for implementing the new NFT functionality.A few points to consider:
- The new path follows the same format as existing paths in the array.
- Its position in the array (second to last) seems appropriate and doesn't disrupt the existing order.
- This change is likely part of a larger set of changes across the codebase to fully implement NFT support.
To ensure this change is properly utilized, let's verify its usage across the codebase:
This script will help us verify that the new path is being properly utilized in the application's routing or navigation logic.
✅ Verification successful
: The addition of '/nft/' to the
START_WITH_PATHarray is correctly implemented and utilized within the routing logic. All usages have been appropriately updated to support the new NFT functionality, aligning with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new NFT path in routing or navigation logic # Test 1: Search for direct usage of '/nft/' in routing logic echo "Searching for direct usage of '/nft/' in routing logic:" rg --type typescript --type javascript "'/nft/'" -C 3 # Test 2: Search for dynamic usage of START_WITH_PATH in routing logic echo "Searching for dynamic usage of START_WITH_PATH in routing logic:" rg --type typescript --type javascript "START_WITH_PATH" -C 3Length of output: 459
Script:
#!/bin/bash # Description: Re-attempt searching for the new NFT path in routing or navigation logic using file extensions # Test 1: Search for direct usage of '/nft/' in routing logic within .ts and .js files echo "Searching for direct usage of '/nft/' in routing logic:" rg -e '/nft/' --type-add 'typescript:*.ts' --type-add 'javascript:*.js' -C 3 # Test 2: Search for dynamic usage of START_WITH_PATH in routing logic within .ts and .js files echo "Searching for dynamic usage of START_WITH_PATH in routing logic:" rg -e 'START_WITH_PATH' --type-add 'typescript:*.ts' --type-add 'javascript:*.js' -C 3Length of output: 8403
packages/extension-polkagate/src/fullscreen/nft/components/ItemFullScreenModal.tsx (2)
12-12: LGTM: Component declaration follows best practices.The component declaration is well-structured:
- It's exported as a default function, which is appropriate for a single-component file.
- Props are correctly destructured and typed using the imported
FullscreenNftModalProps.- The return type
React.ReactElementis explicitly defined, enhancing type safety.This approach promotes clean and maintainable code.
1-30: Overall assessment: Good implementation with room for minor improvements.The
ItemFullscreenModalcomponent is well-structured and fulfills its purpose of displaying NFT images in a fullscreen modal. It correctly uses Material-UI components and React hooks. The main areas for improvement are:
- Code formatting: Address the eslint warning instead of disabling it.
- Style management: Extract inline styles for better maintainability.
- Image handling: Improve the fallback mechanism for the image source.
- Accessibility: Add customizable alt text for the image.
These changes will enhance the component's maintainability, robustness, and accessibility. Great job on implementing this new feature for NFT support!
packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts (1)
1-2: LGTM: Copyright and license information is correct.The copyright notice and license specification are properly included and formatted.
packages/extension-polkagate/src/fullscreen/nft/components/ItemsList.tsx (3)
15-16: Component definition looks goodThe component is well-defined using TypeScript for prop typing, which enhances code reliability. The use of the
useTranslationhook for internationalization is a good practice, aligning with the project's requirements.
20-28: Loading state is well-handledThe use of a custom Progress component for the loading state is a good practice. It provides a clear indication to the user that the application is searching for NFTs/Uniques.
1-53: Summary: ItemsList component successfully implements NFT display functionalityThis new component aligns well with the PR objectives of adding support for NFTs in the PolkaGate extension. It provides a flexible and user-friendly interface for displaying NFT items, with appropriate handling of loading and empty states.
Key strengths:
- Comprehensive conditional rendering for different item states.
- Use of TypeScript for improved type safety.
- Internationalization support through the
useTranslationhook.Areas for potential improvement:
- More flexible container height for better responsiveness.
- Enhanced accessibility for empty state messages.
- Removal of the ESLint disable comment in favor of code refactoring.
Overall, this component is a solid implementation that successfully addresses the requirements outlined in the PR objectives.
packages/extension-polkagate/src/themes/dark.ts (1)
18-18:⚠️ Potential issueUpdate
backgroundFLproperty inPaletteinterfaceThe change from
backgroundFL?: Palette['primary']tobackgroundFL: TypeTextin thePaletteinterface has the following implications:
backgroundFLis now a required property, which might affect existing code that doesn't provide this property.- The type change to
TypeTextprovides more specific typing for the background.To maintain consistency, consider updating the
PaletteOptionsinterface as well:interface PaletteOptions { // ... backgroundFL?: TypeText; }To ensure this change doesn't break existing code, please run the following script to check for any usage of
Palettethat might be affected:This will help identify any potential issues caused by the interface change.
packages/extension-polkagate/src/components/InputFilter.tsx (3)
20-20: LGTM: Addition ofdisabledprop enhances component flexibility.The new
disabledprop in thePropsinterface is a valuable addition. It follows React conventions and allows for more versatile usage of theInputFiltercomponent in scenarios where disabling the input is required.
24-24: LGTM: Function signature updated to includedisabledprop.The
InputFilterfunction signature has been correctly updated to include the newdisabledparameter. This change aligns with thePropsinterface update and allows the component to utilize the new prop internally.
Line range hint
20-45: Summary: Successful implementation ofdisabledprop in InputFilter component.The changes to the
InputFiltercomponent successfully implement thedisabledprop, enhancing its flexibility and reusability. This addition aligns well with the PR objectives of improving the PolkaGate extension's functionality. The implementation is consistent across the component's interface, function signature, and JSX, following React best practices.These changes contribute to a more robust user interface, allowing for better control over input fields in various contexts, which could be particularly useful when dealing with NFT-related functionality as mentioned in the PR objectives.
packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (8)
6-13: LGTM: Well-structured ItemInformation interfaceThe
ItemInformationinterface is well-defined and includes essential properties for NFT identification and ownership. The use of optional properties allows for flexibility, while the boolean flags provide clear information about the item's nature and ownership status.
21-26: LGTM: Comprehensive CheckboxButtonProps interfaceThe
CheckboxButtonPropsinterface is well-defined and includes all necessary properties for a checkbox button component. The proper typing of theonChangeevent handler and the inclusion of thedisabledproperty demonstrate good attention to detail and accessibility considerations.
39-39: LGTM: Flexible ItemsDetail type definitionThe
ItemsDetailtype is well-defined as a record type, allowing for a flexible structure to store item details. The inclusion ofnull | undefinedas possible values appropriately handles cases where item details might not be available.
41-44: LGTM: Well-structured ItemProps interfaceThe
ItemPropsinterface is concise and well-structured, providing the necessary properties for a component that displays an individual NFT item. The use of previously defined types (ItemInformationandItemsDetail) ensures consistency throughout the codebase.
59-64: LGTM: Comprehensive DetailsProp interfaceThe
DetailsPropinterface is well-structured, providing all necessary properties for displaying detailed information about an NFT. The use of previously defined types ensures consistency, and the inclusion of visibility control properties (showandsetShowDetail) allows for proper state management.
66-70: LGTM: Well-defined DetailProp interfaceThe
DetailPropinterface is concise and well-structured, providing the necessary properties for displaying individual detail items. The optionalinlineproperty adds flexibility for different layout requirements.
72-75: LGTM: Well-structured ItemsListProps interfaceThe
ItemsListPropsinterface is well-defined, providing the necessary properties for a component that lists multiple NFTs. The use of previously defined types ensures consistency, and the allowance for null or undefineditemsappropriately handles various data states.
1-75: Overall: Well-structured and comprehensive type definitions for NFT handlingThis file provides a robust set of TypeScript interfaces and types for managing NFTs within the application. The definitions cover various aspects of NFT handling, from basic item information to detailed metadata and UI component props. The use of consistent naming conventions and the logical organization of related interfaces contribute to the file's readability and maintainability.
The minor suggestions provided in the review comments, if implemented, would further enhance the flexibility and type safety of these definitions. Overall, this file serves as a solid foundation for type-safe NFT-related development in the project.
packages/extension-polkagate/src/fullscreen/nft/components/ItemAvatar.tsx (2)
15-18: LGTM: Component signature and initial setup.The component signature is well-defined with appropriate default values for
heightandwidth. The use ofuseThemehook is a good practice for applying theme-specific styles.
1-60: Great implementation of the ItemAvatar component for NFT support.This component aligns well with the PR objectives of adding support for NFTs. It provides a flexible and visually consistent way to display NFT avatars in both full-screen and extension modes. The component handles image presence gracefully, provides optional fullscreen functionality, and is responsive to the current theme.
Key strengths:
- Proper use of Material-UI components for layout and styling.
- Conditional rendering based on prop values.
- Theme-aware styling for visual consistency.
- Fallback to a custom NFTIcon when no image is provided.
The component contributes significantly to the goal of enhancing the PolkaGate extension with NFT support, as outlined in the PR objectives and linked issue #1537.
packages/extension-polkagate/src/fullscreen/nft/components/Item.tsx (3)
1-16: LGTM: Imports and component declaration are well-structured.The import statements and component declaration follow React and TypeScript best practices. The use of type definitions for props (
ItemProps) enhances code reliability and maintainability.
69-76: LGTM: Details component rendering is well-implemented.The conditional rendering of the Details component and the passing of necessary props, including the state setter
setShowDetail, demonstrate good React practices. This implementation ensures that the detail view is only shown when appropriate data is available.
1-79: Overall, the Item component is well-implemented with room for minor improvements.The component successfully fulfills the PR objective of adding support for NFTs and Unique items. It demonstrates good use of React hooks, effective state management, and proper handling of various data states. The component is responsive to user interactions and integrates well with the Material-UI library.
To further enhance the component:
- Consider the suggested accessibility improvement for the clickable Grid container.
- Implement the proposed simplification of the
displayNftlogic for better readability.These minor adjustments will improve the overall quality and maintainability of the component.
packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (2)
18-31: LGTM: Well-structured and reusableDetailcomponent.The
Detailcomponent is well-implemented. It's flexible (supporting both inline and block layouts), uses Material-UI components effectively, and incorporates ReactMarkdown for rich text rendering. This approach enhances reusability and maintains consistency in displaying various types of details.
1-100: Summary: Well-implemented NFT and Unique item details component with minor suggestions for improvement.This new file successfully introduces components for displaying NFT and Unique item details, meeting the objectives outlined in the PR description and linked issue. The implementation is generally well-structured, makes good use of Material-UI, and incorporates important features like internationalization and responsive design.
Key strengths:
- Comprehensive display of NFT/Unique item details
- Support for both modal and fullscreen views
- Use of ReactMarkdown for rich text rendering
- Proper use of translation for internationalization
Suggestions for improvement:
- Address the eslint warning instead of disabling it
- Consider extracting the modal content into a separate component
- Improve the scrollable container's flexibility
- Enhance the visual distinction between NFTs and Unique items
These improvements would further enhance the component's maintainability, accessibility, and user experience. Overall, this is a solid addition to the PolkaGate extension's functionality.
packages/extension-polkagate/src/components/index.ts (1)
85-85: LGTM! Verify usage of the new NFTIcon component.The addition of the
NFTIconexport is consistent with the existing pattern and aligns with the PR objectives of adding NFT support. This change makes the new NFT icon available for use throughout the application.Let's verify the usage of this new component in the codebase:
This script will help us confirm that the new
NFTIconcomponent is being imported and used in other parts of the application, ensuring that this addition is meaningful and contributes to the NFT support feature.✅ Verification successful
Verified: NFTIcon is correctly exported and utilized across the codebase.
The
NFTIconexport has been successfully integrated and is actively used in multiple components, ensuring that this addition effectively supports the NFT functionality as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly added NFTIcon component # Test: Search for imports of NFTIcon echo "Searching for imports of NFTIcon:" rg --type typescript --type tsx "import.*NFTIcon.*from.*components" -g '!index.ts' # Test: Search for usage of NFTIcon component echo "Searching for usage of NFTIcon component:" rg --type typescript --type tsx "<NFTIcon" -g '!index.ts'Length of output: 384
Script:
#!/bin/bash # Description: Verify the usage of the newly added NFTIcon component with corrected file type specifications # Test: Search for imports of NFTIcon echo "Searching for imports of NFTIcon:" rg --type ts --type tsx "import.*NFTIcon.*from.*components" -g '!index.ts' # Test: Search for usage of NFTIcon component echo "Searching for usage of NFTIcon component:" rg --type ts --type tsx "<NFTIcon" -g '!index.ts'Length of output: 354
Script:
#!/bin/bash # Description: Verify the usage of the newly added NFTIcon component using file extension patterns # Test: Search for imports of NFTIcon echo "Searching for imports of NFTIcon:" rg "import.*NFTIcon.*from.*components" -g '*.ts' -g '*.tsx' -g '!index.ts' # Test: Search for usage of NFTIcon component echo "Searching for usage of NFTIcon component:" rg "<NFTIcon" -g '*.ts' -g '*.tsx' -g '!index.ts'Length of output: 1634
packages/extension-polkagate/src/fullscreen/governance/FullScreenHeader.tsx (1)
21-21: Summary: NFT support successfully integratedThe changes to
FullScreenHeader.tsxeffectively implement NFT support as per the PR objectives. The modifications are minimal, focused, and well-integrated into the existing code structure. These changes enable the application to handle NFT-related routing and user interactions consistently with other features.Also applies to: 68-69
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FullScreenAccountMenu.tsx (2)
11-11: LGTM: New imports for NFT support.The added imports for
NFTIconandNFT_CHAINSare consistent with the PR objectives and follow the existing import structure.Also applies to: 13-13
Line range hint
1-236: Overall implementation looks good, verify NFT routing.The changes to add NFT support to the FullScreenAccountMenu are well-implemented and integrate seamlessly with the existing code. The new functionality aligns with the PR objectives and maintains consistency with the current structure and style.
To ensure the NFT routing is correctly set up, please run the following verification script:
This script will help verify that the NFT routing is correctly set up in the application.
packages/extension-polkagate/src/partials/AccountMenu.tsx (3)
14-14: LGTM: NFTIcon import addedThe addition of
NFTIconto the import statement is consistent with the PR objectives to add NFT support.
Line range hint
1-238: Overall assessment: NFT support successfully addedThe changes to
AccountMenu.tsxsuccessfully implement NFT support as per the PR objectives. The additions include:
- Importing necessary components and constants
- Adding an
onNFTAlbumcallback function- Introducing a new MenuItem for the NFT Album
These changes are well-integrated into the existing component structure. Minor suggestions for improvement have been made, but overall, the implementation is solid and achieves the intended functionality.
18-18: LGTM: NFT_CHAINS constant importedThe addition of
NFT_CHAINSto the import statement is consistent with the PR objectives and will be used to determine which chains support NFT functionality.Please verify the contents of the
NFT_CHAINSconstant:✅ Verification successful
Verified: NFT_CHAINS constant correctly defined
The
NFT_CHAINSconstant is properly defined with the expected genesis hashes (STATEMINE_GENESIS_HASH,STATEMINT_GENESIS_HASH) and is correctly imported inAccountMenu.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the NFT_CHAINS constant # Test: Search for the NFT_CHAINS constant definition rg --type typescript 'export const NFT_CHAINS' -A 10Length of output: 89
Script:
#!/bin/bash # Description: Verify the contents of the NFT_CHAINS constant without specifying file type # Test: Search for the NFT_CHAINS constant definition in all files rg 'export const NFT_CHAINS' -A 10Length of output: 871
packages/extension-polkagate/src/util/constants.tsx (2)
Line range hint
1-114: Summary: NFT support added with minimal impactThe addition of the
NFT_CHAINSconstant is the only change to this file. It integrates well with the existing structure and naming conventions, and supports the PR's objective of adding NFT functionality to the PolkaGate extension.Key points:
- The new constant is correctly placed and exported.
- It uses existing genesis hash constants, maintaining consistency.
- The change has minimal impact on the rest of the file.
- The constant sets the foundation for NFT support on Statemine and Statemint chains.
Overall, this change is well-implemented and sets the stage for further NFT-related development in the application.
111-114: Verify usage of NFT_CHAINS across the applicationThe addition of the
NFT_CHAINSconstant is a good step towards implementing NFT support. To ensure its proper integration:
- Verify that this constant is used consistently across the application to enable or disable NFT functionality based on the current chain.
- Check if there are any UI components or logic that need to be updated to use this constant for conditional rendering or processing of NFT-related features.
To help with this verification, you can run the following script to find all occurrences of
NFT_CHAINSin the codebase:This will help identify where the constant is being used and ensure it's implemented correctly throughout the application.
✅ Verification successful
NFT_CHAINS is correctly utilized across the application
The
NFT_CHAINSconstant is consistently used in the following files to manage NFT-related functionality:
packages/extension-polkagate/src/util/constants.tsxpackages/extension-polkagate/src/partials/AccountMenu.tsxpackages/extension-polkagate/src/fullscreen/nft/index.tsxpackages/extension-polkagate/src/fullscreen/homeFullScreen/partials/FullScreenAccountMenu.tsxpackages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsxAll usages align with the intended purpose of enabling or disabling NFT features based on the specified chains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of NFT_CHAINS constant echo "Files using NFT_CHAINS:" rg --type typescript --type javascript "NFT_CHAINS" ./packages/extension-polkagate/srcLength of output: 178
Script:
#!/bin/bash # Search for usage of NFT_CHAINS constant in TypeScript and JavaScript files echo "Files using NFT_CHAINS:" rg "NFT_CHAINS" ./packages/extension-polkagate/src --glob "*.ts" --glob "*.tsx" --glob "*.js"Length of output: 1709
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (1)
13-13: Removal of ActionContext and related importsThe changes in the import statements indicate the removal of
ActionContextand its associated hookuseActionContext. This suggests a shift in how actions are handled within the component. The addition ofnoopfrom@polkadot/utilimplies that some previously active functionality might now be intentionally disabled.To ensure this change doesn't introduce unintended side effects, let's verify the usage of these removed imports throughout the codebase:
Also applies to: 16-16, 19-19
✅ Verification successful
Removal of
ActionContextin AccountInformationForHome.tsx VerifiedThe import and usage of
ActionContextanduseActionContexthave been successfully removed fromAccountInformationForHome.tsx. No unintended side effects detected in this file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of ActionContext or useActionContext rg --type typescript "ActionContext|useActionContext" packages/Length of output: 100
Script:
#!/bin/bash # Search for any remaining usage of ActionContext or useActionContext in .ts and .tsx files rg --type ts --type tsx "ActionContext|useActionContext" packages/Length of output: 96
Script:
#!/bin/bash # Search for any remaining usage of ActionContext or useActionContext in .ts and .tsx files rg "ActionContext|useActionContext" packages/ --glob "*.ts" --glob "*.tsx"Length of output: 24804
packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx (4)
19-19: LGTM: New imports for NFT functionality.The new imports for
NFTIconandNFT_CHAINSare correctly added and necessary for the NFT functionality being implemented.Also applies to: 21-21
121-121: LGTM: Efficient implementation ofnftDisable.The
nftDisablevariable is correctly implemented usinguseMemofor performance optimization. It properly checks if the currentgenesisHashis supported for NFT functionality.
162-164: LGTM: Well-implementedonNFTAlbumcallback.The
onNFTAlbumcallback is correctly implemented usinguseCallback. It properly checks for the address availability and NFT functionality status before opening the NFT album page.
Line range hint
1-290: Summary: NFT support successfully added to CommonTasks component.The changes introduce NFT functionality to the CommonTasks component in a clean and consistent manner. Key additions include:
- New imports for NFT-related components and constants.
- A memoized
nftDisablevariable to determine NFT support based on the chain.- An
onNFTAlbumcallback to handle navigation to the NFT album.- A new TaskButton for accessing the NFT album.
These changes align well with the PR objectives of adding NFT support to the PolkaGate extension. The implementation is efficient, uses appropriate React hooks, and maintains consistency with the existing codebase.
packages/extension-ui/src/Popup/index.tsx (2)
25-25: LGTM: NFTAlbum import added correctlyThe import statement for the NFTAlbum component is correctly placed and follows the existing import pattern in the file. This addition aligns with the PR objective of integrating NFT support into the PolkaGate extension.
25-25: Summary: NFT support successfully integratedThe changes to add NFT support to the PolkaGate extension have been implemented cleanly and consistently with the existing codebase structure. The new NFTAlbum component is correctly imported and a corresponding route has been added, following established patterns for error handling and routing.
These changes align well with the PR objectives and appear to be non-intrusive, maintaining the integrity of the existing functionality while extending the application's capabilities to include NFT support.
Also applies to: 342-342
packages/extension-polkagate/src/fullscreen/nft/utils/util.ts (2)
12-54: LGTM:fetchNFTsfunction implementation is correct.The
fetchNFTsfunction effectively retrieves and processes NFT data for the specified user account.
56-108: LGTM:fetchUniquesfunction implementation is correct.The
fetchUniquesfunction appropriately fetches and processes unique asset data for the user.
packages/extension-polkagate/src/fullscreen/nft/components/ItemFullScreenModal.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/nft/components/ItemFullScreenModal.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/nft/components/FilterItems.tsx
Outdated
Show resolved
Hide resolved
|
Suggestions:
|
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (7)
packages/extension-polkagate/src/fullscreen/accountDetails/components/NftGrouped.tsx (2)
19-22: Consider simplifyingaccountNfttype.The
accountNftprop allows bothnullandundefined. Consider using just one of these types to simplify null checks throughout the component. In TypeScript,undefinedis typically preferred for optional values.interface NftGroupedProps { address: string | undefined; - accountNft: ItemInformation[] | null | undefined; + accountNft?: ItemInformation[]; }
71-81: Enhance loading state feedback.The current loading spinner could be more informative by indicating what's being loaded.
if (isLoading) { return ( <Grid alignItems='center' container item px='10px' width='fit-content'> + <Grid item> <Wordpress color={theme.palette.text.disabled} size={31} timingFunction='linear' /> + </Grid> + <Grid item ml={1}> + <Typography variant="caption" color="text.secondary"> + Loading NFTs... + </Typography> + </Grid> </Grid> ); }packages/extension-polkagate/src/fullscreen/nft/index.tsx (1)
6-26: Consider organizing imports for better maintainabilityGroup related imports together for better code organization:
- External dependencies (React, MUI, FontAwesome)
- Internal components
- Hooks
- Utils and constants
import type { Chain } from '@polkadot/extension-chains/types'; import type { ItemInformation } from './utils/types'; + // External dependencies import React, { useCallback, useEffect, useState } from 'react'; import { Grid, Typography, useTheme } from '@mui/material'; import { useParams } from 'react-router'; import { faGem } from '@fortawesome/free-solid-svg-icons'; import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; + // Internal components import { Warning } from '../../components'; import FullScreenHeader from '../governance/FullScreenHeader'; import Bread from '../partials/Bread'; import { Title } from '../sendFund/InputPage'; import NftList from './components/NftList'; import Tabs from './components/Tabs'; + // Hooks import { useApiWithChain2, useFullscreen, useTranslation } from '../../hooks'; import { getAssetHubByChainName } from '../../hooks/useReferendum'; + // Utils and constants import NftManager from '../../class/nftManager'; import { SUPPORTED_NFT_CHAINS } from './utils/constants'; import { fetchItemMetadata } from './utils/util';packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (1)
93-104: Add proper TypeScript types for better type safety.The component could benefit from more specific type annotations.
- export const WithLoading = ({ children, loaded }: { loaded: boolean, children: React.ReactElement }) => ( + interface WithLoadingProps { + loaded: boolean; + children: React.ReactElement; + } + + export const WithLoading: React.FC<WithLoadingProps> = ({ children, loaded }) => (packages/extension-ui/src/Popup/index.tsx (1)
146-146: Consider memoizing or conditionally calling useNFT.The hook is called unconditionally and its return value is not used. Consider wrapping it in a condition or using React.useMemo to optimize performance.
- useNFT(accounts); + React.useMemo(() => { + if (accounts?.length) { + useNFT(accounts); + } + }, [accounts]);packages/extension-polkagate/src/class/nftManager.ts (2)
93-93: Complete the comment for thegetmethodThe comment on line 93 appears incomplete. It should specify that the method retrieves NFT items for a specific address.
Apply this change:
-// Get nft items for a specific +// Get NFT items for a specific address
4-5: Ensure consistent import statement formattingThere's a missing space in the import statement on line 5, which could affect code readability.
Adjust the import statement:
-import type { NftItemsType} from '../util/types'; +import type { NftItemsType } from '../util/types';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- packages/extension-polkagate/src/class/nftManager.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/NftGrouped.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (6 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/NftList.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/Thumbnail.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/index.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/util.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useNFT.tsx (1 hunks)
- packages/extension-polkagate/src/util/types.ts (3 hunks)
- packages/extension-ui/src/Popup/index.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/extension-polkagate/src/fullscreen/nft/components/NftList.tsx
- packages/extension-polkagate/src/fullscreen/nft/components/Thumbnail.tsx
- packages/extension-polkagate/src/fullscreen/nft/utils/constants.ts
- packages/extension-polkagate/src/fullscreen/nft/utils/util.ts
- packages/extension-polkagate/src/hooks/useNFT.tsx
- packages/extension-polkagate/src/util/types.ts
🔇 Additional comments (8)
packages/extension-polkagate/src/fullscreen/accountDetails/components/NftGrouped.tsx (1)
110-110: LGTM!Appropriate use of memoization to prevent unnecessary re-renders.
packages/extension-polkagate/src/fullscreen/nft/index.tsx (1)
67-73:⚠️ Potential issueRefactor API initialization to follow React hooks rules
The current implementation calls hooks inside a map function, which violates React hooks rules. Here's a better solution using
useMemo:-const apis = Object.fromEntries( - chainNames.map((chainName) => [ - chainName, - // eslint-disable-next-line react-hooks/rules-of-hooks - useApiWithChain2(getAssetHubByChainName(chainName) as Chain) - ]) -); +const apis = React.useMemo(() => { + const apiMap: Record<string, ReturnType<typeof useApiWithChain2>> = {}; + + chainNames.forEach((chainName) => { + const chain = getAssetHubByChainName(chainName) as Chain; + const api = useApiWithChain2(chain); + apiMap[chainName] = api; + }); + + return apiMap; +}, [chainNames]);Likely invalid or redundant comment.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (2)
46-51: LGTM! Clean implementation of collapsible buttons.The addition of the
collapseproperty and its implementation in theAccountButtoncomponent provides a clean way to handle UI space constraints when NFTs are present.Also applies to: 61-78
253-257: LGTM! Clean NFT integration.The
NftGroupedcomponent is well-integrated into the account information display, with proper props for account NFTs and address.packages/extension-ui/src/Popup/index.tsx (2)
344-344: LGTM! Route implementation follows best practices.The NFT route is properly integrated with error boundary wrapping and follows the established routing pattern.
25-25: Verify NFT-related component and hook implementations.Let's verify the existence and implementation of the imported NFT-related code.
Also applies to: 34-34, 146-146, 344-344
✅ Verification successful
Let me gather more information about the NFTAlbum component and its implementation.
Let me check the actual implementation of the NFT component and hook to ensure they are properly implemented.
NFT-related components and hooks are properly implemented
The verification shows that:
- The NFTAlbum component (named as
NFT) is properly implemented inpackages/extension-polkagate/src/fullscreen/nft/index.tsx- The
useNFThook exists inpackages/extension-polkagate/src/hooks/useNFT.tsxwith proper implementation- The component has all necessary dependencies and constants defined in
constants.ts- The implementation includes proper NFT fetching, display, and management functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify NFTAlbum component implementation echo "Checking NFTAlbum component..." fd "NFTAlbum" --type f # Verify useNFT hook implementation echo "Checking useNFT hook..." fd "useNFT" --type f # Check for any NFT-related test files echo "Checking NFT-related tests..." fd "nft.*\.(test|spec)\.(ts|tsx|js|jsx)$"Length of output: 334
Script:
#!/bin/bash # Check for NFT-related files in the extension-polkagate directory echo "Checking NFT files in extension-polkagate..." rg -l "NFTAlbum" packages/extension-polkagate/src/ # Check the implementation of NFTAlbum component echo "Checking NFTAlbum implementation..." ast-grep --pattern 'const NFTAlbum = $_' # Check for NFT-related imports echo "Checking NFT-related imports..." rg "import.*NFT" -A 2Length of output: 2751
Script:
#!/bin/bash # Check NFTAlbum component implementation echo "Checking NFTAlbum component content..." cat packages/extension-polkagate/src/fullscreen/nft/index.tsx # Check useNFT hook implementation echo "Checking useNFT hook content..." cat packages/extension-polkagate/src/hooks/useNFT.tsx # Check NFT constants echo "Checking NFT constants..." cat packages/extension-polkagate/src/fullscreen/nft/utils/constants.tsLength of output: 9420
packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (1)
9-37: The NFT interfaces are well-defined and comprehensiveThe introduction of
ItemOnChainInfo,ItemMetadata, andItemInformationinterfaces effectively structures NFT data management. Combining on-chain information with metadata enhances type safety and code maintainability.packages/extension-polkagate/src/class/nftManager.ts (1)
139-140: Ensure unique keys ingetItemKeymethodTo prevent key collisions, verify that all properties used in the key contribute to uniqueness, especially if
chainName,collectionId, oritemIdcan have overlapping values.Run the following script to check for potential key collisions:
Review
duplicate_keys.txtfor any entries, indicating potential key collisions.✅ Verification successful
The key generation method is safe and appropriate
The
getItemKeymethod uses a combination of properties that together ensure uniqueness:
chainName(required): identifies the specific blockchain (polkadot, kusama, or westend)collectionId(optional): unique identifier for the NFT collectionitemId(optional): unique identifier for the item within its collectionisNft(required): boolean flag distinguishing between NFTs and collectionsThe key structure is sufficient because:
- Items are always scoped to a specific chain
- Collection and item IDs are unique within their chain context
- The
isNftflag helps differentiate between collection-level and item-level entries- The method is used correctly in the codebase for comparing and tracking unique items
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate item keys across all NFTs. # Collect all item keys ast-grep --pattern '$_ = getItemKey($_)' > item_keys.txt # Find duplicates sort item_keys.txt | uniq -d > duplicate_keys.txt # Expected: duplicate_keys.txt should be empty.Length of output: 73
Script:
#!/bin/bash # Let's analyze the NFT item structure and usage of getItemKey # First, find the file containing getItemKey fd "nftManager.ts" --exec cat {} # Then search for any usage of this method across the codebase rg "getItemKey" -A 2Length of output: 6503
Script:
#!/bin/bash # Let's examine the ItemOnChainInfo type to understand the structure and possible values rg -A 10 "interface ItemOnChainInfo" # Also look for any type definitions or interfaces that might extend or implement ItemOnChainInfo rg "ItemOnChainInfo" -A 2Length of output: 2275
packages/extension-polkagate/src/fullscreen/accountDetails/components/NftGrouped.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/accountDetails/components/NftGrouped.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (5)
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx (1)
16-16: Add JSDoc documentation for component propsThe component would benefit from comprehensive JSDoc documentation explaining the purpose and type of each prop.
Add documentation like this:
/** * Displays a row of NFT information with optional price, text, and identity details * @param {string} accountId - The account identifier to display * @param {ApiPromise} api - The API instance for chain interaction * @param {Chain} chain - Chain information * @param {boolean} [divider=true] - Whether to show a divider * @param {boolean} [inline=true] - Whether to display inline * @param {boolean} [isThumbnail] - Whether this is a thumbnail view * @param {string} [link] - Optional link URL * @param {string} [linkName] - Name to display for the link * @param {string|null} [price] - Price in machine format * @param {string} [text] - Additional text content * @param {string} [title] - Row title */packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (1)
Line range hint
109-284: Consider implementing NFT-related state management using React Context.The current implementation manages NFT state at the component level. As NFT support grows, consider moving NFT state management to a React Context to:
- Avoid prop drilling
- Centralize NFT-related state and operations
- Make NFT data easily accessible across components
packages/extension-polkagate/src/fullscreen/nft/utils/util.ts (3)
19-23: Simplify IPFS CID extraction ingetContentUrlThe regular expressions used to sanitize the IPFS URL can be simplified for better readability and robustness.
Consider using a single regular expression:
let cid = url.replace(/^ipfs:(\/\/)?(?:ipfs\/)?/, '').replace(/^\/+/, '');
33-34: Provide more contextual error messagesThrowing a generic error with the message
'Rate limited'might not provide enough context for debugging.Include the URL or status code in the error message:
throw new Error(`Rate limited when fetching ${url}`);
7-7: Validate the import path forNftManagerThe import path for
NftManageruses a relative path that may be brittle and hard to maintain.Consider using absolute paths or aliases if your project setup allows it:
import NftManager from 'class/nftManager';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/extension-polkagate/src/class/nftManager.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (6 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/components/Thumbnail.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/index.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/types.ts (1 hunks)
- packages/extension-polkagate/src/fullscreen/nft/utils/util.ts (1 hunks)
- packages/extension-polkagate/src/util/types.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/extension-polkagate/src/fullscreen/nft/components/Thumbnail.tsx
- packages/extension-polkagate/src/fullscreen/nft/index.tsx
- packages/extension-polkagate/src/fullscreen/nft/utils/types.ts
- packages/extension-polkagate/src/util/types.ts
🔇 Additional comments (11)
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx (1)
1-94: Implementation aligns well with PR objectivesThe InfoRow component provides a flexible and reusable way to display NFT information, aligning with the PR's goal of adding comprehensive NFT support. The implementation follows the suggested modal-based approach for displaying NFT details.
packages/extension-polkagate/src/class/nftManager.ts (4)
7-17: LGTM! Well-structured type definitions and error handling.The type definitions are clear and the custom error class implementation follows best practices.
19-32: LGTM! Constructor implementation follows best practices.The initialization has been properly implemented using an initialization promise, addressing the previous concerns about async operations in the constructor.
51-65: LGTM! Storage loading implementation is robust.The storage loading implementation includes proper error handling and initialization state management.
88-94: LGTM! Storage change handling is well-implemented.The storage change handler correctly updates the internal state and triggers notifications.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (4)
46-51: LGTM: Well-structured interface update.The optional
collapseproperty addition toAccountButtonTypeinterface provides good flexibility for button appearance control.
61-80: LGTM: Clean implementation of collapsible button.The component properly handles the collapse state with good defaults and clean conditional styling.
123-149: LGTM: Well-implemented NFT state management.The useEffect hook properly handles:
- Initialization waiting
- Updates subscription
- Cleanup on unmount
258-262: Verify NftGrouped component integration.The NftGrouped component is integrated with proper props, but let's verify its implementation and usage across the codebase.
✅ Verification successful
NftGrouped component integration is properly implemented
The component is correctly integrated with:
- Proper prop types defined and passed (
accountNft: ItemInformation[],address: string)- Proper implementation including loading states, metadata fetching, and NFT display logic
- Consistent usage in AccountInformationForHome with the required props
- Well-handled edge cases (null/undefined checks, loading states)
- Proper memoization for performance optimization
The implementation follows React best practices and integrates seamlessly with the UI layout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify NftGrouped component implementation and usage # Check NftGrouped component implementation echo "Checking NftGrouped component implementation..." ast-grep --pattern 'export $_ function NftGrouped({ $_ }: { $_ })' # Check other usages of NftGrouped echo "Checking other usages of NftGrouped..." rg "NftGrouped" --type ts --type tsxLength of output: 325
Script:
#!/bin/bash # Let's try a different approach to find the NftGrouped component and its usages # Search for NftGrouped in all files (without file type restriction) echo "Searching for NftGrouped component..." rg "NftGrouped" # Find all TypeScript and React files that might contain NftGrouped echo "Finding TypeScript and React files containing NftGrouped..." fd -e ts -e tsx | xargs rg "NftGrouped" # Look for the file that exports NftGrouped echo "Looking for NftGrouped export..." rg "export.*NftGrouped"Length of output: 1890
Script:
#!/bin/bash # Let's examine the NftGrouped component implementation and its props # Check the implementation of NftGrouped component echo "Checking NftGrouped component implementation..." cat packages/extension-polkagate/src/fullscreen/accountDetails/components/NftGrouped.tsx # Check the accountNft prop type and usage echo "Checking accountNft prop usage in AccountInformationForHome..." cat packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsxLength of output: 16109
packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (1)
4-5: Consider addressing ESLint suppressions.The ESLint suppressions for
react/jsx-max-props-per-lineandcamelcasesuggest potential underlying issues:
- Long prop lists could benefit from prop spreading or component composition
- Camelcase violations often indicate inconsistent naming conventions
#!/bin/bash # Check for other files with similar ESLint suppressions rg -l "eslint-disable.*react/jsx-max-props-per-line|camelcase" --type typescriptAlso applies to: 6-6
packages/extension-polkagate/src/fullscreen/nft/utils/util.ts (1)
187-194: Verify handling ofnullvalues indetailobjectWhen constructing
detail, properties likeanimation_urlandimagemay benull. Ensure downstream code handles thesenullvalues properly.Please check that components consuming
detailcan handlenullwithout causing errors.
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/extension-polkagate/src/class/nftManager.ts (1)
34-37: Add timeout handling to initialization waitThe waitForInitialization method could benefit from a timeout mechanism to prevent infinite waiting.
public async waitForInitialization(): Promise<void> { - return this.initializationPromise; + const timeout = new Promise<never>((_, reject) => { + setTimeout(() => reject(new NftManagerError('Initialization timeout')), 30000); + }); + return Promise.race([this.initializationPromise, timeout]); }packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx (3)
45-50: Document the purpose of the collapse prop.Consider adding JSDoc comments to explain the purpose and behavior of the
collapseprop in theAccountButtonTypeinterface.+/** + * @interface AccountButtonType + * @property {string} text - The button text + * @property {() => void} onClick - Click handler + * @property {React.ReactNode} icon - Button icon + * @property {boolean} [collapse] - When true, hides the button text and shows only the icon + */ interface AccountButtonType { text: string; onClick: () => void; icon: React.ReactNode; collapse?: boolean; }
120-146: Improve error handling in NFT initialization.The error handling in the NFT initialization could be enhanced to:
- Handle initialization errors more gracefully
- Provide user feedback when NFT loading fails
useEffect(() => { if (!address) { return; } const handleNftUpdate = (updatedAddress: string, updatedNfts: ItemInformation[]) => { if (updatedAddress === address) { setNfts(updatedNfts); } }; // Waits for initialization nftManager.waitForInitialization() .then(() => { setNfts(nftManager.get(address)); }) - .catch(console.error); + .catch((error) => { + console.error('Failed to initialize NFTs:', error); + setNfts(null); // Indicate loading failure + }); // subscribe to the possible nft items for the account nftManager.subscribe(handleNftUpdate); return () => { nftManager.unsubscribe(handleNftUpdate); }; }, [address, nftManager]);
224-224: Consider using theme spacing for consistent margins.The divider margins are hardcoded and conditionally modified based on NFT presence. Consider using theme spacing values for consistency:
-<Divider orientation='vertical' sx={{ bgcolor: 'divider', height: '34px', ml: 0, mr: '10px', mx: myNfts ? '5px' : undefined, my: 'auto', width: '1px' }} /> +<Divider orientation='vertical' sx={{ bgcolor: 'divider', height: '34px', ml: 0, mr: (theme) => theme.spacing(1), mx: myNfts ? theme.spacing(0.5) : undefined, my: 'auto', width: '1px' }} /> -<Divider orientation='vertical' sx={{ bgcolor: 'divider', height: '34px', ml: '5px', mr: myNfts ? '5px' : '15px', my: 'auto', width: '1px' }} /> +<Divider orientation='vertical' sx={{ bgcolor: 'divider', height: '34px', ml: (theme) => theme.spacing(0.5), mr: myNfts ? theme.spacing(0.5) : theme.spacing(1.5), my: 'auto', width: '1px' }} />Also applies to: 237-237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
packages/extension-polkagate/src/class/nftManager.ts(1 hunks)packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountSetting.tsx(2 hunks)packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/AccountInformationForHome.tsx(5 hunks)packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/nft/index.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/nft/utils/util.ts(1 hunks)packages/extension-polkagate/src/hooks/index.ts(1 hunks)packages/extension-polkagate/src/util/constants.tsx(2 hunks)packages/extension-polkagate/src/util/types.ts(3 hunks)packages/extension-ui/src/Popup/index.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountSetting.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/CommonTasks.tsx
- packages/extension-polkagate/src/fullscreen/nft/components/InfoRow.tsx
- packages/extension-polkagate/src/fullscreen/nft/index.tsx
- packages/extension-polkagate/src/fullscreen/nft/utils/util.ts
- packages/extension-polkagate/src/hooks/index.ts
- packages/extension-polkagate/src/util/constants.tsx
- packages/extension-polkagate/src/util/types.ts
🔇 Additional comments (3)
packages/extension-ui/src/Popup/index.tsx (3)
26-26: LGTM: NFTAlbum import is properly placed.
The import follows the existing pattern of organizing feature-specific imports.
358-358: LGTM: NFT route follows established patterns.
The NFT route is properly implemented with error boundary wrapping and follows the same pattern as other address-based routes.
338-338: Verify the impact of removed routes.
The removal of these routes might affect existing functionality. Ensure that these features are either deprecated or their functionality has been moved elsewhere.
#!/bin/bash
# Check for any remaining references to the removed routes
echo "Checking for references to removed routes..."
rg -l "derivefs|manageValidators|managePoolValidators" --type ts --type tsx
# Check for any components still importing the removed components
echo "Checking for imports of removed components..."
rg -l "import.*FullscreenDerive|import.*ManageValidators|import.*ManageValidatorsPoolfs" --type ts --type tsxAlso applies to: 356-357
Nick-1979
added
ready to be merged
change requested
and removed
WIP
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/themes/dark.ts (1)
18-18: Add documentation for the backgroundFL property.Consider adding a JSDoc comment explaining the purpose and usage of the
backgroundFLproperty, as it seems to be an important theme customization for the NFT feature.Add documentation like this:
+ /** Background colors used for fullscreen layouts, particularly in NFT views */ backgroundFL: TypeText;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension-polkagate/src/fullscreen/accountDetails/components/AOC.tsx(2 hunks)packages/extension-polkagate/src/themes/dark.ts(1 hunks)packages/extension-polkagate/src/themes/light.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AOC.tsx
🔇 Additional comments (2)
packages/extension-polkagate/src/themes/light.ts (1)
17-17: LGTM: Type declaration aligns with NFT support requirements.
The change to make backgroundFL a required TypeText property ensures consistent theming across the NFT-related UI components.
packages/extension-polkagate/src/themes/dark.ts (1)
18-18: LGTM! Verify theme usage across the codebase.
The type change from optional Palette['primary'] to required TypeText for backgroundFL property improves type safety.
Let's verify the impact of this type change:
✅ Verification successful
Type change for backgroundFL is consistent across themes
The type change is properly implemented:
- Both light and dark themes declare
backgroundFLwithTypeTexttype - The light theme implements it with correct structure:
{ primary: '#DFDFDF', secondary: '#F1F1F1' } - No direct usage found in components, suggesting it's used through theme context
- Type is imported correctly from
@mui/material
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any potential theme-related issues across the codebase
# Test 1: Look for direct usage of backgroundFL in components
echo "Checking for backgroundFL usage in components..."
rg -l "backgroundFL" --type ts --type tsx
# Test 2: Look for theme customizations that might need updates
echo "Checking for other theme customizations..."
rg -l "createTheme|ThemeOptions" --type ts
Length of output: 519
Script:
#!/bin/bash
# Let's check the actual usage and implementation across theme files
# Check backgroundFL implementation in light theme
echo "=== Light Theme Implementation ==="
rg "backgroundFL" packages/extension-polkagate/src/themes/light.ts -A 2
# Check backgroundFL implementation in base theme
echo -e "\n=== Base Theme Implementation ==="
rg "backgroundFL" packages/extension-polkagate/src/themes/baseTheme.ts -A 2
# Check actual usage in View component
echo -e "\n=== View Component Usage ==="
rg "backgroundFL" packages/extension-polkagate/src/components/View.tsx -A 2
# Check TypeText definition
echo -e "\n=== TypeText Definition ==="
rg "TypeText" packages/extension-polkagate/src/themes/ -A 3
Length of output: 2647
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/extension-polkagate/src/fullscreen/governance/components/DraggableModal.tsx (2)
66-66: Consider improving border visibility and consistencyThe current implementation only shows borders in dark mode without blur, which might create inconsistent visual hierarchy. Consider:
- Using a minimum border width of 1px for better visibility
- Maintaining consistent borders across themes with adjusted opacity
- border: isDarkMode && !blurBackdrop ? '0.5px solid' : 'none', + border: !blurBackdrop ? '1px solid' : 'none', + borderColor: theme.palette.mode === 'dark' ? 'secondary.light' : 'rgba(0, 0, 0, 0.12)',
83-96: Add fallback for browsers without backdrop-filter supportWhile the blur implementation is clean, some browsers might not support
backdrop-filter. Consider adding a fallback style.backdropFilter: 'blur(5px)', backgroundColor: 'rgba(0, 0, 0, 0.4)' + '@supports not (backdrop-filter: blur(5px))': { + backgroundColor: 'rgba(0, 0, 0, 0.7)' + }packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx (2)
4-5: Consider using more specific ESLint rule disabling.Instead of disabling ESLint rules globally for the file, consider:
- Using inline disabling for specific instances where needed
- Configuring the rules in the ESLint config file if these are common patterns
-/* eslint-disable react/jsx-max-props-per-line */ -/* eslint-disable camelcase */ +// Disable specific rules only where needed +/* eslint-disable-next-line react/jsx-max-props-per-line */
118-127: Replace switch statement with a constant map for chain names.The switch statement for chain name mapping could be replaced with a constant map for better maintainability and easier updates.
+const CHAIN_NAME_SYMBOLS: Record<string, string> = { + 'Polkadot Asset Hub': 'ahp', + 'Kusama Asset Hub': 'ahk' +}; + const chainNameSymbol = useCallback(() => { - switch (itemInformation.chainName) { - case 'Polkadot Asset Hub': - return 'ahp'; - case 'Kusama Asset Hub': - return 'ahk'; - default: - return ''; - } + return CHAIN_NAME_SYMBOLS[itemInformation.chainName] || ''; }, [itemInformation.chainName]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension-polkagate/src/fullscreen/governance/components/DraggableModal.tsx(3 hunks)packages/extension-polkagate/src/fullscreen/nft/components/Details.tsx(1 hunks)
🔇 Additional comments (2)
packages/extension-polkagate/src/fullscreen/governance/components/DraggableModal.tsx (2)
16-16: LGTM: Props interface update is well-typed
The new optional blurBackdrop prop is properly typed and follows TypeScript conventions.
19-19: LGTM: Component signature maintains backward compatibility
The blurBackdrop prop is correctly added to the destructured parameters while maintaining existing default values and type safety.
| useEffect(() => { | ||
| const getUniqueGif = async () => { | ||
| if (itemInformation.isNft || !itemInformation.mediaUri) { | ||
| setGifSource(null); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const { isIPFS, sanitizedUrl } = getContentUrl(itemInformation.mediaUri); | ||
|
|
||
| if (!isIPFS) { | ||
| setGifSource(null); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const ipfsURL = IPFS_GATEWAY + sanitizedUrl; | ||
|
|
||
| const content = await fetchWithRetry(ipfsURL, 1); | ||
| const contentType = content.headers.get('content-type'); | ||
|
|
||
| if (!contentType?.includes('gif')) { | ||
| setGifSource(null); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const blob = await content.blob(); | ||
| const gifURL = URL.createObjectURL(blob); | ||
|
|
||
| setGifHash(sanitizedUrl); | ||
| setGifSource(gifURL); | ||
| }; | ||
|
|
||
| getUniqueGif().catch(console.error); | ||
| }, [itemInformation.isNft, itemInformation.mediaUri]); |
There was a problem hiding this comment.
Handle potential race conditions in IPFS fetching.
The IPFS fetching logic could lead to race conditions if multiple fetches are triggered in quick succession. Consider implementing a cancellation mechanism.
useEffect(() => {
+ let isMounted = true;
+ const controller = new AbortController();
+
const getUniqueGif = async () => {
if (itemInformation.isNft || !itemInformation.mediaUri) {
- setGifSource(null);
+ if (isMounted) setGifSource(null);
return;
}
const { isIPFS, sanitizedUrl } = getContentUrl(itemInformation.mediaUri);
if (!isIPFS) {
- setGifSource(null);
+ if (isMounted) setGifSource(null);
return;
}
const ipfsURL = IPFS_GATEWAY + sanitizedUrl;
- const content = await fetchWithRetry(ipfsURL, 1);
+ const content = await fetchWithRetry(ipfsURL, 1, { signal: controller.signal });
const contentType = content.headers.get('content-type');
if (!contentType?.includes('gif')) {
- setGifSource(null);
+ if (isMounted) setGifSource(null);
return;
}
const blob = await content.blob();
const gifURL = URL.createObjectURL(blob);
- setGifHash(sanitizedUrl);
- setGifSource(gifURL);
+ if (isMounted) {
+ setGifHash(sanitizedUrl);
+ setGifSource(gifURL);
+ } else {
+ URL.revokeObjectURL(gifURL);
+ }
};
getUniqueGif().catch(console.error);
+
+ return () => {
+ isMounted = false;
+ controller.abort();
+ };
}, [itemInformation.isNft, itemInformation.mediaUri]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const getUniqueGif = async () => { | |
| if (itemInformation.isNft || !itemInformation.mediaUri) { | |
| setGifSource(null); | |
| return; | |
| } | |
| const { isIPFS, sanitizedUrl } = getContentUrl(itemInformation.mediaUri); | |
| if (!isIPFS) { | |
| setGifSource(null); | |
| return; | |
| } | |
| const ipfsURL = IPFS_GATEWAY + sanitizedUrl; | |
| const content = await fetchWithRetry(ipfsURL, 1); | |
| const contentType = content.headers.get('content-type'); | |
| if (!contentType?.includes('gif')) { | |
| setGifSource(null); | |
| return; | |
| } | |
| const blob = await content.blob(); | |
| const gifURL = URL.createObjectURL(blob); | |
| setGifHash(sanitizedUrl); | |
| setGifSource(gifURL); | |
| }; | |
| getUniqueGif().catch(console.error); | |
| }, [itemInformation.isNft, itemInformation.mediaUri]); | |
| useEffect(() => { | |
| let isMounted = true; | |
| const controller = new AbortController(); | |
| const getUniqueGif = async () => { | |
| if (itemInformation.isNft || !itemInformation.mediaUri) { | |
| if (isMounted) setGifSource(null); | |
| return; | |
| } | |
| const { isIPFS, sanitizedUrl } = getContentUrl(itemInformation.mediaUri); | |
| if (!isIPFS) { | |
| if (isMounted) setGifSource(null); | |
| return; | |
| } | |
| const ipfsURL = IPFS_GATEWAY + sanitizedUrl; | |
| const content = await fetchWithRetry(ipfsURL, 1, { signal: controller.signal }); | |
| const contentType = content.headers.get('content-type'); | |
| if (!contentType?.includes('gif')) { | |
| if (isMounted) setGifSource(null); | |
| return; | |
| } | |
| const blob = await content.blob(); | |
| const gifURL = URL.createObjectURL(blob); | |
| if (isMounted) { | |
| setGifHash(sanitizedUrl); | |
| setGifSource(gifURL); | |
| } else { | |
| URL.revokeObjectURL(gifURL); | |
| } | |
| }; | |
| getUniqueGif().catch(console.error); | |
| return () => { | |
| isMounted = false; | |
| controller.abort(); | |
| }; | |
| }, [itemInformation.isNft, itemInformation.mediaUri]); |
# [0.23.0](v0.22.0...v0.23.0) (2024-11-09) ### Features * support NFTs and Uniques ([#1564](#1564)) ([7c38aac](7c38aac))
Works Done
Close: #1537
Summary by CodeRabbit
Release Notes
New Features
AudioPlayercomponent for audio playback within the NFT interface.Detailscomponent for displaying detailed NFT information in a modal.FullScreenNFTcomponent for fullscreen viewing of NFTs.ItemSkeletoncomponent for better user experience.Tabscomponent for managing and displaying different categories of NFTs.NftGroupedcomponent to display grouped views of NFTs associated with accounts.NftManagerclass for local storage and listener notifications.InfoRowcomponent for structured display of account-related NFT details.ItemAvatarcomponent for displaying NFT images with fullscreen functionality.Popupcomponent.blurBackdropoption in theDraggableModalfor customizable backdrop effects.Bug Fixes
AOCcomponent based on asset count.Documentation