-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Don't resolve plugin prompt promise until modal has closed, to avoid a race condition #3970
Don't resolve plugin prompt promise until modal has closed, to avoid a race condition #3970
Conversation
33c1a26
to
68965cf
Compare
68965cf
to
5120233
Compare
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.
Nicely done and very well explained 🙏
@@ -69,6 +71,7 @@ class PromptModal extends PureComponent<{}, State> { | |||
onComplete: undefined, | |||
onCancel: undefined, | |||
onDeleteHint: undefined, | |||
onHide: undefined, |
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.
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.
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.
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
onComplete(value: string) { | ||
resolve(value); | ||
shouldResolve = true; | ||
resolveWith = value; | ||
}, |
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.
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).
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.
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.
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.
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.
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.
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.
32ffd07
to
7e4fdb8
Compare
…a race condition (#3970) Co-authored-by: James Gatz <[email protected]>
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
oronCancel
callback into themodal.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 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 theonComplete
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
In this PR, when translating from the callback-based prompt modal to a promise-based helper for plugins, we only resolve the promise if:
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