fix(migrator): prevent crash by throwing AggregateError instead of mutating existing error
#39582
+84
−7
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
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
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. |
davidmurdoch
changed the title
AggregateError instead of mutating existing error
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in the migrator by switching from mutating error messages directly to using AggregateError to wrap errors with additional context. The original issue (#39567) involved a TypeError when attempting to set the message property of a DOMException, which has a read-only message getter.
Changes:
- Modified error handling in migrator to use
AggregateErrorinstead of mutating error messages - Updated existing test to verify
AggregateErroremission instead of checking for rewritten error messages - Added test for v2 migration flow with changed controller tracking
- Added regression test for clone failures that produce read-only error objects like
DOMException
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/scripts/lib/migrator/index.js | Updated error handling to wrap errors in AggregateError instead of mutating the original error's message property |
| app/scripts/lib/migrator/index.test.js | Updated tests to assert AggregateError behavior, added v2 migration test with controller tracking, and added regression test for structuredClone failures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
davidmurdoch
commented
Jan 27, 2026
Comment on lines
+118
to
+128
| migrator.on('error', onError); | ||
|
|
||
| const initialState = { meta: { version: 0 }, data: { hello: 'world' } }; | ||
| const migratedData = await migrator.migrateData(initialState); | ||
|
|
||
| expect(onError).toHaveBeenCalledTimes(1); | ||
| const [error] = onError.mock.calls[0]; | ||
| expect(error).toBeInstanceOf(AggregateError); | ||
| expect(error.message).toBe('MetaMask Migration Error #1'); | ||
| expect(error.errors[0].message).toBe('test'); | ||
| expect(migratedData.state).toBe(initialState); |
Contributor
Author
There was a problem hiding this comment.
The shape of the error is different now.
This DOES effect migration error display sentry.
davidmurdoch
commented
Jan 27, 2026
| expect(migratedData.state).toBe(initialState); | ||
| }); | ||
|
|
||
| it('runs v2 migrations and reports changed controllers', async () => { |
Contributor
Author
There was a problem hiding this comment.
specifically testing the v2 migration flow here
Co-authored-by: Copilot <[email protected]>
davidmurdoch
had a problem deploying
to
pr-comment
GitHub Actions
Error
davidmurdoch
commented
Jan 27, 2026
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [93e7590]
UI Startup Metrics (1336 ± 120 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
…sk/metamask-extension into fix-migration-error-catch
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [14c66fd]
UI Startup Metrics (1309 ± 128 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
davidmurdoch
had a problem deploying
to
pr-comment
GitHub Actions
Error
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
Contributor
Builds ready [77d5acd]
UI Startup Metrics (1289 ± 97 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs
|
davidmurdoch
temporarily deployed
to
pr-comment
GitHub Actions
Inactive
itsyoboieltr
approved these changes
Jan 30, 2026
DDDDDanica
approved these changes
Jan 30, 2026
github-project-automation
bot
moved this from Needs dev review
to Review finalised - Ready to be merged
in PR review queue
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 30, 2026
…mutating existing error (#39582) ## **Description** We switched migrator error reporting to use AggregateError instead of rewriting the original error message. Problem: Existing migrator tests still expected the old error rewrite behavior, and didn’t cover v2 migration flow or clone failures. Solution: Update migrator unit tests to assert AggregateError emission, verify v2 migration behavior and changed controller reporting, and add a regression test for clone failures (issue 39567). [](https://codespaces.new/MetaMask/metamask-extension/pull/39582?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Partially Addresses: #39567 <!-- ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Moderate risk because it changes the shape/type of errors emitted from the migrator, which may affect any callers/listeners that inspect error messages or instances. Core migration flow remains the same and is covered by expanded unit tests, reducing regression risk. > > **Overview** > **Migrator error reporting now wraps failures in `AggregateError`** (instead of mutating the original error message) and emits that wrapped error via the `error` event. > > **Tests were updated and expanded** to assert the new emitted error shape, verify v2 migration behavior (including `changedKeys` tracking), and add a regression case for `structuredClone`/`DataCloneError` failures. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 77d5acd. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Copilot <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 30, 2026
…mutating existing error (#39582) ## **Description** We switched migrator error reporting to use AggregateError instead of rewriting the original error message. Problem: Existing migrator tests still expected the old error rewrite behavior, and didn’t cover v2 migration flow or clone failures. Solution: Update migrator unit tests to assert AggregateError emission, verify v2 migration behavior and changed controller reporting, and add a regression test for clone failures (issue 39567). [](https://codespaces.new/MetaMask/metamask-extension/pull/39582?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Partially Addresses: #39567 <!-- ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Moderate risk because it changes the shape/type of errors emitted from the migrator, which may affect any callers/listeners that inspect error messages or instances. Core migration flow remains the same and is covered by expanded unit tests, reducing regression risk. > > **Overview** > **Migrator error reporting now wraps failures in `AggregateError`** (instead of mutating the original error message) and emits that wrapped error via the `error` event. > > **Tests were updated and expanded** to assert the new emitted error shape, verify v2 migration behavior (including `changedKeys` tracking), and add a regression case for `structuredClone`/`DataCloneError` failures. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 77d5acd. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Copilot <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
metamaskbot
added
the
release-13.18.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
](https://codespaces.new/MetaMask/metamask-extension/pull/39582?quickstart=1){kind=link}
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
We switched migrator error reporting to use AggregateError instead of rewriting the original error message.
Problem:
Existing migrator tests still expected the old error rewrite behavior, and didn’t cover v2 migration flow or clone failures.
Solution:
Update migrator unit tests to assert AggregateError emission, verify v2 migration behavior and changed controller reporting, and add a regression test for clone failures (issue 39567).
Changelog
CHANGELOG entry: null
Related issues
Partially Addresses: #39567
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Moderate risk because it changes the shape/type of errors emitted from the migrator, which may affect any callers/listeners that inspect error messages or instances. Core migration flow remains the same and is covered by expanded unit tests, reducing regression risk.
Overview
Migrator error reporting now wraps failures in
AggregateError(instead of mutating the original error message) and emits that wrapped error via theerrorevent.Tests were updated and expanded to assert the new emitted error shape, verify v2 migration behavior (including
changedKeystracking), and add a regression case forstructuredClone/DataCloneErrorfailures.Written by Cursor Bugbot for commit 77d5acd. This will update automatically on new commits. Configure here.