fix: improve race condition connectivity handling #2342
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
stipsan
commented
Dec 21, 2024
| @@ -75,8 +75,8 @@ export type Node<S extends Message, R extends Message> = { | |||
| handler: (event: U['data']) => U['response'], | |||
| ) => () => void | |||
| onStatus: ( | |||
| handler: (status: Omit<Status, 'disconnected'>) => void, | |||
| filter?: Omit<Status, 'disconnected'>, | |||
| handler: (status: Exclude<Status, 'disconnected'>) => void, | |||
There was a problem hiding this comment.
Omit on string types doesn't work, with Exclude I managed to find a few places that still expected onStatus to emit a disconnected event 🙌
Comment on lines
+540
to
+542
| if (cachedStatus) { | ||
| handler(cachedStatus) | ||
| } |
There was a problem hiding this comment.
This logic ensures that although useComlink itself now uses onStatus = connected to determine when to forward the comlink to <Overlays />, other hooks are still able to subscribe to onStatus and reach eventual consistency even though the connected event has happened before they subscribed.
| @@ -517,6 +517,8 @@ export default function PresentationTool(props: { | |||
| <Flex direction="column" flex={1} height="fill" ref={setBoundaryElement}> | |||
| <BoundaryElementProvider element={boundaryElement}> | |||
| <Preview | |||
| // Make sure the iframe is unmounted if the targetOrigin has changed | |||
There was a problem hiding this comment.
This is more of a "protecting against future bugs" kind of thing
Closed
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follows up on #2308, turns out the root cause is a race condition that happens on fast machines and optimal race conditions. Where
<VisualEditing />winds up sendingawait comlink.fetchbefore<PresentationTool />has had time to mount relevantPostMessagecomponents, and run theiruseEffect's that sets upcomlink.on()handlers on the other side.See the https://github.com/sanity-io/visual-editing/compare/debug branch for examples, in the test studio: https://visual-editing-studio-git-debug.sanity.dev/page-builder-demo/presentation/dndTestPage/60a2a9dd-7514-4b3c-8a2a-af06fb4cc31e?preview=/dnd
The short term fix is to delay a bit when the comlink is set in
<VisualEditing />hoping its setup by then. The real long term fix is to refactor<PresentationTool />itself to use a state machine to handle comlink, and thus have all the handlers ready right away as soon as the iframe is loaded.cc @rdunk so you see this eventually, after 🎄❤️