Popup ready signal to service worker

[JasonMHasperhoven]

Fixes penumbra-zone/web#285

[grod220]

Can you confirm this solves the problem with #130? On the testnet whale, you can test this by trying to do an EndAll on the auctions page.

Approving, but think this also requires an approval from @turbocrime as they had a vision for how this was going to work from way back.

[grod220]

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.

[turbocrime]

blocking due to issues described by inline comments.

i think part of these issues are due to the lack of documentation for InternalMessage and associated types. it is related to some of the earliest messaging work on the extension, so maybe @grod220 has some input. i will take the opportunity to explain some of the intentions as i understand them in a following comment.

[turbocrime]

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.

[turbocrime]

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.

[turbocrime]

suggestion: revert gitignore diff

[turbocrime]

sorry if this is excessively thorough or verbose. you mentioned a lower
familiarity with typescript, the problems here are decently subtle, and i'm
offboarding so i'm taking this opportunity to deliver as much detail as
possible.

it makes a lot of sense to simply add a new type string to what looks like a
collection of message types, and none of this was documented or explained.

InternalMessage

InternalMessage is used to type specific, scoped document-to-document
messaging within prax. it may be used to describe exactly two message types
involved in a simple request-response dialogue, which can be retrieved by the
utility types InternalRequest and InternalResponse.

InternalMessage's first parameter is type name string, second parameter
request shape, and third parameter response shape. These parameters are
independent and any of these type parameters may be type unions. union
parameters are used to aggregate purpose-specific messages, and are shared by
message handlers and control utilities.

The actual contents are often derived from user input, and importantly, an
InternalMessage handler will encounter messages of unknown shape for unknown
destinations, so the presence of a type field string does not actually validate
the remaining message structure - the request type parameter suggests the shape
that should be validated.

response types are less intensively validated because chrome allows the caller
to await a 'sendMessage' call to receive a specific, call-scoped response. so in
handlers the response type utilities are used to create appropriate responses,
and in message-sending utilities the response types are used to type return
values, but these are not guarded.

Some basic structure is enforced by InternalMessage and the design is intended
to accommodate error transmission, but message-sending utilities are designed to
conceal those details and simply accept bare parameters, return bare values, and
rethrow deserialized errors.

So: A type string is just a label, and doesn't validate type. A handler will
process requests carefully. A caller can accept responses confidently.

message types are grouped by purpose

There are a few sets of messages grouped by purpose, indicated by a type union
of those messages. Generally you can expect that a single handler will process
all request members of the union, from unknown input, disambiguated through that
purpose-specific message type union.

if the messages relate to a control utility, there may be a control utility
typed with param/return types matching the inner request/response contents.

PopupMessage

The popup specifically uses type unions PopupRequest and PopupResponse which
may be optionally narrowed. they are synonymous with InternalRequest and
InternalResponse restricted to and defaulting to the union of PopupMessage.

The main extension popup control utility is simply called popup, located at
src/popup.ts, and it uses these types to describe its valid input, and the
input type corresponds to a return type derived from each request's appropriate
response. Adding members of InternalMessage to the type union PopupMessage
will expand the valid inputs of the popup method input, and instruct
typescript to expect the corresponding return.

PopupType

PopupType enumerates the available types of popup provided by the popup
control utility. at the moment this corresponds nicely with single decision
methods requiring input and output, so it is used both as a type string for
PopupMessage and a parameter for the popup control utility.

for popup, PopupType controls:

• the page route navigated by the launched window
• the request type indexed to type the parameter
• the response type indexed to type the return

for popup message type guards, PopupType controls the contents of the union
that a type guard must discriminate in order to identify a message.

for message handlers, PopupType affects

• what kind of message is considered relevant
• the union which the handler must discriminate after detecting a relevant message
• what kind of input should be applied to the response callback

The Problems

so, adding PopupType.Ready to the popup type enum and adding the type
InternalMessage<PopupType.Ready, null, { popupId: string; }> to the
PopupMessage union means that the popup utility now understands it may
identify a route for 'Ready' to ask the user to take some action regarding
null. the return type does not include null so the user will never close the
window without responding, and result of popup will reliably be a response
object with a popupId.

// extension/src/areuready.ts
import { popup } from './popup';
import { PopupType, Ready } from './message/popup';
const { popupId } = await popup<Ready>({ type: PopupType.Ready, request: null });
console.log('ready response:', popupId.blink());

thankfully, PopupType.Ready does not correspond to any extension route, so the
controller just throws.

additionally, the expanded type union and deletion of discriminating checks in
the type guards

1. allows the popup message handler to accept more inputs, for which no handler code is added
2. claims to the popup message handler that possibly undefined input fields are definitely not undefined, for which no behavior is certain

so actual message entry into the handler is still affected, and the results of
that are uncertain. this is possibly dangerous.

The Suggestions

suggestion: Don't use InternalMessage at all.

You're providing a UUID to the popup via its URI hash. That specific popup can
uniquely identify itself, guaranteed, for this specific purpose, by transmitting
a message containing that uuid alone. No type guard, no object comparison.

simplifying popupReady

const popupReady = (id: string): Promise<void> =>
  new Promise((resolve, reject) => {
    AbortSignal.timeout(POPUP_READY_TIMEOUT).onabort = reject;

    const idListen = (rdy: unknown, _: chrome.runtime.MessageSender, respond: () => void) => {
      if (rdy === id) {
        resolve();
        chrome.runtime.onMessage.removeListener(idListen);
        respond();
      }
    };

    chrome.runtime.onMessage.addListener(idListen);
  });

And this requires no message types changes or handler refactors. In the popup,
you'd have a similar pattern to now.

It seems intuitive to provide the 'input' as the response instead of nothing,
but then we'd have to refactor the existing types handlers etc, so i think this
is better.

alternative suggestion: Use InternalMessage and define the { popupId } payload as the request, not the response.

the request type is intended for parameter input and use in handlers and type
guards, and should exclude null. the response type is intended for use in
response callbacks and returns, and needs no type guards because it's obtained
from typed functions or deserialized from a targeted message.

but you've defined Ready as null-request with typed response, even though it's
being emitted by sendMessage to initiate a request cycle. the direction of the
message - worker to popup, or popup to worker - is not relevant here, but maybe
it was misleading.

flip the definition:

export const Ready = InternalMessage<PopupType.Ready, { popupId: string }, null>;

after that there is much more to this, but now the type is used more
appropriately.

Ready still doesn't make sense as a popup control message, and
there's still the problem with the concept of Ready-type messages as an input
type and affecting the broader popup type logic, but at least it's not null.

todo: throwIfAlreadyOpen

this is pretty critical, i think the explanation above suffices

update throwIfAlreadyOpen to not compare URI segments that are guaranteed to be unique.

todo: use hash router instead of query params

the hash router is already used to path all the popup routes, and we should use
one utility only to manipulate and read parameters.

[JasonMHasperhoven]

Can you confirm this solves the problem with #130? On the testnet whale, you can test this by trying to do an EndAll on the auctions page.

Approving, but think this also requires an approval from @turbocrime as they had a vision for how this was going to work from way back.

Seems to work though it takes time @grod220

[JasonMHasperhoven]

Thank you for the extensive write-up @turbocrime. I understand now that this message should not be of the InternalMessage type. I've refactored it according to your suggestions, please re-review.

[turbocrime]

it looks like throwIfAlreadyOpen is not updated to consider the path containing uuid will always be unique

[turbocrime]

approving, but please correct use or implementation of throwIfAlreadyOpen

[turbocrime]

question:

is there a reason this hook accepts a function parameter?

is there a reason that the parameter uses a named type instead of the signature (isReady?: boolean)?

issue:

this implementation seems like it could send additional messages if the parameters change. they shouldn't, but it might be better to eliminate that.

[turbocrime]

Suggested change

	import { useEffect, useRef } from 'react';
	import { useSearchParams } from 'react-router-dom';
	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] = useSearchParams();
	const popupId = searchParams.get('popupId');
	useEffect(() => {
	if (popupId && (isReady === undefined || isReady) && !sentMessagesRef.current.has(popupId)) {
	sentMessagesRef.current.add(popupId);
	void chrome.runtime.sendMessage(popupId);
	}
	}, [popupId, isReady]);
	};
	import { useEffect, useRef } from 'react';
	import { useSearchParams } from 'react-router-dom';
	// signals that react is ready (mounted) to service worker
	export const usePopupReady = () => {
	const sentReady = useRef<string>();
	const [searchParams] = useSearchParams();
	useEffect(() => {
	const popupId = searchParams.get('popupId');
	if (!sentReady.current && popupId) {
	sentReady.current ??= popupId;
	void chrome.runtime.sendMessage(sentReady.current);
	}
	}, [searchParams]);
	};

[JasonMHasperhoven]

Actually we can remove the function arg, the initial idea (before I placed it into the layout) was that you would be able to control the ready state from a parent component, but this is no longer needed and overcomplicates things.

[grod220]

A subtle, but noticeable difference is that the extension popup content loads faster (at least for me). Nice job!