Sketch Lab: prevent possible race condition on image upload #70706
Conversation
| updateSources({ | ||
| updateSources(prevSources => ({ | ||
| source: { | ||
| ...serializedData, |
There was a problem hiding this comment.
There's a shift here so that the first updateSources is about serializing the core canvas updates (ie, serializedData), and the one below is about just updating the upload status of new files (ie, newFilesWithUploadStatus).
| } | ||
|
|
||
| // Save if needed | ||
| if (!readonlyWorkspaceRef.current) { |
There was a problem hiding this comment.
This is slightly unholy (I think these updater functions are supposed to be "pure", no side effects), but seems not the worst to me.
There was a problem hiding this comment.
Is there a way to make this work with a useEffect? Might be tricky with needing to track forceSave 🤔 I'm not sure how risky putting this side effect inside the updater, but I worry about confusing bugs in the future.
There was a problem hiding this comment.
Yeah I was thinking about useEffect, and yes was discouraged by the forceSave bit. Maybe? The other thing about moving to useEffect is that it would get called any time sources change (reinitialization, etc) vs. our current implementation that is more targeted (ie, when updateSources called directly.
There was a problem hiding this comment.
The other still unholy but different unholy option would be to assign newSources to a variable scoped outside of the updater function and save outside of the updater function, but not sure if that is better?
There was a problem hiding this comment.
@sanchitmalhotra126 I recall you having a proposal for dealing with the codebridge style of needing to do operations on the sources before saving. Would that be useful here?
There was a problem hiding this comment.
Found the proposal here. I'm not sure this wouldn't be subject to race conditions since we assume the source is always up to date with recent changes.
There was a problem hiding this comment.
Discussed this in the office briefly; I'd be curious if using currentSources instead of serializedData (below) would accomplish the same thing, since the value of currentSources should always be the most up to date version of sources (whatever was last saved/updated).
And yep, similarly in that proposal, doing the codebridge style of updates still requires you to pass in the value of currentSources, but it should be the most up to date version.
There was a problem hiding this comment.
Cool thanks Sanchit! I will try and simplify this tomorrow AM.
3 participants
Follow-up to #70674
In adding an update to sources that waits for image uploads to finish in the PR above, I realized that I think there's a possible race condition here, where an out of date set of serialized data from Excalidraw could be saved to sources.
I think the solution here is to make our source state updates use an updater function, which is the recommended way to make state updates that depend on previous state: https://react.dev/reference/react/useState#updating-state-based-on-the-previous-state
We already do this in our SourcesContainer, but the update here is to allow a consumer of SourcesContainer (eg, SketchlabView) to also use this previous state.
Testing story
I tested manually that I'm still able to upload images / update state successfully with the ?s3-images=true URL param.
Follow-up work
I think after this I'm ready to turn on S3 image storage! 🎉