feat: add wallet collapse functionality to all wallet types [superseded by #39645]#39583
Closed
georgewrmarshall wants to merge 1 commit intomainfrom
Closed
feat: add wallet collapse functionality to all wallet types [superseded by #39645]#39583georgewrmarshall wants to merge 1 commit intomainfrom
georgewrmarshall wants to merge 1 commit intomainfrom
Conversation
Contributor
|
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. |
6851344 to
4120f55
Compare
Contributor
✨ Files requiring CODEOWNER review ✨🔑 @MetaMask/accounts-engineers (2 files, +104 -5)
|
ui/components/multichain-accounts/multichain-account-list/multichain-account-list.tsx
Outdated
Show resolved
Hide resolved
Contributor
Builds ready [4120f55]
UI Startup Metrics (1321 ± 124 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Contributor
Builds ready [2e83626]
UI Startup Metrics (1348 ± 119 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
ccharly
reviewed
Jan 28, 2026
| setExpandedWallets((current) => ({ | ||
| ...current, | ||
| // Toggle: if currently false, set to true; otherwise set to false (default expanded) | ||
| [walletId]: current[walletId] === false, |
Contributor
There was a problem hiding this comment.
Cosmetic: But I think using !value is pretty neat for "toggles"? But I think we're not doing this here because by default, expanded can be either true or undefined? (since expandedWallets is not initialized with all existing wallet IDs).
Am I getting that right? IMO that would be clearer if were dealing with boolean only
| result.push(...accounts); | ||
| // Only add accounts if wallet is expanded (default is expanded) | ||
| const isWalletExpanded = | ||
| expandedWallets[walletId as AccountWalletId] !== false; |
Contributor
There was a problem hiding this comment.
That would also avoid this kind of explicit check to !== false with something that looks like a boolean 😅
| type: 'wallet-header'; | ||
| key: string; | ||
| text: string; | ||
| walletId: string; |
Contributor
There was a problem hiding this comment.
What about using a AccountWalletId directly to avoid type-casting later on?
|
|
||
| if (item.type === 'wallet-header') { | ||
| const isWalletExpanded = | ||
| expandedWallets[item.walletId as AccountWalletId] !== false; |
Contributor
There was a problem hiding this comment.
Regarding my other comment about AccountWalletId, we would not need any type-cast like this one (if we go with my suggestion)
2e83626 to
ac89136
Compare
| // Only show accounts if wallet is expanded | ||
| if (isWalletExpanded) { | ||
| result.push(...accounts); | ||
| } |
There was a problem hiding this comment.
New wallets appear collapsed instead of expanded by default
Medium Severity
The expandedWallets state is initialized only once on mount with existing wallet IDs set to true. If a new wallet is added to the wallets prop after mount, it won't exist in expandedWallets, causing expandedWallets[walletId] to return undefined. Since the check if (isWalletExpanded) treats undefined as falsy, new wallets appear collapsed instead of expanded by default—contradicting the PR's stated behavior. The check should use expandedWallets[walletId] !== false to treat undefined as expanded.
Additional Locations (1)
ac89136 to
daa423b
Compare
daa423b to
b8a632f
Compare
- Add expandedWallets state with proper boolean initialization - Add handleWalletToggle callback for toggling wallet expansion - Create new wallet-header ListItem type for collapsible headers - Integrate collapse logic into list building (only show accounts when expanded) - Add wallet-header rendering with arrow icon - Import virtualized-list component from main - Use Object.keys() pattern for cleaner type assertions This integrates the wallet collapse functionality with the new virtualized list structure from main. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
b8a632f to
732fa4c
Compare
9249e67 to
732fa4c
Compare
Contributor
Builds ready [732fa4c]
UI Startup Metrics (1338 ± 124 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
Contributor
Author
|
Closing this PR in favor of #39645, which implements a more robust solution to the wallet collapse functionality. PR #39645 handles virtualized list height recalculation better and includes additional improvements like collapsible pinned sections. Thank you to everyone who reviewed this PR! The exploration here was valuable, and the final implementation in #39645 benefits from the lessons learned. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR explored adding collapsible wallet sections to the MultichainAccountList component. After implementation and testing, we discovered that PR #39645 implements a more robust solution to the same problem.
Why PR #39645 is preferred:
getItemKeyin virtualizer config for better React reconciliationThis PR is being closed in favor of #39645, which provides the same end-user functionality with a superior technical implementation.
Original implementation details preserved below for reference:
Implemented collapsible wallet sections in the MultichainAccountList component, extending the collapse functionality that was previously only available for hidden accounts to all wallet types (Entropy, Keyring, Snap).
Changes in this PR:
expandedWalletsstate to track collapse state per walletChangelog
CHANGELOG entry: null (superseded by #39645)
Related issues
Superseded by: #39645
Related: https://consensyssoftware.atlassian.net/browse/MDP-678
Manual testing steps
See PR #39645 for testing steps.
Screenshots/Recordings
Before
Wallet headers were not interactive - all wallets were always expanded.
before720.mov
After
Wallet headers are now clickable buttons with collapse/expand arrows. Each wallet can be independently collapsed to hide its accounts.
collapse.after720.mov
🤖 Generated with Claude Code