-
-
Notifications
You must be signed in to change notification settings - Fork 188
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.
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
feat: Focus approval request UI when a request is queued #4456
feat: Focus approval request UI when a request is queued #4456
Conversation
@@ -83,6 +83,7 @@ export type QueuedRequestControllerOptions = { | |||
request: QueuedRequestMiddlewareJsonRpcRequest, | |||
) => boolean; | |||
clearPendingConfirmations: () => void; | |||
showApprovalRequest: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be optional?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not? this controller will only be used on the extension AFAIK and I believe we will always want this hook passed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's optional in the sense that this isn't required for the QueuedRequestController to perform it's core function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thats definitely true. I guess I think of the type as safeguard for accidental removal... though maybe that's not really true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Releases `queued-request-controller` #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 theApprovalController
never actually receives queued requests until they are ready to be shown to the user, it becomes the responsibility of theQueuedRequestController
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
@metamask/queued-request-controller
QueuedRequestController
constructor params now requires ashowApprovalRequest
hook that is called when the approval request UI should be opened/focused as the result of a request with confirmation being enqueued.Checklist