-
Notifications
You must be signed in to change notification settings - Fork 524
[Hour of AI] Save generated Dance flyout to project sources #69653
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
Conversation
| updateSources({workspaceSerialization, flyoutDefinition}); | ||
| onFlyoutGenerated(flyoutDefinition); |
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.
One thought: instead of having two "update" pathways (necessitating the mergeSources handler), would it work if we either a) only called updateSources and update the toolbox in the subsequent useEffect that runs and replaces the blocks in the workspace, or b) only update the workspace and flyout directly (i.e. loadBlocksToWorkspace/updateToolbox), and let the onBlockSpaceChange callback run and save blocks + flyout there?
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.
Option a) could work, but it would require deferring runProgram() until after the workspace and toolbox are reloaded in the following render. Option b) likely wouldn’t persist the flyout correctly, since updating the toolbox doesn’t trigger the block-space change listener that saves sources.
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.
Oh hmm interesting...well right now wouldn't we have the same issue because the workspace blocks aren't updated until the useEffect runs anyway?
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.
You're right tha the workspace blocks don’t get updated until the useEffect runs.
After generating, I think it's going like this:
- Call
updateSources(which schedules the source change for the next render). - Immediately call
onFlyoutGeneratedto update the toolbox in the current render. - Then call
runProgram(), which triggerssaveBlocks(\)before React has re-rendered with the new sources.
So the workspace update does wait for the useEffect, but the save happens sooner, using the old sources.
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.
Gotcha! Chatted offline - I keep forgetting that the currentSources are stale which means that the toolbox gets erased.
sanchitmalhotra126
left a comment
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.
Thanks for navigating a tricky area!
This change removes the use of
sessionStoragefor saving the generated Blockly flyout/toolbox and instead persists it directly in project sources as a newtoolboxDefinitionfield. When a generated flyout is created, it’s now serialized into the project’smain.jsonand re-applied on load.To prevent
saveBlocksfrom overwriting this field due to stale state, a smallmergeSourceswrapper was added inDanceView. The wrapper uses a ref to always merge updates into the latestcurrentSourcesbefore callingupdateSources, ensuring multiple quick saves don’t clobber each other. [Slack discussion]Together, these changes make the generated toolbox persist across reloads and tabs while keeping project source updates consistent.
Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Creation Checklist: