Skip to content

Commit

Permalink
feat: Focus approval request UI when a request is queued (#4456)
Browse files Browse the repository at this point in the history
## 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: MetaMask/metamask-extension#25397

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@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
  • Loading branch information
jiexi authored Jun 24, 2024
1 parent 1dcf200 commit 9b2b7bc
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe('QueuedRequestController', () => {
messenger: buildQueuedRequestControllerMessenger(),
shouldRequestSwitchNetwork: () => false,
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
};

const controller = new QueuedRequestController(options);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -819,6 +846,7 @@ describe('QueuedRequestController', () => {
shouldRequestSwitchNetwork: ({ method }) =>
method === 'eth_sendTransaction',
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
};

const controller = new QueuedRequestController(options);
Expand Down Expand Up @@ -901,6 +929,7 @@ describe('QueuedRequestController', () => {
shouldRequestSwitchNetwork: ({ method }) =>
method === 'eth_sendTransaction',
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
};

const controller = new QueuedRequestController(options);
Expand Down Expand Up @@ -1037,6 +1066,7 @@ function buildQueuedRequestController(
messenger: buildQueuedRequestControllerMessenger(),
shouldRequestSwitchNetwork: () => false,
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
...overrideOptions,
};

Expand Down
57 changes: 37 additions & 20 deletions packages/queued-request-controller/src/QueuedRequestController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export type QueuedRequestControllerOptions = {
request: QueuedRequestMiddlewareJsonRpcRequest,
) => boolean;
clearPendingConfirmations: () => void;
showApprovalRequest: () => void;
};

/**
Expand Down Expand Up @@ -149,20 +150,33 @@ 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.
*
* @param options - Controller options.
* @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,
Expand All @@ -175,8 +189,10 @@ export class QueuedRequestController extends BaseController<
messenger,
state: { queuedRequestCount: 0 },
});

this.#shouldRequestSwitchNetwork = shouldRequestSwitchNetwork;
this.#clearPendingConfirmations = clearPendingConfirmations;
this.#showApprovalRequest = showApprovalRequest;
this.#registerMessageHandlers();
}

Expand Down Expand Up @@ -301,6 +317,25 @@ export class QueuedRequestController extends BaseController<
});
}

async #waitForDequeue(origin: string): Promise<void> {
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.
*
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 9b2b7bc

Please sign in to comment.