Don't resolve plugin prompt promise until modal has closed, to avoid a race condition

[develohpanda]

The background

All modals in this codebase are callback based. That is, to process the result of a modal, you need to send an onComplete or onCancel callback into the modal.show() method.

The plugin API which exposes the prompt modal (context.app.prompt) is not callback based, it is promise based. That is, if the prompt was cancelled, context.app.prompt should reject with an error, and if the prompt was submitted, context.app.prompt should resolve with the prompt value.

The callback-based prompt is converted to a promise-based helper in packages/insomnia-app/app/plugins/context/app.tsx.

The alert modal "attempts" to make itself be both callback-based and promise-based, which is additional confusion, so this (and modals in general) do need a cleanup which is not done in this PR.

The change

The onComplete callback for a prompt was previously synchronous, however an update in #3474 introduced asynchronous handlers, adding a loading state and not closing the modal until the onComplete handler was finished.

The race condition

This change introduced a race condition with the plugin API implementation translation. On clicking submit and triggering the onComplete callback, the plugin API (which resolves the wrapped promise - see previous implementation) would resolve the promise and tell a plugin that "the prompt is complete", while at the same time the application would be in the process of closing the modal. If the plugin immediately opened another prompt (while the application was still trying to close the first, because of a race condition), the second prompt would close.

The fix

We already know that the modal APIs need to be improved, and ideally an attempt made to remove the difference in API (callback-based vs promise-based) to avoid mistakes in translation such as this one. This PR only makes a minimal fix for context.app.prompt. One approach to make this plugin stable would be to decouple the PromptModal used within the app from the plugin API and have a minimal PromptModal specifically for plugins with a restricted implementation matching the API available the plugin developers.

In this PR, when translating from the callback-based prompt modal to a promise-based helper for plugins, we only resolve the promise if:

1. the user pressed submit
2. after the modal has hidden

The demo

This demo uses the example plugin in #3959 and also https://github.com/Vyoam/insomnia-plugin-postman-export, both of which were impacted by this race condition.

Closes #3959

[gatzjames]

Nicely done and very well explained 🙏

[dimitropoulos]

why not just not define it (to match the type)? when we have https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#exact-optional-property-types on we'll need to come back and fix this.

[develohpanda]

Because it shouldn't be initialized to anything; we should just leave it off the initial state here, but since the other handlers initialize as undefined I followed the same pattern here. I vote, in this instance, on coming back to fix it once that's enabled, because this isn't the only instance that will need fixing in this file

[dimitropoulos]

I'm guessing you probably agree, actually, but I'm worried about how confusing it is that calling an onComplete callback doesn't actually... well... haha... complete. The promise doesn't actually resolve until the onHide function is called, and that's only called from within the a custom callback on a custom component on a custom wrapper...

        showModal(WrapperModal, {
          title,
          body: <HtmlElementWrapper el={body} onUnmount={options.onHide} />,
          tall: options.tall,
          skinny: options.skinny,
          wide: options.wide,
        });

I totally understand how in this moment it can make sense to do it this way, but I don't think this is an acceptable long-term solution because it adds even more complexity and cognitive-overhead to something that's already quite complex (hence, us introducing the original bug in the first place).

[develohpanda]

I think the snippet above is from something else, but fundamentally I understand and agree with your observation 🙌 All of this arises from the fact that we have single instance modals shared across all calls; we fix that, we fix many of these issues.

[dimitropoulos]

Overall, I want to see this fixed in production as soon as we can, so I don't think we should blockade this solution for now, but I definitely want to (if we merge this approach) see us fix the problem of global modals soon, which will (in itself) completely solve this issue. I can give examples you want of codebases that accomplish non-global modals.

[dimitropoulos]

This needs to be fixed immediately. The fix here works. Therefore I approve.

However, I think the fact that we're having a patch release due to a gotcha in the overall architecture of modals and then we had to fix it by introducing what I consider to be a new gotcha (that onComplete only defers the actual completion logic to onHide) should cause us to consider improving how modals work in the app. A necessary gotcha - I agree - given the current state of things. But we can change the state of things and incrementally improve. I think we should do so soon.