refactor(5564): migrate confirmation and settings page to v5-compat #37508
+7,939
−8,581
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/extension-platform (1 files, +111 -5)
🔒 @MetaMask/extension-security-team (1 files, +8 -10)
|
DDDDDanica
force-pushed
the
refactor/settings-v5-compat
branch
from
e4c145c to
6e64965
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
Builds ready [9446f28]
UI Startup Metrics (1230 ± 96 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/settings-v5-compat
branch
from
9446f28 to
268c62c
Compare
Collaborator
Builds ready [268c62c]
UI Startup Metrics (1258 ± 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/settings-v5-compat
branch
from
268c62c to
6d7878a
Compare
Collaborator
Builds ready [6d7878a]
UI Startup Metrics (1240 ± 97 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/settings-v5-compat
branch
from
6d7878a to
020aa7e
Compare
Collaborator
Builds ready [020aa7e]
UI Startup Metrics (1210 ± 90 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 [33cbb0d]
UI Startup Metrics (1242 ± 95 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/settings-v5-compat
branch
from
33cbb0d to
732e1ce
Compare
Collaborator
Builds ready [732e1ce]
UI Startup Metrics (1233 ± 86 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/settings-v5-compat
branch
from
732e1ce to
98d8f5b
Compare
Collaborator
Builds ready [9b0daa7]
UI Startup Metrics (1210 ± 95 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| path={`${CONFIRM_TRANSACTION_ROUTE}/:id?${ENCRYPTION_PUBLIC_KEY_REQUEST_PATH}`} | ||
| element={<ConfirmEncryptionPublicKey />} | ||
| /> | ||
| <Route path="*" element={<ConfirmTransactionSwitch />} /> |
There was a problem hiding this comment.
Bug: Nested Routes: Path Resolution Bug
Nested v5-compat Routes are using absolute paths that include CONFIRM_TRANSACTION_ROUTE prefix, but they should use relative paths since they're already nested within a parent route at /confirm-transaction/:id?. The paths should be DECRYPT_MESSAGE_REQUEST_PATH and ENCRYPTION_PUBLIC_KEY_REQUEST_PATH directly (or with leading slash removed), not ${CONFIRM_TRANSACTION_ROUTE}/:id?${DECRYPT_MESSAGE_REQUEST_PATH}. This causes route matching to fail because v5-compat nested routes match against the remaining path after the parent route, not the full pathname.
Collaborator
Builds ready [64b1ea4]
UI Startup Metrics (1282 ± 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 [24b27ab]
UI Startup Metrics (1248 ± 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 [4e1d964]
UI Startup Metrics (1229 ± 93 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
2 similar comments
Collaborator
Builds ready [4e1d964]
UI Startup Metrics (1229 ± 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 [4e1d964]
UI Startup Metrics (1229 ± 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 [c2d1877]
UI Startup Metrics (1227 ± 89 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
|
| // TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31973 | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| Container?: any, | ||
| confirmationId?: string, |
There was a problem hiding this comment.
Bug: Prevent Default Route Drift
The renderHookWithConfirmContextProvider function has an inconsistent default value for the pathname parameter. It uses '/' as the default while renderWithConfirmContextProvider uses DEFAULT_ROUTE (which equals '/'). More importantly, this inconsistency could cause subtle issues when the default route constant is later changed, as one function would be updated while the other wouldn't be. For consistency with the other helper function and to follow the PR's pattern of using constants throughout, this should be DEFAULT_ROUTE.
Collaborator
Builds ready [975a04a]
UI Startup Metrics (1134 ± 97 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
|
This commit restores the codebase to the working state before the refactoring attempts. All commit history is preserved - this is a regular commit on top of the existing history.
DDDDDanica
requested review from
a team,
HowardBraham and
dbrans
as code owners
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
| } | ||
| return; | ||
| isPasswordSynced = true; | ||
| return isPasswordSynced; |
There was a problem hiding this comment.
Bug: Undefined Return Breaks Password Sync Flow
The syncPasswordAndUnlockWallet method now returns isPasswordSynced in some branches but was previously documented as returning void. However, in the social login flow branch, it returns early without initializing isPasswordSynced, causing it to return undefined instead of a proper boolean value in that case.
Collaborator
Builds ready [f45a154]
UI Startup Metrics (1115 ± 94 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚀 Bundle size reduced!]
|
Contributor
Author
|
Re-open PR with a clean fix history in #37964 changes are lost or corrupted in rebasing . |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Migrate
ui/pages/confirmationtoreact-router-v5-compat.Current Routing Architecture (v5 → v5-compat → v6 Migration):
Changelog
CHANGELOG entry: null
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/5564
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Migrates confirmation flows and related settings/pages to react-router-dom-v5-compat, replacing history-based APIs with navigate/Navigate, updating routes, helpers, and tests accordingly.
useHistory/Redirect/SwitchwithuseNavigate/Navigate/Routesacross confirmations, send, and settings.PREVIOUS_ROUTEand update handlers to use it; standardize home nav viaDEFAULT_ROUTE.routes.component.tsxto bridge v5 → v5-compat viacreateV5CompatRouteand wrap protected routes withAuthenticatedV5Compat.ConfirmTransactionnow accepts v5 props, uses v5-compat routing for nested decrypt/public-key routes, and passesconfirmationIdintoConfirm.ConfirmContextProvideranduseCurrentConfirmationsupport explicitconfirmationId;useSyncConfirmPathsyncs URL when missing:id.ConfirmationPage(templated) switched to v5-compatuseNavigate/useParams; integratesConfirmContextProvider.confirm-transaction-switchand token switch moved to v5-compat; newextractIdFromPathnameutil.useNavigateSendPage,useSendActions,useSendQueryParams,useConfirmSendNavigation, loader/header) to use v5-compat andPREVIOUS_ROUTE.extractIdFromPathnameand tests; extend routes constants; minor header-visibility logic uses new routes.withRouter/composefrom backup-and-sync and experimental tabs; minor JSX fix.render-helpers-navigate; mock v5-compat hooks throughout; add/adjust unit tests for new navigation and utils.Written by Cursor Bugbot for commit 975a04a. This will update automatically on new commits. Configure here.