Conversation
ca7d2e8 to
c2404be
Compare
c2404be to
7ff3a8a
Compare
| controller.setClientState(true); | ||
|
|
||
| expect(listener).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
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.
|
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 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? |
packages/application-state-controller/src/ApplicationStateController.ts
Outdated
Show resolved
Hide resolved
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. 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. |
|
.@Kriys94 and I spoke earlier today about this. For completeness here is more or less a summary of what I said: Implications on polling controllersTo 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. NamingThe other thing I mentioned in the call is around naming:
I think the name 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 |
| * | ||
| * @returns A messenger for the controller. | ||
| */ | ||
| function createMessenger(): ApplicationStateControllerMessenger { |
There was a problem hiding this comment.
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:
| * @param options.state - Initial state to set on the controller. | ||
| * @returns The controller and messenger. | ||
| */ | ||
| function createController(options?: { |
There was a problem hiding this comment.
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:
| }); | ||
|
|
||
| expect(controller.state.isClientOpen).toBe(true); | ||
| expect(controller.isClientOpen).toBe(true); |
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
Where are you opening the client in this test? What does that mean?
Maybe this should read:
| it('updates state when client opens', () => { | |
| it('updates isClientOpen in state to the given value', () => { |
| 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(); | ||
| }); |
There was a problem hiding this comment.
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?
| 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", |
There was a problem hiding this comment.
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?
| "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. | |||
There was a problem hiding this comment.
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?
| Manages application lifecycle state (client open/closed) for cross-platform MetaMask applications. | |
| Tracks and manages the lifecycle state of MetaMask as an application. |
| AppState.addEventListener('change', (nextAppState) => { | ||
| if (nextAppState !== 'active' && nextAppState !== 'background') { | ||
| return; | ||
| } | ||
| controllerMessenger.call( | ||
| 'ApplicationStateController:setClientOpen', | ||
| nextAppState === 'active', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Maybe we don't need the conditional up front?
| 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', | |
| ); | |
| }); |
| (newState) => { | ||
| if (newState.isClientOpen) { | ||
| this.startPolling(); | ||
| } else { | ||
| this.stopPolling(); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| startPolling() { | ||
| // Start polling when client opens | ||
| } | ||
|
|
||
| stopPolling() { | ||
| // Stop polling when client closes | ||
| } |
There was a problem hiding this comment.
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:
| (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 | |
| } |
| 'ApplicationStateController:stateChange', | ||
| (newState) => { | ||
| if (newState.isClientOpen) { | ||
| this.connect(); | ||
| } else { | ||
| this.disconnect(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Similar as above — we should use a selector:
| 'ApplicationStateController:stateChange', | |
| (newState) => { | |
| if (newState.isClientOpen) { | |
| this.connect(); | |
| } else { | |
| this.disconnect(); | |
| } | |
| }, | |
| (isClientOpen) => { | |
| if (isClientOpen) { | |
| this.connect(); | |
| } else { | |
| this.disconnect(); | |
| } | |
| }, | |
| applicationStateControllerSelectors.selectIsClientOpen, |


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:ApplicationStateController:setClientStateApplicationStateController:stateChangeand manage themselvesThis 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 asfalse)Messenger API:
ApplicationStateController:setClientState- Called by platform codeApplicationStateController:stateChange- Subscribed to by consumer controllersReferences
Checklist
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 insrc/index.tsappears to reference a non-existentApplicationStateControllerSetClientStateAction(potential build/typing break).Overview
Adds a new package,
@metamask/application-state-controller, providing anApplicationStateControllerthat tracks a non-persistedisClientOpenlifecycle flag and exposes it via messenger (${controllerName}:getState,${controllerName}:setClientOpen) plus the standard${controllerName}:stateChangeevent.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 intoyarn.lock.Written by Cursor Bugbot for commit 06a874b. This will update automatically on new commits. Configure here.