refactor: use accountGroupId query param instead of URL segment#40773
Merged
refactor: use accountGroupId query param instead of URL segment#40773
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. |
Contributor
✨ Files requiring CODEOWNER review ✨🔑 @MetaMask/accounts-engineers (7 files, +98 -69)
👨🔧 @MetaMask/core-extension-ux (2 files, +13 -11)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...ages/multichain-accounts/multichain-account-details-page/multichain-account-details-page.tsx
Outdated
Show resolved
Hide resolved
...multichain-accounts/multichain-account-details-page/multichain-account-details-page.test.tsx
Outdated
Show resolved
Hide resolved
ui/pages/multichain-accounts/wallet-details-page/wallet-details-page.tsx
Outdated
Show resolved
Hide resolved
n3ps
changed the title
n3ps
changed the title
Contributor
Builds ready [97d2ef9]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
n3ps
commented
Mar 11, 2026
| mockUseParams.mockReturnValue({ | ||
| id: 'keyring:Ledger Hardware/0xc42edfcc21ed14dda456aa0756c153f7985d8813', | ||
| }); | ||
| setSearchParams(LEDGER_ACCOUNT_GROUP_ID); |
Contributor
Author
There was a problem hiding this comment.
Reducing magic strings
ui/components/multichain-accounts/multichain-account-menu/multichain-account-menu.test.tsx
Outdated
Show resolved
Hide resolved
Contributor
Builds ready [52b5640]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
ui/components/multichain/menu-items/account-details-menu-item.test.js
Outdated
Show resolved
Hide resolved
...multichain-accounts/multichain-account-details-page/multichain-account-details-page.test.tsx
Outdated
Show resolved
Hide resolved
n3ps
force-pushed
the
n3ps/more-account-id-url
branch
from
f4d21ed to
273b81f
Compare
Contributor
Builds ready [273b81f]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
ui/pages/multichain-accounts/wallet-details-page/wallet-details-page.tsx
Show resolved
Hide resolved
Contributor
Builds ready [52a9314]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
ameliejyc
approved these changes
Mar 11, 2026
|
Contributor
Builds ready [453f2fd]
⚡ Performance Benchmarks
🌐 Dapp Page Load BenchmarksCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
ccharly
reviewed
Mar 11, 2026
| ); | ||
| expect(mockNavigate).toHaveBeenCalledWith({ | ||
| pathname: MULTICHAIN_ACCOUNT_DETAILS_PAGE_ROUTE, | ||
| search: `accountGroupId=${encodeURIComponent(mockInternalAccount.address)}`, |
Contributor
There was a problem hiding this comment.
Note for us, this is completely wrong 😅 it's not a valid group ID!
| }, | ||
| { | ||
| path: `${MULTICHAIN_ACCOUNT_DETAILS_PAGE_ROUTE}/:id`, | ||
| path: MULTICHAIN_ACCOUNT_DETAILS_PAGE_ROUTE, |
Contributor
There was a problem hiding this comment.
Nit: But that would have been nice to add a small "note" about the new query param for the ID (just so that we know the route expects a required parameter)
Contributor
Author
There was a problem hiding this comment.
Good idea. Have to think about common convention for this
| }, | ||
| { | ||
| path: `${MULTICHAIN_WALLET_DETAILS_PAGE_ROUTE}/:id`, | ||
| path: MULTICHAIN_WALLET_DETAILS_PAGE_ROUTE, |
...ages/multichain-accounts/multichain-account-details-page/multichain-account-details-page.tsx
Show resolved
Hide resolved
ccharly
approved these changes
Mar 11, 2026
metamaskbot
added
the
release-13.23.0
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.
4 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
Continuation of #40718
Context:
We use this format: entropy:01JKAF3DSGM3AB87EM9N0K41AJ/0
as part of this URL pattern: /multichain-account-address-list/:accountGroupId
We are able work fine with encodeURIComponent but this is more of a fluke in the way useRoutes works. Once we update the react-router setup for example to use Outlet, it exposes the problem in stricter browsers like Firefox where the encoded %2F gets decoded back to / before react-router has a chance to read it, resulting in:
/page/entropy%3A01J.../0 // 3 segments
Proposal:
Convert to querystring
// Before
/page/${encodeURIComponent(accountGroupId)})// After
/page?accountGroupId=${encodeURIComponent(accountGroupId)}Changelog
CHANGELOG entry: refactor: use search params for account id
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Medium risk because it changes client-side routing and URL construction for multichain details pages; incorrect param handling could break navigation or redirects, though changes are localized and covered by updated tests.
Overview
Updates multichain account and wallet details pages to use query parameters instead of URL path segments for identifiers that may contain
/.Navigation from menus/pages now calls
navigate({ pathname, search })withcreateSearchParams(e.g.accountGroupId,id), and the route definitions forMULTICHAIN_ACCOUNT_DETAILS_PAGE_ROUTEandMULTICHAIN_WALLET_DETAILS_PAGE_ROUTEare updated to no longer require/:id. The account details page now readsaccountGroupIdviauseSearchParamsand redirects toDEFAULT_ROUTEwhen missing/invalid; stories and unit tests are adjusted to match the new URLs.Written by Cursor Bugbot for commit 453f2fd. This will update automatically on new commits. Configure here.