Skip to content

refactor: remove KeyringController:getState dep, init() and destroy() #7762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mikesposito merged 2 commits into from
Jan 29, 2026
Merged

refactor: remove KeyringController:getState dep, init() and destroy() #7762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f1c26e
refactor: remove `KeyringController:getState` dep, `init()` and `dest…
mikesposito Jan 28, 2026
8e526e9
remove unused imports
mikesposito Jan 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
278 changes: 30 additions & 248 deletions packages/config-registry-controller/src/ConfigRegistryController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import type { MockAnyNamespace } from '@metamask/messenger';
import type {
MockAnyNamespace,
MessengerActions,
MessengerEvents,
} from '@metamask/messenger';
import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger';
import { useFakeTimers } from 'sinon';

Expand All @@ -19,20 +23,11 @@ import { advanceTime } from '../../../tests/helpers';

const namespace = 'ConfigRegistryController' as const;

type RootMessenger = Messenger<
MockAnyNamespace,
| { type: 'RemoteFeatureFlagController:getState'; handler: () => unknown }
| {
type: 'KeyringController:getState';
handler: () => { isUnlocked: boolean };
}
| {
type: 'ConfigRegistryApiService:fetchConfig';
handler: (options?: { etag?: string }) => Promise<FetchConfigResult>;
},
| { type: 'KeyringController:unlock'; payload: [] }
| { type: 'KeyringController:lock'; payload: [] }
>;
type AllActions = MessengerActions<ConfigRegistryMessenger>;

type AllEvents = MessengerEvents<ConfigRegistryMessenger>;

type RootMessenger = Messenger<MockAnyNamespace, AllActions, AllEvents>;

/**
* Constructs a messenger for ConfigRegistryController.
Expand All @@ -43,42 +38,28 @@ function getConfigRegistryControllerMessenger(): {
messenger: ConfigRegistryMessenger;
rootMessenger: RootMessenger;
} {
const rootMessenger = new Messenger<
MockAnyNamespace,
| { type: 'RemoteFeatureFlagController:getState'; handler: () => unknown }
| {
type: 'KeyringController:getState';
handler: () => { isUnlocked: boolean };
}
| {
type: 'ConfigRegistryApiService:fetchConfig';
handler: (options?: { etag?: string }) => Promise<FetchConfigResult>;
},
| { type: 'KeyringController:unlock'; payload: [] }
| { type: 'KeyringController:lock'; payload: [] }
>({
const rootMessenger: RootMessenger = new Messenger({
namespace: MOCK_ANY_NAMESPACE,
});

const configRegistryControllerMessenger = new Messenger<
typeof namespace,
never,
| { type: 'KeyringController:unlock'; payload: [] }
| { type: 'KeyringController:lock'; payload: [] },
typeof rootMessenger
>({
namespace,
parent: rootMessenger,
}) as ConfigRegistryMessenger;
const configRegistryControllerMessenger: ConfigRegistryMessenger =
new Messenger<
typeof namespace,
AllActions,
AllEvents,
typeof rootMessenger
>({
namespace,
parent: rootMessenger,
});

rootMessenger.delegate({
messenger: configRegistryControllerMessenger,
actions: [
'RemoteFeatureFlagController:getState',
'KeyringController:getState',
'ConfigRegistryApiService:fetchConfig',
] as never[],
events: ['KeyringController:unlock', 'KeyringController:lock'] as never[],
],
events: ['KeyringController:unlock', 'KeyringController:lock'],
});

return { messenger: configRegistryControllerMessenger, rootMessenger };
Expand Down Expand Up @@ -168,7 +149,6 @@ type WithControllerCallback<ReturnValue> = (args: {
messenger: ConfigRegistryMessenger;
mockApiServiceHandler: jest.Mock;
mockRemoteFeatureFlagGetState: jest.Mock;
mockKeyringControllerGetState: jest.Mock;
}) => Promise<ReturnValue> | ReturnValue;

type WithControllerOptions = {
Expand Down Expand Up @@ -204,23 +184,11 @@ async function withController<ReturnValue>(
mockRemoteFeatureFlagGetState,
);

const mockKeyringControllerGetState = jest.fn().mockReturnValue({
isUnlocked: false,
});

rootMessenger.registerActionHandler(
'KeyringController:getState',
mockKeyringControllerGetState,
);

const controller = new ConfigRegistryController({
messenger,
...options,
});

// Initialize the controller after creation
controller.init();

try {
return await testFunction({
controller,
Expand All @@ -229,11 +197,9 @@ async function withController<ReturnValue>(
messenger,
mockApiServiceHandler,
mockRemoteFeatureFlagGetState,
mockKeyringControllerGetState,
});
} finally {
controller.stopPolling();
controller.destroy();
clock.restore();
mockApiServiceHandler.mockReset();
}
Expand Down Expand Up @@ -522,10 +488,6 @@ describe('ConfigRegistryController', () => {
'RemoteFeatureFlagController:getState',
mockRemoteFeatureFlagGetState,
);
testRootMessenger.registerActionHandler(
Copy link

@cursor cursor bot Jan 28, 2026

Choose a reason for hiding this comment

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

Delegating action without registered handler in test

Low Severity

The PR removed the registerActionHandler call for KeyringController:getState but left 'KeyringController:getState' in the delegate actions array at line 495. This creates inconsistent test code where an action is delegated without a corresponding handler being registered. While this won't cause test failures (since the controller no longer calls this action), it's dead code that was likely an oversight during the refactor.

Fix in Cursor Fix in Web

Copy link
Member Author

@mikesposito mikesposito Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed these additional things to be removed in tests, but I also wanted to refactor the way some tests use the testRootMessenger, so I left them for another PR where I will propose a more broad refactor of root messenger usage in tests.

'KeyringController:getState',
jest.fn().mockReturnValue({ isUnlocked: false }),
);
testRootMessenger.delegate({
messenger: testMessenger,
actions: [
Expand Down Expand Up @@ -2296,103 +2258,6 @@ describe('ConfigRegistryController', () => {
});

describe('KeyringController event listeners', () => {
it('starts polling when KeyringController is already unlocked on initialization', async () => {
await withController(async ({ clock }) => {
const mockKeyringControllerGetState = jest.fn().mockReturnValue({
isUnlocked: true,
});

const testRootMessenger = new Messenger<
MockAnyNamespace,
| {
type: 'RemoteFeatureFlagController:getState';
handler: () => unknown;
}
| {
type: 'KeyringController:getState';
handler: () => { isUnlocked: boolean };
}
| {
type: 'ConfigRegistryApiService:fetchConfig';
handler: (options?: {
etag?: string;
}) => Promise<FetchConfigResult>;
},
| { type: 'KeyringController:unlock'; payload: [] }
| { type: 'KeyringController:lock'; payload: [] }
>({
namespace: MOCK_ANY_NAMESPACE,
});

const testMessenger = new Messenger<
typeof namespace,
never,
| { type: 'KeyringController:unlock'; payload: [] }
| { type: 'KeyringController:lock'; payload: [] },
typeof testRootMessenger
>({
namespace,
parent: testRootMessenger,
}) as ConfigRegistryMessenger;

testRootMessenger.registerActionHandler(
'ConfigRegistryApiService:fetchConfig',
jest.fn(buildMockApiServiceHandler()),
);
testRootMessenger.registerActionHandler(
'RemoteFeatureFlagController:getState',
jest.fn().mockReturnValue({
remoteFeatureFlags: {
configRegistryApiEnabled: true,
},
cacheTimestamp: Date.now(),
}),
);
testRootMessenger.registerActionHandler(
'KeyringController:getState',
mockKeyringControllerGetState,
);
testRootMessenger.delegate({
messenger: testMessenger,
actions: [
'RemoteFeatureFlagController:getState',
'KeyringController:getState',
'ConfigRegistryApiService:fetchConfig',
] as never[],
events: [
'KeyringController:unlock',
'KeyringController:lock',
] as never[],
});

const controller = new ConfigRegistryController({
messenger: testMessenger,
});

// Initialize the controller after creation
controller.init();

const executePollSpy = jest.spyOn(controller, '_executePoll');

await advanceTime({ clock, duration: 100 });

expect(mockKeyringControllerGetState).toHaveBeenCalled();
expect(executePollSpy).toHaveBeenCalledTimes(1);

controller.stopPolling();
});
});

it('handles KeyringController:getState error gracefully when KeyringController is unavailable', async () => {
await withController(({ controller, mockKeyringControllerGetState }) => {
mockKeyringControllerGetState.mockImplementation(() => {
throw new Error('KeyringController not available');
});

expect(controller.state.lastFetched).toBeNull();
});
});

it('starts polling when KeyringController:unlock event is published', async () => {
await withController(async ({ controller, clock, rootMessenger }) => {
const executePollSpy = jest.spyOn(controller, '_executePoll');
Expand All @@ -2412,27 +2277,16 @@ describe('ConfigRegistryController', () => {
});

it('stops polling when KeyringController:lock event is published', async () => {
await withController(
async ({
controller,
clock,
rootMessenger,
mockKeyringControllerGetState,
}) => {
mockKeyringControllerGetState.mockReturnValue({
isUnlocked: true,
});

const stopPollingSpy = jest.spyOn(controller, 'stopPolling');
await withController(async ({ controller, clock, rootMessenger }) => {
const stopPollingSpy = jest.spyOn(controller, 'stopPolling');

await advanceTime({ clock, duration: 0 });
expect(stopPollingSpy).not.toHaveBeenCalled();
await advanceTime({ clock, duration: 0 });
expect(stopPollingSpy).not.toHaveBeenCalled();

rootMessenger.publish('KeyringController:lock');
rootMessenger.publish('KeyringController:lock');

expect(stopPollingSpy).toHaveBeenCalled();
},
);
expect(stopPollingSpy).toHaveBeenCalled();
});
});

it('calls startPolling with default parameter when called without arguments', async () => {
Expand Down Expand Up @@ -2638,76 +2492,4 @@ describe('ConfigRegistryController', () => {
});
});
});

describe('destroy', () => {
it('cleans up event listeners and action handlers', async () => {
await withController(({ controller, messenger }) => {
const unsubscribeSpy = jest.spyOn(messenger, 'unsubscribe');
const unregisterActionHandlerSpy = jest.spyOn(
messenger,
'unregisterActionHandler',
);
const stopPollingSpy = jest.spyOn(controller, 'stopPolling');

controller.destroy();

expect(stopPollingSpy).toHaveBeenCalled();
expect(unsubscribeSpy).toHaveBeenCalledWith(
'KeyringController:unlock',
expect.any(Function),
);
expect(unsubscribeSpy).toHaveBeenCalledWith(
'KeyringController:lock',
expect.any(Function),
);
expect(unregisterActionHandlerSpy).toHaveBeenCalledWith(
'ConfigRegistryController:startPolling',
);
expect(unregisterActionHandlerSpy).toHaveBeenCalledWith(
'ConfigRegistryController:stopPolling',
);
});
});

it('handles unsubscribe errors gracefully', async () => {
await withController(({ controller, messenger }) => {
jest.spyOn(messenger, 'unsubscribe').mockImplementation(() => {
throw new Error('Handler not subscribed');
});

// Should not throw even if unsubscribe fails
expect(() => controller.destroy()).not.toThrow();
});
});

it('clears pending timeout when destroying', async () => {
const pollingInterval = 10000;
const recentTimestamp = Date.now() - 2000;
await withController(
{
options: {
pollingInterval,
state: {
lastFetched: recentTimestamp,
},
},
},
async ({ controller, clock }) => {
const executePollSpy = jest.spyOn(controller, '_executePoll');
controller.startPolling(null);

// Verify timeout was set
await advanceTime({ clock, duration: 0 });
expect(executePollSpy).not.toHaveBeenCalled();

// Destroy should clear the timeout
controller.destroy();

// Advance time past when the timeout would have fired
await advanceTime({ clock, duration: pollingInterval });
expect(executePollSpy).not.toHaveBeenCalled();
},
);
});
});
});
Loading
Loading