Skip to content

Conversation

@mikeharv
Copy link
Contributor

@mikeharv mikeharv commented Nov 19, 2025

This change removes the use of sessionStorage for saving the generated Blockly flyout/toolbox and instead persists it directly in project sources as a new toolboxDefinition field. When a generated flyout is created, it’s now serialized into the project’s main.json and re-applied on load.

To prevent saveBlocks from overwriting this field due to stale state, a small mergeSources wrapper was added in DanceView. The wrapper uses a ref to always merge updates into the latest currentSources before calling updateSources, 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

  • Jira:

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Creation Checklist:

  • Tests provide adequate coverage
  • Privacy impacts have been documented
  • Security impacts have been documented
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Follow-up work items (including potential tech debt) are tracked and linked

@mikeharv mikeharv marked this pull request as ready for review November 19, 2025 16:15
@mikeharv mikeharv requested review from a team and sanchitmalhotra126 November 19, 2025 16:15
Comment on lines +176 to +177
updateSources({workspaceSerialization, flyoutDefinition});
onFlyoutGenerated(flyoutDefinition);
Copy link
Contributor

@sanchitmalhotra126 sanchitmalhotra126 Nov 19, 2025

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. Call updateSources (which schedules the source change for the next render).
  2. Immediately call onFlyoutGenerated to update the toolbox in the current render.
  3. Then call runProgram(), which triggers saveBlocks(\) 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.

Copy link
Contributor

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.

Copy link
Contributor

@sanchitmalhotra126 sanchitmalhotra126 left a 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!

@mikeharv mikeharv merged commit 47e69ca into staging Nov 19, 2025
6 checks passed
@mikeharv mikeharv deleted the mike/save-flyout branch November 19, 2025 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants