feat: add new ConnectivityController
#7623
Conversation
cryptodev-2s
force-pushed
the
feature/add-connectivity-controller
branch
from
b14162d to
daa5333
Compare
cryptodev-2s
force-pushed
the
feature/add-connectivity-controller
branch
from
daa5333 to
5e51275
Compare
|
@metamaskbot publish-preview |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| draftState.connectivityStatus = status; | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Controller doesn't clean up connectivity service subscription on destroy
Medium Severity
The ConnectivityController subscribes to connectivityService.onConnectivityChange() in the constructor but neither stores a reference to the service nor overrides destroy() to clean up. The BaseController.destroy() documentation states subclasses "should be extended...to clean up any additional connections or events." Other controllers like TokenListController and CurrencyRateController follow this pattern. Without cleanup, if the controller is destroyed while the service continues running, the service retains a reference to the callback (which captures this), potentially causing memory leaks and unintended state updates on a destroyed controller.
There was a problem hiding this comment.
That's true. We haven't been consistent about implementing destroy. But maybe we should do this?
If the ConnectivityService used the messenger then we could call unsubscribe. Otherwise, we would need ConnectivityService to implement EventEmitter so that it could call removeEventListener in its destroy.
Maybe ConnectivityService should use the messenger? Curious to know your thoughts.
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| export type ConnectivityControllerState = { | ||
| /** | ||
| * The current device connectivity status. | ||
| * Named with 'connectivity' prefix to avoid conflicts when state is flattened in Redux. |
| * | ||
| * Platform implementations: | ||
| * - Mobile: Use `NetInfoConnectivityService` with `@react-native-community/netinfo` | ||
| * - Extension (same context): Use `BrowserConnectivityService` | ||
| * - Extension (cross-context): Use `PassiveConnectivityService` and call | ||
| * `setStatus()` from the UI context |
There was a problem hiding this comment.
Do we need to include this here? It seems that we would need to update this documentation if the implementation in the client changes.
| * | |
| * Platform implementations: | |
| * - Mobile: Use `NetInfoConnectivityService` with `@react-native-community/netinfo` | |
| * - Extension (same context): Use `BrowserConnectivityService` | |
| * - Extension (cross-context): Use `PassiveConnectivityService` and call | |
| * `setStatus()` from the UI context |
| * **Platform implementations:** | ||
| * | ||
| * - **Mobile:** Inject `NetInfoConnectivityService` using `@react-native-community/netinfo` | ||
| * - **Extension:** Inject `PassiveConnectivityService` in the background. | ||
| * Status is updated via the `setDeviceConnectivityStatus` API, which is called from: | ||
| * - MV3: Offscreen document (where browser events work reliably) | ||
| * - MV2: Background page (where browser events work directly) |
There was a problem hiding this comment.
Similar to above — do we need to include this in the documentation?
| * **Platform implementations:** | |
| * | |
| * - **Mobile:** Inject `NetInfoConnectivityService` using `@react-native-community/netinfo` | |
| * - **Extension:** Inject `PassiveConnectivityService` in the background. | |
| * Status is updated via the `setDeviceConnectivityStatus` API, which is called from: | |
| * - MV3: Offscreen document (where browser events work reliably) | |
| * - MV2: Background page (where browser events work directly) |
| draftState.connectivityStatus = status; | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
That's true. We haven't been consistent about implementing destroy. But maybe we should do this?
If the ConnectivityService used the messenger then we could call unsubscribe. Otherwise, we would need ConnectivityService to implement EventEmitter so that it could call removeEventListener in its destroy.
Maybe ConnectivityService should use the messenger? Curious to know your thoughts.
| * Connectivity status constants. | ||
| * Used to represent whether the device has internet connectivity. | ||
| */ | ||
| export const ConnectivityStatus = { |
There was a problem hiding this comment.
What do you think about calling this CONNECTIVITY_STATUSES? We don't have formal guidelines on these "quasi-enums", but I recently found this guide and have found it to be useful.
Explanation
This PR introduces a new
@metamask/connectivity-controllerpackage that provides a centralized way to track and manage device internet connectivity status across MetaMask clients (extension and mobile).What is the current state of things and why does it need to change?
What is the solution your changes offer and how does it work?
The
ConnectivityControlleris a platform-agnostic controller that:onlineoroffline) in its stateConnectivityServiceimplementations are injected at construction timestateChangeevents when connectivity status changes, allowing other controllers and UI components to react accordinglyThe controller is designed to work with different service implementations:
NetInfoConnectivityServiceusing@react-native-community/netinfoBrowserConnectivityServiceusing browser APIsAre there any changes whose purpose might not be obvious to those unfamiliar with the domain?
connectivityStatusfield in state is prefixed with "connectivity" to avoid conflicts when state is flattened in Reduxpersist: falsesince connectivity status is ephemeral and should not be restored from storageReferences https://consensyssoftware.atlassian.net/browse/WPC-210
Checklist
Note
New package:
@metamask/connectivity-controllerConnectivityController(extendsBaseController) to store device connectivity status inconnectivityStatususing an injectedConnectivityService; subscribes toonConnectivityChangeand emitsConnectivityController:stateChangeConnectivityStatus,ConnectivityService, controller messenger/actions/events) viasrc/index.ts; defaults toonlineand state is not persistedsrc/ConnectivityController.test.ts; package setup includespackage.json, Jest/TypeDoc configs,LICENSE,README, andCHANGELOGMonorepo integrations
README.md(package list and dependency graph),.github/CODEOWNERS,teams.json,tsconfig.json,tsconfig.build.json, andyarn.lockto register the new packageWritten by Cursor Bugbot for commit 9e00b40. This will update automatically on new commits. Configure here.