From 9b2b7bc26304d3c96973bc619aa268d91f322b00 Mon Sep 17 00:00:00 2001 From: jiexi Date: Mon, 24 Jun 2024 14:01:20 -0700 Subject: [PATCH] feat: Focus approval request UI when a request is queued (#4456) ## Explanation Currently when a request is queued, it does not cause the existing confirmation window to be focused. This is because queued requests are withheld from the `ApprovalController` which is responsible for showing batched/scrubbable confirmations to the user. Since the `ApprovalController` never actually receives queued requests until they are ready to be shown to the user, it becomes the responsibility of the `QueuedRequestController` to determine when the confirmation window must be focused because a new confirmation has been enqueued. This PR adds a new callback/hook to the `QueuedRequestController`, enabling it to trigger the notification window receiving focus. ## References Fixes: https://github.com/MetaMask/metamask-extension/issues/25397 ## Changelog ### `@metamask/queued-request-controller` - **BREAKING**: `QueuedRequestController` constructor params now requires a `showApprovalRequest` hook that is called when the approval request UI should be opened/focused as the result of a request with confirmation being enqueued. ## 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 highlighted breaking changes using the "BREAKING" category above as appropriate --- .../src/QueuedRequestController.test.ts | 30 ++++++++++ .../src/QueuedRequestController.ts | 57 ++++++++++++------- 2 files changed, 67 insertions(+), 20 deletions(-) diff --git a/packages/queued-request-controller/src/QueuedRequestController.test.ts b/packages/queued-request-controller/src/QueuedRequestController.test.ts index 6b1f517013..aa52b7f7e4 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.test.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.test.ts @@ -28,6 +28,7 @@ describe('QueuedRequestController', () => { messenger: buildQueuedRequestControllerMessenger(), shouldRequestSwitchNetwork: () => false, clearPendingConfirmations: jest.fn(), + showApprovalRequest: jest.fn(), }; const controller = new QueuedRequestController(options); @@ -184,6 +185,32 @@ describe('QueuedRequestController', () => { await secondRequest; }); + it('focuses the existing approval request UI if a request from another origin is being processed', async () => { + const mockShowApprovalRequest = jest.fn(); + const controller = buildQueuedRequestController({ + showApprovalRequest: mockShowApprovalRequest, + }); + // Trigger first request + const firstRequest = controller.enqueueRequest( + { ...buildRequest(), origin: 'https://exampleorigin1.metamask.io' }, + () => new Promise((resolve) => setTimeout(resolve, 10)), + ); + + const secondRequestNext = jest.fn(); + const secondRequest = controller.enqueueRequest( + { ...buildRequest(), origin: 'https://exampleorigin2.metamask.io' }, + secondRequestNext, + ); + + // should focus the existing approval immediately after being queued + expect(mockShowApprovalRequest).toHaveBeenCalledTimes(1); + + await firstRequest; + await secondRequest; + + expect(mockShowApprovalRequest).toHaveBeenCalledTimes(1); + }); + it('drains batch from queue when current batch finishes', async () => { const controller = buildQueuedRequestController(); // Trigger first batch @@ -819,6 +846,7 @@ describe('QueuedRequestController', () => { shouldRequestSwitchNetwork: ({ method }) => method === 'eth_sendTransaction', clearPendingConfirmations: jest.fn(), + showApprovalRequest: jest.fn(), }; const controller = new QueuedRequestController(options); @@ -901,6 +929,7 @@ describe('QueuedRequestController', () => { shouldRequestSwitchNetwork: ({ method }) => method === 'eth_sendTransaction', clearPendingConfirmations: jest.fn(), + showApprovalRequest: jest.fn(), }; const controller = new QueuedRequestController(options); @@ -1037,6 +1066,7 @@ function buildQueuedRequestController( messenger: buildQueuedRequestControllerMessenger(), shouldRequestSwitchNetwork: () => false, clearPendingConfirmations: jest.fn(), + showApprovalRequest: jest.fn(), ...overrideOptions, }; diff --git a/packages/queued-request-controller/src/QueuedRequestController.ts b/packages/queued-request-controller/src/QueuedRequestController.ts index 63358b2004..712caa5b32 100644 --- a/packages/queued-request-controller/src/QueuedRequestController.ts +++ b/packages/queued-request-controller/src/QueuedRequestController.ts @@ -83,6 +83,7 @@ export type QueuedRequestControllerOptions = { request: QueuedRequestMiddlewareJsonRpcRequest, ) => boolean; clearPendingConfirmations: () => void; + showApprovalRequest: () => void; }; /** @@ -149,8 +150,18 @@ export class QueuedRequestController extends BaseController< request: QueuedRequestMiddlewareJsonRpcRequest, ) => boolean; + /** + * This is a function that clears all pending confirmations across + * several controllers that may handle them. + */ #clearPendingConfirmations: () => void; + /** + * This is a function that makes the confirmation notification view + * become visible and focused to the user + */ + #showApprovalRequest: () => void; + /** * Construct a QueuedRequestController. * @@ -158,11 +169,14 @@ export class QueuedRequestController extends BaseController< * @param options.messenger - The restricted controller messenger that facilitates communication with other controllers. * @param options.shouldRequestSwitchNetwork - A function that returns if a request requires the globally selected network to match the dapp selected network. * @param options.clearPendingConfirmations - A function that will clear all the pending confirmations. + * @param options.showApprovalRequest - A function for opening the UI such that + * the existing request can be displayed to the user. */ constructor({ messenger, shouldRequestSwitchNetwork, clearPendingConfirmations, + showApprovalRequest, }: QueuedRequestControllerOptions) { super({ name: controllerName, @@ -175,8 +189,10 @@ export class QueuedRequestController extends BaseController< messenger, state: { queuedRequestCount: 0 }, }); + this.#shouldRequestSwitchNetwork = shouldRequestSwitchNetwork; this.#clearPendingConfirmations = clearPendingConfirmations; + this.#showApprovalRequest = showApprovalRequest; this.#registerMessageHandlers(); } @@ -301,6 +317,25 @@ export class QueuedRequestController extends BaseController< }); } + async #waitForDequeue(origin: string): Promise { + const { promise, reject, resolve } = createDeferredPromise({ + suppressUnhandledRejection: true, + }); + this.#requestQueue.push({ + origin, + processRequest: (error: unknown) => { + if (error) { + reject(error); + } else { + resolve(); + } + }, + }); + this.#updateQueuedRequestCount(); + + return promise; + } + /** * Enqueue a request to be processed in a batch with other requests from the same origin. * @@ -330,26 +365,8 @@ export class QueuedRequestController extends BaseController< this.state.queuedRequestCount > 0 || this.#originOfCurrentBatch !== request.origin ) { - const { - promise: waitForDequeue, - reject, - resolve, - } = createDeferredPromise({ - suppressUnhandledRejection: true, - }); - this.#requestQueue.push({ - origin: request.origin, - processRequest: (error: unknown) => { - if (error) { - reject(error); - } else { - resolve(); - } - }, - }); - this.#updateQueuedRequestCount(); - - await waitForDequeue; + this.#showApprovalRequest(); + await this.#waitForDequeue(request.origin); } else if (this.#shouldRequestSwitchNetwork(request)) { // Process request immediately // Requires switching network now if necessary