Skip to content

feat(ApplicationState): init controller#7808

Open
Kriys94 wants to merge 2 commits intomainfrom
feature/ApplicationState
Open

feat(ApplicationState): init controller#7808
Kriys94 wants to merge 2 commits intomainfrom
feature/ApplicationState

Conversation

@Kriys94
Copy link
Contributor

@Kriys94 Kriys94 commented Feb 2, 2026

Explanation

Current state: Application lifecycle management (knowing when the UI is open or closed) is currently scattered across platform-specific code (Extension and Mobile). When the UI state changes, MetamaskController must manually notify each controller/service that cares about this state via imperative calls.

Problem: This approach doesn't scale well. Every time a new controller needs to respond to client state changes (stop polling, disconnect WebSockets, pause subscriptions), the platform code has to be modified to add another manual call. Platform code becomes tightly coupled to controller-specific behavior.

Solution: This PR introduces @metamask/application-state-controller, a new shared controller that centralizes application lifecycle state. The pattern follows an inversion of control approach:

  1. Platform code calls a single messenger action: ApplicationStateController:setClientState
  2. Consumer controllers subscribe to ApplicationStateController:stateChange and manage themselves
  3. Platform code no longer needs controller-specific logic (e.g., starting/stopping polling, connecting/disconnecting WebSockets) — controllers handle this internally by reacting to state changes

This enables polling controllers to stop when the client closes, WebSocket connections to disconnect, and real-time subscriptions to pause—all without modifying platform code, but with all the logic encapsulated in core.

State Properties:

  • isClientOpen: boolean - Whether the client (UI) is currently open (not persisted, always starts as false)

Messenger API:

  • Action: ApplicationStateController:setClientState - Called by platform code
  • Event: ApplicationStateController:stateChange - Subscribed to by consumer controllers

References

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Mostly additive and isolated, but it introduces a new cross-cutting lifecycle signal (setClientOpen/stateChange) that downstream controllers may rely on, and the public type export in src/index.ts appears to reference a non-existent ApplicationStateControllerSetClientStateAction (potential build/typing break).

Overview
Adds a new package, @metamask/application-state-controller, providing an ApplicationStateController that tracks a non-persisted isClientOpen lifecycle flag and exposes it via messenger (${controllerName}:getState, ${controllerName}:setClientOpen) plus the standard ${controllerName}:stateChange event.

Includes full package scaffolding (TS build config, Jest tests with 100% thresholds, Typedoc config, README/CHANGELOG/LICENSE), registers ownership in CODEOWNERS/teams.json, and wires the workspace into yarn.lock.

Written by Cursor Bugbot for commit 06a874b. This will update automatically on new commits. Configure here.

@Kriys94 Kriys94 force-pushed the feature/ApplicationState branch 3 times, most recently from ca7d2e8 to c2404be Compare February 2, 2026 14:24
@Kriys94 Kriys94 force-pushed the feature/ApplicationState branch from c2404be to 7ff3a8a Compare February 2, 2026 15:02
@Kriys94 Kriys94 marked this pull request as ready for review February 2, 2026 15:48
@Kriys94 Kriys94 requested a review from a team as a code owner February 2, 2026 15:48
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

controller.setClientState(true);

expect(listener).not.toHaveBeenCalled();
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate test cases testing identical behavior

Low Severity

Tests "does not update state when setting the same value" (lines 107-116) and "does not publish stateChange when state does not change" (lines 143-152) are functionally identical. Both set state to true, subscribe a listener, call setClientState(true) again, and assert the listener wasn't called. One of these tests should be removed.

Fix in Cursor Fix in Web

@mcmire
Copy link
Contributor

mcmire commented Feb 2, 2026

@Kriys94

I plan on reviewing this but first I want to better understand the problems you see this controller solving and how you anticipate it being used.

In the explanation for this PR, you say that as new controllers with data subscriptions are added, clients must be updated to inform those controllers to start when the UI is active and stop when the UI is not. You say that this isn't scalable because this code is scattered throughout the clients. Furthermore, in the technical proposal you created earlier, you seem to say that the UI should not be responsible for managing data subscriptions, the controller layer should. This is further implied in this PR when suggesting that with the existence of an ApplicationStateController, other controllers can now be aware of when the UI is open and automatically start and stop data subscriptions.

First, I don't think it is bad that there is some code in the UI which controls when a data subscription is started or stopped. After all, not every screen needs every piece of data we use throughout MetaMask, so it seems perfectly fine to me to place that control at the React component level (i.e., "when this component mounts, start polling or open a WebSocket; when this components unmounts, stop").

My sense is that the problem we're trying to solve here is not how to handle what happens if a user switches away from a screen, but how to handle when the app (whether that's the mobile app, or a tab in a browser) is backgrounded or foregrounded. Here, the user is still on a screen — so no component is unmounted — but the user is not actively using the app. And in this case, it seems to me that if the current screen is actively polling for data or has a WebSocket open, then that data subscription needs to be paused, so that when the user returns, it can be resumed.

In other words, alongside this controller I wonder if we ought to update the pattern that we've been using so that polling controllers, WebSocket services, and other places have both a pause and a resume method in addition to start and stop. (resume would be like start, but the subscription would only be activated if it was previously paused; otherwise nothing would happen. And pause would merely track the state of the subscription before stopping.)

I think reframing this controller — and the examples presented in places like the README — in this way would make more sense to me, but what do you think?

@Kriys94
Copy link
Contributor Author

Kriys94 commented Feb 3, 2026

First, I don't think it is bad that there is some code in the UI which controls when a data subscription is started or stopped. After all, not every screen needs every piece of data we use throughout MetaMask, so it seems perfectly fine to me to place that control at the React component level (i.e., "when this component mounts, start polling or open a WebSocket; when this components unmounts, stop").

Currently, when you look at the core codebase, you don't have a complete picture of how/when some code is executed. This is extremely hard to understand the whole workflow when you don't know that some logic lives in the UI.
This is especially useful when the application state is used in multiple places.

To me, it's the same utility as when we switch accounts: we have a state and events that we can listen to in any Controller, with no UI code/hooks needed. Thanks to this App State Event, we could consider removing all polling logic from the extension and mobile, which represents quite some code to clean, but ultimately will make platform code lighter.

@mcmire
Copy link
Contributor

mcmire commented Feb 4, 2026

.@Kriys94 and I spoke earlier today about this. For completeness here is more or less a summary of what I said:

Implications on polling controllers

To be clear, I don't think this controller is not needed. I can see the value in encapsulating the concept of whether the user is currently using MetaMask so that we can access that information in an agnostic fashion.

However, there are caveats around how this controller ought to be used that I think should be highlighted or at least addressed somewhere. Namely, when it comes to polling controllers, I don't know that we would want to suggest that engineers can update their controllers to start polling when MetaMask is active and stop polling when it's inactive, e.g.:

this.#messenger.subscribe(
  'ApplicationStateController:stateChange',
  (isClientOpen) => {
    if (isClientOpen) {
      this.#start();
    } else {
      this.#stop();
    }
  },
  (state) => state.isClientOpen
);

This pattern implies that we would be moving polling management to a more global location: as soon as MetaMask opens, no matter what screen the user is on, we start subscribing to updates for all kinds of data — even for ones that the current screen doesn't need. I don't feel like this is a good strategy long-term. We've gotten in trouble in the past about making network requests before users complete onboarding and I feel like we would run into that again if we went that direction.

That's why I suggested continuing to have the UI drive which polls are active, but then introducing the idea of "pausing" and "resuming", for instance:

this.#messenger.subscribe(
  'ApplicationStateController:stateChange',
  (isClientOpen) => {
    if (isClientOpen) {
      this.#resume();
    } else {
      this.#pause();
    }
  },
  (state) => state.isClientOpen
);

If we are providing examples for how to use this controller in this PR, then I think we should suggest that engineers follow these practices.

Naming

The other thing I mentioned in the call is around naming:

  1. The extension repo already has an AppStateController, so introducing an ApplicationStateController could get confusing.
  2. The state property is called isClientOpen. This could be possibly inconsistent and confusing depending on perspective.

I think the name ApplicationStateController is fine. It's a bit generic, but I can imagine having one or two other state properties that we might want to track that don't really belong anywhere else. Maybe we can plan to rename AppStateController in extension to ExtensionApplicationStateController or something, though?

I also wonder if we should rename the state property? I am fine with the word "open". On extension, it could reasonably mean that the popup is open, or the full-screen view is open (regardless of whether the tab is active), or the sidepanel is open. And on mobile, it would mean that the app is foregrounded. But, "client" seems like a synonym for "application" to me. So maybe we should rename the state property either isApplicationOpen or isUiOpen?

*
* @returns A messenger for the controller.
*/
function createMessenger(): ApplicationStateControllerMessenger {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having one function that sets up both the root messenger and controller-specific messenger, what are your thoughts on creating two functions? See the tests for SampleGasPricesController for more:

function getRootMessenger(): RootMessenger {

* @param options.state - Initial state to set on the controller.
* @returns The controller and messenger.
*/
function createController(options?: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having this function take the controller options, what do you think about having it take an options bag (and one of the options is the set of controller options)? Alternatively, what do you think about using the withController pattern? See here:

async function withController<ReturnValue>(

});

expect(controller.state.isClientOpen).toBe(true);
expect(controller.isClientOpen).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the controller also have a getter for isClientOpen? I wonder if we need this, when we also have a selector.

});

describe('setClientState', () => {
it('updates state when client opens', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are you opening the client in this test? What does that mean?

Maybe this should read:

Suggested change
it('updates state when client opens', () => {
it('updates isClientOpen in state to the given value', () => {

Comment on lines 107 to 116
it('does not update state when setting the same value', () => {
const { controller, messenger } = createController();
controller.setClientState(true);
const listener = jest.fn();
messenger.subscribe(`${controllerName}:stateChange`, listener);

controller.setClientState(true);

expect(listener).not.toHaveBeenCalled();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseController already ensures that if you attempt to update state but it results in no changes, no state change event will occur. So do we need this test?

Suggested change
it('does not update state when setting the same value', () => {
const { controller, messenger } = createController();
controller.setClientState(true);
const listener = jest.fn();
messenger.subscribe(`${controllerName}:stateChange`, listener);
controller.setClientState(true);
expect(listener).not.toHaveBeenCalled();
});

{
"name": "@metamask/application-state-controller",
"version": "0.0.0",
"description": "Manages application lifecycle state (client open/closed) for cross-platform MetaMask applications",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Everything in core is designed to be used in a cross-platform way. Maybe we can be a bit more generic too in case we introduce other state properties later?

Suggested change
"description": "Manages application lifecycle state (client open/closed) for cross-platform MetaMask applications",
"description": "Tracks and manages the lifecycle state of MetaMask as an application.",

@@ -0,0 +1,219 @@
# `@metamask/application-state-controller`

Manages application lifecycle state (client open/closed) for cross-platform MetaMask applications.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Everything in core is designed to be used in a cross-platform way. Maybe we can be a bit more generic too in case we introduce other state properties later?

Suggested change
Manages application lifecycle state (client open/closed) for cross-platform MetaMask applications.
Tracks and manages the lifecycle state of MetaMask as an application.

Comment on lines +108 to +116
AppState.addEventListener('change', (nextAppState) => {
if (nextAppState !== 'active' && nextAppState !== 'background') {
return;
}
controllerMessenger.call(
'ApplicationStateController:setClientOpen',
nextAppState === 'active',
);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't need the conditional up front?

Suggested change
AppState.addEventListener('change', (nextAppState) => {
if (nextAppState !== 'active' && nextAppState !== 'background') {
return;
}
controllerMessenger.call(
'ApplicationStateController:setClientOpen',
nextAppState === 'active',
);
});
AppState.addEventListener('change', (state) => {
controllerMessenger.call(
'ApplicationStateController:setClientOpen',
state === 'active',
);
});

Comment on lines +129 to +145
(newState) => {
if (newState.isClientOpen) {
this.startPolling();
} else {
this.stopPolling();
}
},
);
}

startPolling() {
// Start polling when client opens
}

stopPolling() {
// Stop polling when client closes
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make sure to at least use a selector, but also, as suggested elsewhere, it might be good to suggest that polling be paused and resumed rather than stopped and started:

Suggested change
(newState) => {
if (newState.isClientOpen) {
this.startPolling();
} else {
this.stopPolling();
}
},
);
}
startPolling() {
// Start polling when client opens
}
stopPolling() {
// Stop polling when client closes
}
(isClientOpen) => {
if (isClientOpen) {
this.resumePolling();
} else {
this.pausePolling();
}
},
applicationStateControllerSelectors.selectIsClientOpen,
);
}
resumePolling() {
// Start polling if previously paused, otherwise do nothing
}
pausePolling() {
// Mark whether polling is currently running for `resumePolling` later
// and ensure that polling is stopped
}

Comment on lines +159 to +166
'ApplicationStateController:stateChange',
(newState) => {
if (newState.isClientOpen) {
this.connect();
} else {
this.disconnect();
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar as above — we should use a selector:

Suggested change
'ApplicationStateController:stateChange',
(newState) => {
if (newState.isClientOpen) {
this.connect();
} else {
this.disconnect();
}
},
(isClientOpen) => {
if (isClientOpen) {
this.connect();
} else {
this.disconnect();
}
},
applicationStateControllerSelectors.selectIsClientOpen,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants