refactor(7380): convert app/scripts files to TypeScript - #44397
Open
DDDDDanica with Copilot wants to merge 8 commits into
Open
refactor(7380): convert app/scripts files to TypeScript#44397DDDDDanica with Copilot wants to merge 8 commits into
DDDDDanica with Copilot wants to merge 8 commits into
Conversation
Copilot
AI
changed the title
[WIP] Convert app/scripts files to TypeScript
feat: convert app/scripts files to TypeScript
Jul 13, 2026
DDDDDanica
changed the title
DDDDDanica
force-pushed
the
copilot/convert-app-scripts-to-typescript
branch
from
July 13, 2026 19:20
7c5b5f4 to
4542225
Compare
Contributor
Builds ready [4542225]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 11 warn · 🔴 0 fail)
Bundle sizes
|
DDDDDanica
force-pushed
the
copilot/convert-app-scripts-to-typescript
branch
from
July 13, 2026 20:04
4542225 to
d3b982a
Compare
Contributor
Builds ready [d3b982a]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 11 warn · 🔴 0 fail)
Bundle sizes
|
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
Builds ready [ea4c821]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 9 warn · 🔴 1 fail)
Bundle size diffs
|
…cripts-to-typescript
DDDDDanica
force-pushed
the
copilot/convert-app-scripts-to-typescript
branch
from
July 30, 2026 00:35
ea4c821 to
bac5b34
Compare
Contributor
Builds ready [bac5b34]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 8 warn · 🔴 2 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Contributor
Builds ready [7fafda0]
⚡ Performance Benchmarks (Total: 🟢 11 pass · 🟡 12 warn · 🔴 1 fail)
Bundle size diffs
|
DDDDDanica
marked this pull request as ready for review
July 30, 2026 12:29
DDDDDanica
enabled auto-merge
July 30, 2026 12:31
ulissesferreira
previously approved these changes
Jul 30, 2026
MajorLift
requested changes
Jul 30, 2026
| _onWindowClosed(windowId: number) { | ||
| if (windowId === this._popupId) { | ||
| this._setCurrentPopupId(undefined); | ||
| this._setCurrentPopupId?.(undefined); |
Contributor
There was a problem hiding this comment.
This changes behavior, not just types — previously an unset setter threw, now it silently no-ops:
- this._setCurrentPopupId(undefined);
+ this._setCurrentPopupId?.(undefined);I traced it and believe it's currently unreachable — _onWindowClosed only acts when windowId === this._popupId, and _popupId is only set by showPopup, which also sets the setter. So this is a note, not a defect.
Worth calling out only because the ?. was introduced to satisfy the hand-written | undefined above rather than by intent. Deriving the type per the suggestion on :19-21 removes the need for it — a swallowed no-op where a throw used to be is the kind of thing worth not acquiring by accident.
Contributor
There was a problem hiding this comment.
Makes sense. Better to keep the hard call so we don’t quietly change what happens if the setter isn’t set. 1847ffb
Contributor
There was a problem hiding this comment.
! in expression position is blocked by @typescript-eslint/no-non-null-assertion — it's the 107:7 Forbidden non-null assertion failing Test lint at 1847ffb.
It's also pointed at the wrong nullability: ! asserts the field is set, which erases the check rather than enforcing it, so it doesn't deliver the hard call it's there for.
Since the id and the setter are only ever assigned together, the invariant can be structural instead of asserted — group them, and the compiler proves the setter is present wherever the id matched:
diff --git a/app/scripts/lib/notification-manager.ts b/app/scripts/lib/notification-manager.ts
index 85572242f2a..d561ef29b04 100644
--- a/app/scripts/lib/notification-manager.ts
+++ b/app/scripts/lib/notification-manager.ts
@@ -17,9 +17,17 @@ export const NOTIFICATION_MANAGER_EVENTS = {
export default class NotificationManager extends EventEmitter {
platform: ExtensionPlatform;
- _popupId: number | undefined;
-
- _setCurrentPopupId: AppStateController['setCurrentPopupId'] | undefined;
+ /**
+ * Popup bookkeeping. The id and its setter are only ever assigned together,
+ * by `showPopup` — grouping them lets the compiler prove the setter is present
+ * wherever the id matched, instead of asserting it.
+ */
+ _popup:
+ | {
+ id: number | undefined;
+ setCurrentPopupId: AppStateController['setCurrentPopupId'];
+ }
+ | undefined;
_popupAutomaticallyClosed: boolean | undefined;
@@ -50,8 +58,8 @@ export default class NotificationManager extends EventEmitter {
setCurrentPopupId: AppStateController['setCurrentPopupId'],
currentPopupId: number | undefined,
) {
- this._popupId = currentPopupId;
- this._setCurrentPopupId = setCurrentPopupId;
+ const popupState = { id: currentPopupId, setCurrentPopupId };
+ this._popup = popupState;
const popup = await this._getPopup();
// Bring focus to chrome popup
if (popup) {
@@ -96,16 +104,15 @@ export default class NotificationManager extends EventEmitter {
}
// pass new created popup window id to appController setter
// and store the id to private variable this._popupId for future access
- this._setCurrentPopupId(popupWindow.id);
- this._popupId = popupWindow.id;
+ popupState.setCurrentPopupId(popupWindow.id);
+ popupState.id = popupWindow.id;
}
}
_onWindowClosed(windowId: number) {
- if (windowId === this._popupId) {
- // Set alongside `_popupId` in `showPopup`; keep a hard call so an unset setter throws.
- this._setCurrentPopupId!(undefined);
- this._popupId = undefined;
+ if (this._popup && windowId === this._popup.id) {
+ this._popup.setCurrentPopupId(undefined);
+ this._popup.id = undefined;
this.emit(NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED, {
automaticallyClosed: this._popupAutomaticallyClosed,
});
@@ -134,7 +141,7 @@ export default class NotificationManager extends EventEmitter {
return windows
? windows.find((win) => {
// Returns notification popup
- return win && win.type === 'popup' && win.id === this._popupId;
+ return win && win.type === 'popup' && win.id === this._popup?.id;
})
: null;
}That's a real patch against 1847ffb, not an excerpt — copy the block and pbpaste | git apply from the repo root. (I'd have used a suggestion block, but this thread is outdated at the current head, so GitHub disables Apply on it.)
showPopup's signature is unchanged, so background.js and notification-manager.test.ts need no edits. Verified before posting: full-project tsc --noEmit clean, and all 3 tests in notification-manager.test.ts pass.
Two deliberate details: holding the local popupState rather than re-reading this._popup after the await means the post-create branch needs no re-check; and on close it clears _popup.id rather than the whole record, so a second close stays a no-op instead of throwing.
Worth ruling out explicitly: assert does not fix this. It's the natural next reach once ! is blocked, and it's already used in four files under app/scripts:
import { assert } from '@metamask/utils';
assert(this._setCurrentPopupId);
this._setCurrentPopupId(undefined);It narrows via asserts value and is lint-clean, but an AssertionError is still a synthetic error with its own fingerprint — same objection as the hand-written throw, different syntax.
Superseded by a comment anchored on 85997a4 — the patch above was generated against 1847ffb and no longer applies cleanly.
Contributor
There was a problem hiding this comment.
Moved to a comment anchored on 85997a4, where the change actually landed: #44397 (comment)
| */ | ||
| function deepMap(target = {}, visit) { | ||
| function deepMap( | ||
| target: Record<string, unknown>, |
Contributor
There was a problem hiding this comment.
The = {} default is gone: this was function deepMap(target = {}, visit).
With the parameter now non-optional and the caller in JS, nothing checks it — getObjStructure(undefined) would reach Object.entries(undefined) and throw.
Suggested change
| target: Record<string, unknown>, | |
| target: Record<string, unknown> = {}, |
I traced reachability and it's inert today: by the time migrator.on('error') can fire (background.js:1095), the :1087-1090 fallback guarantees an object; and for the null that persistenceManager.get() actually returns, the default never applied in the old code either.
So no bug — but this sits in the Sentry migration-error reporter, where a throw would replace a migration report with a TypeError, i.e. the failure would be invisible exactly when you most need the report. Restoring the default also unblocks the suggestion on :28-30.
Contributor
There was a problem hiding this comment.
Good call. Even if it’s inert today, that default is cheap insurance on the migration error path. Addressed in 1847ffb
Contributor
There was a problem hiding this comment.
My suggestion was wrong — sorry.
Restoring = {} puts a default parameter before a required one, which is fine in JS but an error under @typescript-eslint/default-param-last.
Guarding at the call site keeps the protection, drops the default, and lets the cast go too:
export default function getObjStructure(
obj: MetaMaskStorageStructure | undefined,
): Record<string, unknown> {
const structure = cloneDeep(obj);
- return deepMap(structure as Record<string, unknown> | undefined, (value) => {
+ return deepMap(structure ?? {}, (value) => {
return value === null ? 'null' : typeof value;
});
}
function deepMap(
- target: Record<string, unknown> = {},
+ target: Record<string, unknown>,
visit: (value: unknown) => string,
): Record<string, unknown> {Typechecked against the real MetaMaskStorageStructure before posting this time — structure ?? {} satisfies Record<string, unknown> on its own, so the assertion isn't needed.
MajorLift
reviewed
Jul 30, 2026
Contributor
There was a problem hiding this comment.
For the TypeScript migrations to be useful for preventing stability bugs and not compromise type safety instead, deriving and inferring types wherever possible instead of hardcoding/duplicating them in-line is arguably as important as avoiding any:
Types MUST be derived from existing, authoritative sources wherever possible instead of hardcoded in-line or duplicated ad-hoc in order to prevent code drift.
-- https://github.com/MetaMask/MetaMask-planning/issues/5284#:~:text=guide%20our%20work.-,TypeScript%20Policies,-It%27s%20crucial%20that
- A hand-written type that restates an existing one duplicates the shape: every reader must reconcile the two, and every change has to touch both.
- The restatement is a guess at the source's shape, and the guess is almost always wider than the real type — it admits values the authoritative type would reject (see Avoid unintentionally widening an inferred type with a type annotation). Marking every field optional is the most common way this happens.
- Because the copy is hard-coded rather than derived, it is brittle against code drift (see Prefer type inference over annotations and assertions): when the source evolves the copy silently goes stale, and the compiler cannot flag the divergence, so the failure surfaces at runtime rather than at build.
-- TypeScript guidelines:
Derive types from authoritative sources instead of re-declaring them
A conversion is also not licensed to change what the program does — the runtime edits that look like typing work are the other half:
- A literal replaced by a runtime lookup.
['IFRAME_SCRIPTING']becoming[SomeApi.Reason.IFRAME_SCRIPTING]adds a dependency on that object existing at runtime.- A default parameter or fallback deleted.
function f(x = {})→function f(x: T)removes a guard. Ask what the guard was for.- A call made optional.
obj.method()→obj.method?.(), added to satisfy a hand-written| undefined, converts a throw into a silent no-op.- A widened local to keep a check alive. If the runtime check is genuinely needed, the input type is wrong.
--
compiler-blindspots:A typing change should not change runtime behavior
MajorLift
reviewed
Jul 30, 2026
Contributor
🧪 Validation RunVerdict: What would have falsified the claim, and what turned up
The findings are the 12 inline comments in the review above — not restated here. MethodTwo arms at the same commit — no rebase, no merge boundary:
A further probe compiled the original Why this doesn't show up in CI: Cleared, not silently droppedFive claims the probes refuted — listed because a sweep that only ever confirms is indistinguishable from one that never isolated anything:
Method and rationale: |
Contributor
Builds ready [1847ffb]
⚡ Performance Benchmarks (Total: 🟢 15 pass · 🟡 9 warn · 🔴 0 fail)
Bundle size diffs [🚀 Bundle size reduced!]
|
DDDDDanica
force-pushed
the
copilot/convert-app-scripts-to-typescript
branch
from
July 30, 2026 18:36
939dae9 to
85997a4
Compare
|
Contributor
Builds ready [85997a4]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 7 warn · 🔴 3 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
MajorLift
reviewed
Jul 30, 2026
Comment on lines
+106
to
+111
| // Set alongside `_popupId` in `showPopup`; throw if the setter was never wired. | ||
| const setCurrentPopupId = this._setCurrentPopupId; | ||
| if (!setCurrentPopupId) { | ||
| throw new Error('NotificationManager: setCurrentPopupId was never set'); | ||
| } | ||
| setCurrentPopupId(undefined); |
Contributor
There was a problem hiding this comment.
This pollutes the error that bubbles to Sentry.
new Error('NotificationManager: setCurrentPopupId was never set') replaces the native TypeError with a synthetic one, which isn't neutral just because both still bubble. Sentry fingerprints on exception type + top frames + message, so the hand-written one opens a new issue with no history — the trend line, first-seen date, and regression detection all reset, and any ignore rule, sampling rule, alert condition, or ownership routing written against the old shape stops matching. The stack origin moves too: a raw error's top frame is the offending call, while a guard-thrown one points at the guard.
The guard also can't be reached. _onWindowClosed only enters when windowId === this._popupId; _popupId becomes a number only at showPopup's first line, with the setter assigned on the very next statement and no await between them. So inside this branch the setter is always set — this adds a runtime check and a fresh Sentry fingerprint for a state that cannot occur.
Worth noting the throw isn't free even as dead code: if it ever did fire it would abort the rest of _onWindowClosed — the _popupId reset, the POPUP_CLOSED emit, the _popupAutomaticallyClosed reset — so a listener would silently stop hearing about closed popups.
Grouping the id and the setter makes the check unnecessary rather than cheaper. The compiler proves the setter is present wherever the id matched, so there's nothing to guard and no error to substitute:
diff --git a/app/scripts/lib/notification-manager.ts b/app/scripts/lib/notification-manager.ts
index f2e8e5e6687..a23ef5c6879 100644
--- a/app/scripts/lib/notification-manager.ts
+++ b/app/scripts/lib/notification-manager.ts
@@ -17,9 +17,17 @@ export const NOTIFICATION_MANAGER_EVENTS = {
export default class NotificationManager extends EventEmitter {
platform: ExtensionPlatform;
- _popupId: number | undefined;
-
- _setCurrentPopupId: AppStateController['setCurrentPopupId'] | undefined;
+ /**
+ * Popup bookkeeping. The id and its setter are only ever assigned together,
+ * by `showPopup` — grouping them lets the compiler prove the setter is present
+ * wherever the id matched, instead of asserting or re-checking it.
+ */
+ _popup:
+ | {
+ id: number | undefined;
+ setCurrentPopupId: AppStateController['setCurrentPopupId'];
+ }
+ | undefined;
_popupAutomaticallyClosed: boolean | undefined;
@@ -50,8 +58,8 @@ export default class NotificationManager extends EventEmitter {
setCurrentPopupId: AppStateController['setCurrentPopupId'],
currentPopupId: number | undefined,
) {
- this._popupId = currentPopupId;
- this._setCurrentPopupId = setCurrentPopupId;
+ const popupState = { id: currentPopupId, setCurrentPopupId };
+ this._popup = popupState;
const popup = await this._getPopup();
// Bring focus to chrome popup
if (popup) {
@@ -96,20 +104,15 @@ export default class NotificationManager extends EventEmitter {
}
// pass new created popup window id to appController setter
// and store the id to private variable this._popupId for future access
- this._setCurrentPopupId(popupWindow.id);
- this._popupId = popupWindow.id;
+ popupState.setCurrentPopupId(popupWindow.id);
+ popupState.id = popupWindow.id;
}
}
_onWindowClosed(windowId: number) {
- if (windowId === this._popupId) {
- // Set alongside `_popupId` in `showPopup`; throw if the setter was never wired.
- const setCurrentPopupId = this._setCurrentPopupId;
- if (!setCurrentPopupId) {
- throw new Error('NotificationManager: setCurrentPopupId was never set');
- }
- setCurrentPopupId(undefined);
- this._popupId = undefined;
+ if (this._popup && windowId === this._popup.id) {
+ this._popup.setCurrentPopupId(undefined);
+ this._popup.id = undefined;
this.emit(NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED, {
automaticallyClosed: this._popupAutomaticallyClosed,
});
@@ -138,7 +141,7 @@ export default class NotificationManager extends EventEmitter {
return windows
? windows.find((win) => {
// Returns notification popup
- return win && win.type === 'popup' && win.id === this._popupId;
+ return win && win.type === 'popup' && win.id === this._popup?.id;
})
: null;
}Real patch against 85997a4, not an excerpt — copy the block and pbpaste | git apply from the repo root. (A suggestion block would be nicer, but this thread is outdated at the current head so GitHub disables Apply on it.)
showPopup's signature is unchanged, so background.js and notification-manager.test.ts need no edits. Verified before posting: full-project tsc --noEmit clean, 3/3 tests in notification-manager.test.ts pass, patch applies cleanly.
Two deliberate details: holding the local popupState rather than re-reading this._popup after the await means the post-create branch needs no re-check; and on close it clears _popup.id rather than the whole record, so a second close stays a no-op instead of throwing.
Worth ruling out explicitly: assert does not fix this. It's the natural next reach once ! is blocked — it's already used in four files under app/scripts, narrows via asserts value, and is lint-clean:
import { assert } from '@metamask/utils';
assert(this._setCurrentPopupId);
this._setCurrentPopupId(undefined);But an AssertionError is still a synthetic error with its own fingerprint, so it pollutes the Sentry stream exactly the way the hand-written throw does. It clears the lint error without addressing what the lint error was pointing at, and it still asserts the invariant rather than proving it. Same objection, different syntax.
MajorLift
reviewed
Jul 30, 2026
| function deepMap(target = {}, visit) { | ||
| Object.entries(target).forEach(([key, value]) => { | ||
| function deepMap( | ||
| target: Record<string, unknown> | undefined, |
Contributor
There was a problem hiding this comment.
This changes behavior on null, silently.
deepMap(null) used to throw (Object.entries(null)); it now returns {}. ?? catches null, the = {} default did not — and the parameter type still says | undefined, so nothing in the signature records that the nullable surface grew.
null isn't hypothetical here: BaseStore.get() is typed Promise<MetaMaskStorageStructure | null>, so it is a real value in this pipeline. It's unreachable today — background.js:1087-1090 replaces an empty value before the migrator can emit error — and I think the widening is right for a Sentry error reporter, since throwing here would replace a migration error report with a TypeError. Flagging it because it happened without being declared, not because it's wrong.
(The undefined half is genuinely a non-change: target = {} already accepted undefined and substituted {}, so that part of the new signature just states what the default parameter was already doing.)
The underlying mismatch is that the declared nullability is the wrong one. The producer emits | null; the signature says | undefined, following the JSDoc on preMigrationVersionedData — which is itself only half right, since that variable is let-declared so it starts undefined, then gets assigned from get() which returns null. Matching the real shape also lets the cast go:
export default function getObjStructure(
- obj: MetaMaskStorageStructure | undefined,
+ obj: MetaMaskStorageStructure | null | undefined,
): Record<string, unknown> {
const structure = cloneDeep(obj);
- return deepMap(structure as Record<string, unknown> | undefined, (value) => {
+ return deepMap(structure, (value) => {
return value === null ? 'null' : typeof value;
});
}
function deepMap(
- target: Record<string, unknown> | undefined,
+ target: Record<string, unknown> | null | undefined,
visit: (value: unknown) => string,
): Record<string, unknown> {The as Record<string, unknown> | undefined is unnecessary either way — I compiled it both with and without: MetaMaskStorageStructure is a type alias, so it gets an implicit index signature and is assignable to Record<string, unknown> with no assertion. As written the cast also asserts away the exact null case the ?? exists to handle.
Continues the thread on the previous head, where the = {} default came from a suggestion of mine that turned out to trip @typescript-eslint/default-param-last — this comment is anchored on 85997a4 because that's where the current shape landed.
Contributor
|
Running this skill on changes should catch drifted types, runtime behavior changes etc:
Feedback welcome on the PR itself as well. |
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 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.
Converts 11 JavaScript files in
app/scriptsto TypeScript as part of the ongoing TS migration (MetaMask/MetaMask-planning#7380). Colocated test forcleanErrorStackis also converted.Files converted
app/scripts/first-time-state.ts— addedFirstTimeStatetypeapp/scripts/disable-console.ts— rename onlyapp/scripts/sentry-install.ts— rename only (globals already declared intypes/global.d.ts)app/scripts/lib/extractEthjsErrorMessage.ts— typedstringparam/returnapp/scripts/lib/cleanErrorStack.ts+.test.ts— typedErrorparam/return; test usesas anycast for the intentionalundefinedname assignmentapp/scripts/controllers/permissions/index.ts— rename only (re-exports already-typed TS files)app/scripts/lib/getObjStructure.ts— typed withRecord<string, unknown>app/scripts/offscreen.ts— typed listener callback,Promise<void>, connectivity callback; added?? 0for optionalbrowser.Windows.Windowproperties (top?,left?,width?)app/scripts/lib/ens-ipfs/resolver.ts— introduced localEthProvidertype for the provider parameterapp/scripts/lib/ens-ipfs/setup.ts— typed setup options object and inner function parametersapp/scripts/lib/notification-manager.ts— added class property declarations, typed method signatures, used optional chaining on_setCurrentPopupIdOther changes
development/ts-migration-dashboard/files-to-convert.jsonreuqest→request,unrecongized→unrecognizedNote
Low Risk
Mostly type annotations and file renames with small behavioral hardening (popup id clearing, optional window bounds); no auth or transaction logic changes.
Overview
Continues the
app/scriptsTypeScript migration by converting several background helpers from JavaScript to.ts, updating the migration dashboard, and adding ambient module types for ENS/IPFS dependencies.Popup / notification wiring:
AppStateController.setCurrentPopupIdnow acceptsnumber | undefinedso the active notification popup id can be cleared on close, with a matching unit test.NotificationManageris fully typed, clears app state viasetCurrentPopupId(undefined)when the popup window closes (and throws if that setter was never wired), and uses?? 0for optional Chrome window geometry when positioning new popups.Typed utilities:
cleanErrorStack,extractEthjsErrorMessage,getObjStructure,first-time-state, ENS/IPFSresolver/setup, andoffscreengain explicit TypeScript signatures (includingEthProvideraligned withNetworkControllerand safer handling whenclientsis missing pre–Chrome 116).Housekeeping: Removed the converted paths from
development/ts-migration-dashboard/files-to-convert.json; addeddeclare moduleentries foreth-ens-namehashand@ensdomains/content-hash; fixed comment typos (request,unrecognized).Reviewed by Cursor Bugbot for commit 85997a4. Bugbot is set up for automated code reviews on this repo. Configure here.