chore: Align headers for Import Assets flow #24304
Conversation
…to header/importtokennft
…to header/importtokennft
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
metamaskbot
added
the
team-design-system
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
brianacnguyen
changed the title
brianacnguyen
added
the
No QA Needed
…to header/importtokennft
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsThis PR is a UI/UX refactoring that affects multiple areas of the app:
Selected tags rationale:
The risk is medium because:
|
| description: strings('collectible.ownership_verification_error'), | ||
| }, | ||
| hasNoTimeout: false, | ||
| }); |
There was a problem hiding this comment.
Potential crash when calling navigation.goBack() on optional navigation prop
The navigation prop is defined as optional (line 112: navigation?: any), but navigation.goBack() is called directly without null checking at lines 295 and 299. If the component is rendered without the navigation prop, this will cause a runtime crash when addNft() completes successfully or when cancelAddCollectible() is called.
Other components in the codebase correctly use optional chaining for this pattern (e.g., app/components/UI/Card/routes/index.tsx:86 uses navigation?.goBack()).
🔬 Verification Test
Code analysis:
// Line 112 - navigation is optional
interface AddCustomCollectibleProps {
navigation?: any; // Optional
...
}
// Line 295 - No null check before calling goBack()
const addNft = async (): Promise<void> => {
...
setLoading(false);
navigation.goBack(); // Will crash if navigation is undefined
};
// Line 299 - No null check before calling goBack()
const cancelAddCollectible = (): void => {
navigation.goBack(); // Will crash if navigation is undefined
};Test evidence:
The test file at line 669-679 (Props Variations section) explicitly tests rendering without the navigation prop:
it('renders without navigation prop', () => {
const { getByTestId } = renderWithProvider(
<AddCustomCollectible
setOpenNetworkSelector={jest.fn()}
networkId={'0x1'}
selectedNetwork={'Ethereum Mainnet'}
networkClientId={'mainnet'}
/>,
{ state: initialRootState },
);
expect(getByTestId('import-nft-screen')).toBeTruthy();
});This test passes because it only renders the component. If this test attempted to trigger addNft or cancelAddCollectible, it would crash.
Fix: Replace navigation.goBack() with navigation?.goBack() at both locations.
There was a problem hiding this comment.
| description: strings('collectible.ownership_verification_error'), | ||
| }, | ||
| hasNoTimeout: false, | ||
| }); |
There was a problem hiding this comment.
Potential crash when calling navigation.goBack() on optional navigation prop
The navigation prop is defined as optional (line 112: navigation?: any), but navigation.goBack() is called directly without null checking at lines 295 and 299. If the component is rendered without the navigation prop, this will cause a runtime crash when addNft() completes successfully or when cancelAddCollectible() is called.
Other components in the codebase correctly use optional chaining for this pattern (e.g., app/components/UI/Card/routes/index.tsx:86 uses navigation?.goBack()).
The test file explicitly tests rendering without the navigation prop in the "Props Variations" section (line 668-679), confirming this is a valid use case. If that test attempted to trigger addNft or cancelAddCollectible, it would crash.
Fix: Replace navigation.goBack() with navigation?.goBack() at both locations.
| ); | ||
|
|
||
| if (!isOwner) | ||
| Alert.alert( |
There was a problem hiding this comment.
ChainId mismatch - validation uses global state chainId instead of selected network's chainId
The validateCustomCollectibleAddress function uses chainId from global state (retrieved via useSelector(selectChainId) on line 150), but the component receives a networkId prop that represents the user-selected network's chain ID. When a user selects a different network via the network selector UI, the validation will still occur against the globally-selected network, not the one the user chose.
This causes:
- Smart contract address validation to query the wrong blockchain
- NFTs may fail to validate even when they exist on the selected network
- Analytics tracking (
getAnalyticsParamsat line 169) reports the wrong chain ID
In AddAsset.tsx:279, the networkId prop is correctly set to the selected network's chain ID, but AddCustomCollectible ignores this prop for validation and uses the global chainId instead. Notably, the Avatar component at line 344 correctly uses networkId, making this inconsistency more evident.
Scenario: User's wallet is connected to Polygon, but they select Ethereum Mainnet in the NFT import network selector. The smart contract validation will query Polygon (global state) instead of Ethereum (selected network), causing incorrect validation results.
Fix: Replace chainId with networkId in the isSmartContractAddress call and getAnalyticsParams function.
There was a problem hiding this comment.
| description: strings('collectible.ownership_verification_error'), | ||
| }, | ||
| hasNoTimeout: false, | ||
| }); |
There was a problem hiding this comment.
Inconsistent networkClientId fallback logic - validation uses fallback but addNft/ownership check do not
High Severity
The validateCustomCollectibleAddress function at line 181 uses a fallback pattern for the network client ID:
const clientId = networkClientId || selectedNetworkClientId;However, validateCollectibleOwnership (line 228) and addNft (line 281) use networkClientId directly without the fallback:
// Line 228 in validateCollectibleOwnership:
const isOwner = await NftController.isNftOwner(
selectedAddress,
address,
tokenId,
networkClientId, // No fallback!
);
// Line 281 in addNft:
await NftController.addNft(address, tokenId, networkClientId); // No fallback!This inconsistency can cause:
- Address validation passes using
selectedNetworkClientIdas fallback whennetworkClientIdis null - Ownership validation and NFT addition then fail or behave unexpectedly because they receive
nullinstead of the fallback value - The NFT could be added to the wrong network or the operation could fail silently
The networkClientId prop is typed as string | null (line 119), so null values are explicitly allowed.
| description: strings('collectible.ownership_verification_error'), | ||
| }, | ||
| hasNoTimeout: false, | ||
| }); |
There was a problem hiding this comment.
Missing try-catch in addNft causes UI to get stuck in loading state on error
High Severity
The addNft function sets setLoading(true) at the start but has no try-catch block around the operations that can throw. If NftController.addNft (line 281) or isSmartContractAddress (called via validateCustomCollectible) throws an error (e.g., network failure), the function will exit without:
- Calling
setLoading(false)- leaving the UI stuck in a loading state with a disabled button - Calling
endTrace({ name: TraceName.ImportNfts })- leaving an orphaned trace - Showing any error feedback to the user
Contrast this with validateCollectibleOwnership (lines 219-262) which properly wraps its async operations in try-catch and shows a toast on error.
const addNft = async (): Promise<void> => {
setLoading(true);
// ... validation ...
trace({ name: TraceName.ImportNfts });
await NftController.addNft(address, tokenId, networkClientId); // Can throw!
endTrace({ name: TraceName.ImportNfts }); // Never reached on error
// ...
setLoading(false); // Never reached on error - UI stuck!
navigation.goBack();
};The isSmartContractAddress function in app/util/transactions/index.js also lacks error handling and will throw on network errors, propagating up through validateCustomCollectibleAddress → validateCustomCollectible → addNft.
|
Description
This PR updates the header and footer components in the import token/NFT flows to align with the design system. The changes include:
Replaced legacy navbar functions with the new
HeaderCentercomponent:getImportTokenNavbarOptions→getHeaderCenterNavbarOptionsgetWebviewNavbar→getHeaderCenterNavbarOptions(removed from Navbar)Updated bottom action buttons layout:
KeyboardAwareScrollViewinActionViewso they remain fixed at the bottom of the screenReplaced Alert dialogs with Toast notifications in
AddCustomCollectiblefor ownership verification errorsUpdated bottom sheets to use
HeaderCenterandBottomSheetFootercomponents instead ofSheetHeaderandSheetActionViewFixed button text casing for NFT import flow (CANCEL → Cancel, IMPORT → Import)
Changelog
CHANGELOG entry: Updated headers and footers in import token and NFT flows to use design system components
Related issues
Fixes: https://consensyssoftware.atlassian.net/jira/software/c/projects/MDP/boards/2972?assignee=62afb43d33a882e2be47c36f&quickFilter=3325&selectedIssue=MDP-269
Manual testing steps
Screenshots/Recordings
Before
After
Import Token
https://github.com/user-attachments/assets/95ed563b-b94f-4c4b-a437-2fc174a758cf
Import NFT
https://github.com/user-attachments/assets/e08d9e0f-6133-4af5-8289-f9d881439194
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Aligns import asset flows with design-system components and stabilizes action CTAs across screens.
getHeaderCenterNavbarOptionsinAddAsset,ConfirmAddAsset,SimpleWebview, and related bottom sheets; add share icon via header end button inSimpleWebview; removegetWebviewNavbarActionViewfooter outsideKeyboardAwareScrollViewand adjust platform-specific paddingAlertdialogs for Toasts inAddCustomCollectibleownership checks; update tests to useToastContextBottomSheetFooterandHeaderCenterin sheets; fix NFT import button casing and update locale stringsWritten by Cursor Bugbot for commit dbf963c. This will update automatically on new commits. Configure here.