Skip to content
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

Popup ready signal to service worker #174

Merged
merged 12 commits into from
Aug 28, 2024
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,4 @@ packages/*/package

apps/extension/chromium-profile

scripts/private
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: revert gitignore diff

24 changes: 24 additions & 0 deletions apps/extension/src/hooks/popup-ready.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { useEffect, useRef } from 'react';
import { PopupResponse, PopupType, Ready } from '../message/popup';

type IsReady = boolean | undefined;

// signals that react is ready (mounted) to service worker
export const usePopupReady = (isReady: IsReady = undefined) => {
const sentMessagesRef = useRef(new Set());
const searchParams = new URLSearchParams(window.location.search);
const popupId = searchParams.get('popupId');
JasonMHasperhoven marked this conversation as resolved.
Show resolved Hide resolved

useEffect(() => {
if (popupId && (isReady === undefined || isReady) && !sentMessagesRef.current.has(popupId)) {
sentMessagesRef.current.add(popupId);

void chrome.runtime.sendMessage({
type: PopupType.Ready,
data: {
popupId,
},
} as PopupResponse<Ready>);
}
}, [popupId, isReady]);
};
26 changes: 23 additions & 3 deletions apps/extension/src/message/popup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import { OriginRecord } from '../storage/types';
export enum PopupType {
TxApproval = 'TxApproval',
OriginApproval = 'OriginApproval',
Ready = 'PopupReady',
JasonMHasperhoven marked this conversation as resolved.
Show resolved Hide resolved
}

export type PopupMessage = TxApproval | OriginApproval;
export type PopupMessage = TxApproval | OriginApproval | Ready;
export type PopupRequest<T extends PopupMessage = PopupMessage> = InternalRequest<T>;
export type PopupResponse<T extends PopupMessage = PopupMessage> = InternalResponse<T>;

Expand All @@ -34,6 +35,14 @@ export type TxApproval = InternalMessage<
}
>;

export type Ready = InternalMessage<
PopupType.Ready,
null,
{
popupId: string;
}
>;
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (blocking)

defining an InternalMessage with a null second parameter and including that in the PopupMessage union means that PopupRequest now includes a union member shaped { type: PopupType.Ready, request: null }, which affects methods that rely on the PopupRequest type to constrain input.

todo:

Don't use InternalMessage with a null request parameter, and possibly don't use InternalMessage at all.

Copy link
Contributor

@turbocrime turbocrime Aug 24, 2024

Choose a reason for hiding this comment

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

for example, this PR presently assures the compiler this call is ok, even though it's not an intended use of this message type:

const readyResponse: { popupId: string }  = await popup<Ready>({ type: PopupType.Ready, request: null });

this verifies the user is logged in, and then thankfully throws before opening a popup when the popup method can't identify a route for PopupType.Ready.


export const isPopupRequest = (req: unknown): req is PopupRequest =>
req != null &&
typeof req === 'object' &&
Expand All @@ -42,8 +51,19 @@ export const isPopupRequest = (req: unknown): req is PopupRequest =>
typeof req.type === 'string' &&
req.type in PopupType;

export const isPopupResponse = (res: unknown): res is PopupResponse =>
res != null &&
typeof res === 'object' &&
('data' in res || 'error' in res) &&
'type' in res &&
typeof res.type === 'string' &&
res.type in PopupType;

export const isOriginApprovalRequest = (req: unknown): req is InternalRequest<OriginApproval> =>
isPopupRequest(req) && req.type === PopupType.OriginApproval && 'origin' in req.request;
isPopupRequest(req) && req.type === PopupType.OriginApproval;

export const isTxApprovalRequest = (req: unknown): req is InternalRequest<TxApproval> =>
isPopupRequest(req) && req.type === PopupType.TxApproval && 'authorizeRequest' in req.request;
isPopupRequest(req) && req.type === PopupType.TxApproval;
JasonMHasperhoven marked this conversation as resolved.
Show resolved Hide resolved

export const isPopupReadyResponse = (res: unknown): res is InternalResponse<Ready> =>
isPopupResponse(res) && res.type === PopupType.Ready;
43 changes: 37 additions & 6 deletions apps/extension/src/popup.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import { sessionExtStorage } from './storage/session';
import { PopupMessage, PopupRequest, PopupType } from './message/popup';
import {
isPopupReadyResponse,
PopupMessage,
PopupRequest,
PopupResponse,
PopupType,
} from './message/popup';
import { PopupPath } from './routes/popup/paths';
import type { InternalRequest, InternalResponse } from '@penumbra-zone/types/internal-msg/shared';
import { Code, ConnectError } from '@connectrpc/connect';
Expand All @@ -18,9 +24,10 @@ const isChromeResponderDroppedError = (
export const popup = async <M extends PopupMessage>(
req: PopupRequest<M>,
): Promise<M['response']> => {
await spawnPopup(req.type);
// We have to wait for React to bootup, navigate to the page, and render the components
await new Promise(resolve => setTimeout(resolve, 800));
const popupId = crypto.randomUUID();
await spawnPopup(req.type, popupId);
await popupReady(popupId);
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: we should add a code comment here that informs devs that this is necessary given it takes a bit of time for the popup to be ready to accept messages.


const response = await chrome.runtime
.sendMessage<InternalRequest<M>, InternalResponse<M>>(req)
.catch((e: unknown) => {
Expand All @@ -30,6 +37,7 @@ export const popup = async <M extends PopupMessage>(
throw e;
}
});

if (response && 'error' in response) {
throw errorFromJson(response.error, undefined, ConnectError.from(response));
} else {
Expand Down Expand Up @@ -73,8 +81,8 @@ const throwIfNeedsLogin = async () => {
}
};

const spawnPopup = async (pop: PopupType) => {
const popUrl = new URL(chrome.runtime.getURL('popup.html'));
const spawnPopup = async (pop: PopupType, popupId: string) => {
const popUrl = new URL(chrome.runtime.getURL(`popup.html?popupId=${popupId}`));
JasonMHasperhoven marked this conversation as resolved.
Show resolved Hide resolved

await throwIfNeedsLogin();

Expand All @@ -89,3 +97,26 @@ const spawnPopup = async (pop: PopupType) => {
throw Error('Unknown popup type');
}
};

const POPUP_READY_TIMEOUT = 60 * 1000;

const popupReady = async (popupId: string): Promise<void> => {
return new Promise((resolve, reject): void => {
setTimeout(() => {
reject(new Error('Popup ready timed out'));
}, POPUP_READY_TIMEOUT);
JasonMHasperhoven marked this conversation as resolved.
Show resolved Hide resolved

const handlePopupReady = (res: PopupResponse): void => {
if (!isPopupReadyResponse(res)) {
return;
}

if ('data' in res && res.data.popupId === popupId) {
chrome.runtime.onMessage.removeListener(handlePopupReady);
resolve();
}
};

chrome.runtime.onMessage.addListener(handlePopupReady);
});
};
15 changes: 10 additions & 5 deletions apps/extension/src/routes/popup/popup-layout.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Outlet } from 'react-router-dom';
import { usePopupReady } from '../../hooks/popup-ready';

/**
* @todo: Fix the issue where the detached popup isn't sized correctly. This
Expand All @@ -11,8 +12,12 @@ import { Outlet } from 'react-router-dom';
* routes here in `PopupLayout`, and using a different root class name for each,
* then removing the hard-coded width from `globals.css`.
*/
export const PopupLayout = () => (
<div className='flex grow flex-col bg-card-radial'>
<Outlet />
</div>
);
export const PopupLayout = () => {
usePopupReady();
turbocrime marked this conversation as resolved.
Show resolved Hide resolved

return (
<div className='flex grow flex-col bg-card-radial'>
<Outlet />
</div>
);
};