refactor: use const for navigate(-1) and navigate('/'); also migrate the remaining files to v5-compat #37819
+701
−476
Conversation
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
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. |
Collaborator
✨ Files requiring CODEOWNER review ✨🔑 @MetaMask/accounts-engineers (6 files, +38 -32)
👨🔧 @MetaMask/core-extension-ux (7 files, +24 -20)
💎 @MetaMask/metamask-assets (1 files, +5 -2)
🔔 @MetaMask/notifications (1 files, +0 -10)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ui/helpers/higher-order-components/with-router-hooks/with-router-hooks.tsx
Show resolved
Hide resolved
Collaborator
Builds ready [66acd3c]
UI Startup Metrics (1269 ± 105 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [b0a310d]
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
from
b0a310d to
12736f7
Compare
Collaborator
Builds ready [12736f7]
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [202a374]
UI Startup Metrics (1256 ± 106 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
from
996910f to
95d2990
Compare
Collaborator
Builds ready [6d381a4]
UI Startup Metrics (1293 ± 104 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
from
6d381a4 to
fce7555
Compare
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
2 times, most recently
from
6e7d162 to
70f27cf
Compare
| <ConnectedAccounts navigate={this.props.navigate} /> | ||
| </ScrollContainer> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Routes removed breaking navigation to connected pages
The Route components for CONNECTED_ROUTE and CONNECTED_ACCOUNTS_ROUTE were removed and replaced with conditional rendering based on location.pathname. This breaks routing because these paths are now rendered inside the DEFAULT_ROUTE handler instead of being proper routes. The router won't match these URLs correctly, causing navigation issues. Users navigating to /connected or /connected/accounts will see the correct content but the router state will be incorrect, potentially breaking back navigation and other routing-dependent features.
Collaborator
Builds ready [70f27cf]
UI Startup Metrics (1224 ± 99 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
2 times, most recently
from
5d86d7c to
b94e29a
Compare
Collaborator
Builds ready [b94e29a]
UI Startup Metrics (1185 ± 92 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
from
b94e29a to
faa950e
Compare
| </ScrollContainer> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Manual route handling bypasses React Router
The Home component manually checks location.pathname and renders ConnectedSites and ConnectedAccounts components directly, bypassing React Router's routing system. These routes (CONNECTED_ROUTE and CONNECTED_ACCOUNTS_ROUTE) were previously handled by <Route> components but are now being rendered conditionally in the Home component's render method. This breaks proper route handling, prevents route transitions, and could cause navigation issues since these components are rendered outside the router context.
Contributor
Author
There was a problem hiding this comment.
same as above, this is not valid
DDDDDanica
force-pushed
the
refactor/5477/v5-compat
branch
from
faa950e to
62fcbcf
Compare
| <ConnectedAccounts navigate={this.props.navigate} /> | ||
| </ScrollContainer> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Connected routes broken by incorrect routing architecture
The CONNECTED_ROUTE and CONNECTED_ACCOUNTS_ROUTE handling was moved from router-level Route components into the Home component's render method. However, Home is mounted at DEFAULT_ROUTE (/), so when users navigate to /connected or /connected/accounts, the router won't match the Home component at all. The conditional checks location?.pathname === CONNECTED_ROUTE will never be true because Home won't be rendered for these paths. These routes need to be restored in the router configuration or Home needs to be mounted at a parent route that can handle these child paths.
Contributor
Author
There was a problem hiding this comment.
This is not correct under our situation, the parent router in routes.component.tsx still uses v5's <Switch> which does prefix matching, so Home at path="/" matches /connected too, allowing our conditional rendering to work:
// routes.component.tsx - STILL using v5 <Switch> (not migrated yet)
<Switch>
<RouteWithLayout path={DEFAULT_ROUTE} /* "/" */ layout={RootLayout}>
{createV5CompatRoute(Home, { includeNavigate: true, includeLocation: true })}
</RouteWithLayout>
</Switch>
// home.component.js - uses v5-compat navigate/location props
if (location?.pathname === '/connected') return <ConnectedSites navigate={navigate} />;
if (location?.pathname === '/connected/accounts') return <ConnectedAccounts navigate={navigate} />;
return <HomeContent />; // default for '/'
We're mixing v5 routing structure (Switch) with v5-compat navigation (useNavigate, useLocation) - that's why we can't use nested <Routes> inside Home yet. And this will be removed by v6
Collaborator
Builds ready [62fcbcf]
UI Startup Metrics (1223 ± 93 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [15d2ea2]
UI Startup Metrics (1218 ± 105 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
david0xd
reviewed
Nov 24, 2025
Collaborator
Builds ready [8e8a6d3]
UI Startup Metrics (1222 ± 98 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
david0xd
reviewed
Nov 24, 2025
david0xd
reviewed
Nov 24, 2025
Collaborator
Builds ready [f76f9b5]
UI Startup Metrics (1261 ± 109 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Collaborator
Builds ready [dc9beda]
UI Startup Metrics (1207 ± 106 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
david0xd
approved these changes
Nov 24, 2025
HowardBraham
approved these changes
Nov 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Changelog
CHANGELOG entry: null
Related issues
Fixes: partial is for https://github.com/MetaMask/MetaMask-planning/issues/6231
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Replace history-based navigation with v5-compat navigate and PREVIOUS_ROUTE/DEFAULT_ROUTE across the app, updating routing scaffolding, components, hooks, tests, and stories.
navigatebridge inroutes.component.tsx; passnavigate,location, andparamsto v5-compat components via helpers.navigate(-1)withnavigate(PREVIOUS_ROUTE); standardize uses ofDEFAULT_ROUTEand other route constants.react-router-dom-v5-compatAPIs (useNavigate,useLocation,useMatch,matchPath) across files.navigateinstead ofhistory(e.g.,Alerts,InvalidCustomNetworkAlert,HideTokenConfirmationModal,NicknamePopover, SRP-related components, NFT image view, transaction details, permissions page, snaps list/view, home, connected accounts/sites, asset options/token asset, notifications settings, wallet/account details).PREVIOUS_ROUTEfor back actions (e.g., SRP flows, bridge tx details, send hooks, confirm flows).useClaimState,useCurrentAsset,useSegmentContext).utils.jsroute matching to v5-compatmatchPathsignature.DEFAULT_ROUTE,PREVIOUS_ROUTE); adjust mocks accordingly.navigateargs.rewardsslice (remove extraneous whitespace).Written by Cursor Bugbot for commit dc9beda. This will update automatically on new commits. Configure here.