Skip to content

Trigger Scratch remix on first save of educator projects#1397

Merged
abcampo-iry merged 6 commits intomainfrom
issues/1238-trigger-remix-when-saving-for-the-first-time
Mar 25, 2026
Merged

Trigger Scratch remix on first save of educator projects#1397
abcampo-iry merged 6 commits intomainfrom
issues/1238-trigger-remix-when-saving-for-the-first-time

Conversation

@abcampo-iry
Copy link
Contributor

@abcampo-iry abcampo-iry commented Mar 20, 2026

Closes: https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1238

Summary

This updates the Scratch flow so the first save of a saved Scratch project that the current user does not own sends a Scratch remix instead of a normal save.

It also keeps the save button in the Scratch saving state through remix events, updates the parent project identifier when Scratch reports the remixed id, and avoids reloading the Scratch iframe by keeping the iframe bound to its original project id.

What changed

  • Track Scratch remix start, success, and failure in the shared save-state hook.
  • Store the original Scratch iframe project id separately from the parent project id.
  • Update the parent project id from scratch-gui-project-id-updated.
  • Use the updated parent id to make remix happen only on the first save.
  • Keep the iframe src pinned to the original Scratch project id so the iframe does not reload after remix.
  • Extract Scratch save helpers so the generic save button and Scratch project bar share the same save/remix.

Behaviour

  • Pressing save for the first time when viewing an educator project should trigger a scratch remix
  • The save button should be in it's 'saving' state until the remix is completed.
  • The page or Scratch iframe shouldn't reload (this could be split out if complex)
remix-flow.mov

Handle Scratch remix start, success, and failure events in the shared save-state hook, and forward Scratch remix creation failures from the iframe host page.
Store the original Scratch iframe project identifier, update the parent project id when Scratch posts a remixed id, and use that state to remix only on the first save without reloading the iframe.
Move iframe-specific message handling into shared Scratch helpers and centralize the Scratch save button wiring in a hook so the first-save remix flow reads closer to the regular project save flow.
Copilot AI review requested due to automatic review settings March 20, 2026 14:41
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 20, 2026 14:42 — with GitHub Actions Inactive
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Scratch editor save flow so that when a user saves a Scratch project they don’t own, the first save triggers a Scratch “remix” instead of a normal save, while keeping the UI save state consistent and preventing the Scratch iframe from reloading after the remix identifier is issued.

Changes:

  • Adds remix-aware save state handling (start/success/failure) and a shared Scratch save/remix pathway used by both the generic Save button and the Scratch project bar.
  • Tracks the original Scratch iframe project identifier separately from the parent project identifier, and updates the parent identifier when Scratch reports a new remixed id.
  • Subscribes to Scratch GUI “project id updated” messages and pins the iframe project_id to the original identifier to avoid iframe reloads.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/utils/scratchIframe.js Adds remix decision helper and message subscription; updates postMessage target origin handling.
src/utils/scratchIframe.test.js Adds tests for new scratch iframe helpers and identifier update subscription.
src/scratch.jsx Emits remix-failed event for Scratch GUI “creatingError” alerts.
src/redux/reducers/loadProjectReducers.js Initializes and sets scratchIframeProjectIdentifier during project load lifecycle.
src/redux/reducers/loadProjectReducers.test.js Updates reducer expectations for new scratch iframe identifier state.
src/redux/EditorSlice.js Stores original Scratch iframe identifier on project set; adds action to update only parent identifier.
src/redux/EditorSlice.test.js Tests scratch iframe identifier initialization and parent-id-only updates.
src/hooks/useScratchSaveState.js Extends save-state lifecycle to include remix events; supports remix command.
src/hooks/useScratchSaveState.test.js Updates tests for remix command behavior and remix lifecycle.
src/hooks/useScratchSave.js New hook encapsulating Scratch save/remix eligibility + save state wiring.
src/components/WebComponentProject/WebComponentProject.integration.test.js Ensures identifier-changed event fires when remix updates the parent identifier.
src/components/SaveButton/SaveButton.jsx Routes Scratch projects through shared Scratch save/remix hook; disables during Scratch saving/remixing.
src/components/SaveButton/SaveButton.test.js Adds coverage that first Scratch save triggers remix via postMessage instead of triggerSave.
src/components/ProjectBar/ScratchProjectBar.jsx Uses shared Scratch save/remix hook and passes remix intent on click.
src/components/ProjectBar/ScratchProjectBar.test.js Adds coverage for remix-on-first-save and remix “saving” UI state.
src/components/Editor/Project/ScratchContainer.jsx Subscribes to Scratch id updates and pins iframe to original identifier while updating parent identifier.
src/components/Editor/Project/ScratchContainer.test.js Tests parent identifier updates without changing iframe project_id.
Comments suppressed due to low confidence (1)

src/components/Editor/Project/ScratchContainer.jsx:38

  • iframeSrcUrl is built by string-concatenating process.env.ASSETS_URL. If ASSETS_URL is configured with a trailing slash, this produces a double-slash path; if it's ever configured as a path (e.g. "/") it can produce a protocol-relative URL like //scratch.html pointing at the wrong host. Building this with the URL constructor (and ensuring the base is an absolute URL) avoids malformed iframe URLs and keeps the iframe origin aligned with the postMessage origin checks.
  const iframeSrcUrl = `${
    process.env.ASSETS_URL
  }/scratch.html?${queryParams.toString()}`;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +8 to +10
const allowedOrigin = process.env.ASSETS_URL || window.location.origin;
getScratchIframeContentWindow().postMessage(message, allowedOrigin);
};
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postMessage targetOrigin must be an origin (scheme+host+port) or "*". Using process.env.ASSETS_URL directly can be invalid if it contains a path or trailing slash (or is set to something like "/" via CRA publicUrl), which will throw at runtime and/or fail origin checks. Consider normalizing via new URL(process.env.ASSETS_URL, window.location.href).origin (with a safe fallback) and reusing the same normalization anywhere you compare event.origin to the allowed origin (e.g. useScratchSaveState).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@abcampo-iry abcampo-iry marked this pull request as draft March 24, 2026 11:54
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from 6bd0aac to dc02a70 Compare March 24, 2026 12:31
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 12:31 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from dc02a70 to 6f88606 Compare March 24, 2026 12:37
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 12:37 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from 6f88606 to 08d2e2f Compare March 24, 2026 13:25
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 13:25 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from 08d2e2f to 7b89eab Compare March 24, 2026 13:33
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 13:33 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from 7b89eab to db9527f Compare March 24, 2026 13:46
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 13:47 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from db9527f to 051923c Compare March 24, 2026 14:05
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 14:05 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from 051923c to 3728ad2 Compare March 24, 2026 14:10
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 24, 2026 14:11 — with GitHub Actions Inactive
@abcampo-iry abcampo-iry marked this pull request as ready for review March 24, 2026 14:11
@abcampo-iry
Copy link
Contributor Author

I think I was trying to add the functionality to save button but this button doesn't really exist in the scratch flow, I was going through some rabbithole.

After that is simplified and this covers the paths I was looking for.

@abcampo-iry abcampo-iry force-pushed the issues/1238-trigger-remix-when-saving-for-the-first-time branch from 3728ad2 to 9768186 Compare March 25, 2026 07:58
@abcampo-iry abcampo-iry temporarily deployed to previews/1397/merge March 25, 2026 07:58 — with GitHub Actions Inactive
@zetter-rpf
Copy link
Contributor

Do you think this should have a browser based cypress test?

Copy link
Contributor

@zetter-rpf zetter-rpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me, just that question about if we should have e2e test coverage.

Since this is a larger change it would be good for someone else to take a look - could you have a look @DNR500 since you have worked in this area too.

@zetter-rpf zetter-rpf requested a review from DNR500 March 25, 2026 09:07
Copy link
Contributor

@DNR500 DNR500 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like dealing with the rendering of the iFrame was a bit tricky - well done on sorting that out.

There is a minor nit pick on there but otherwise good work!

<DesignSystemButton
className="project-bar__btn btn--save btn--primary"
onClick={saveScratchProject}
onClick={() => saveScratchProject({ shouldRemixOnSave })}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: minor point but I think we pass out shouldRemixOnSave from the useScratchSave only to pass it back in. I don't think that shouldRemixOnSave is used anywhere else?

Maybe it could be just used in useScratchSave something like..

const {
  saveScratchProject: triggerScratchSaveOrRemixCommand,
  scratchSaveLabelKey,
  isScratchSaving,
} = useScratchSaveState({ enabled: enableScratchSaveState });

const saveScratchProject = () =>
  triggerScratchSaveOrRemixCommand({ shouldRemixOnSave });

and usage here 'onClick={() => saveScratchProject()} '

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @DNR500, I do prefer the explicitness in the callback.
I think it obscure a little bit less the function.

When you are looking at the code it makes you think (or at least it does to me) that the button has more than one behaviour if passed there.

@abcampo-iry abcampo-iry merged commit 81e4aa5 into main Mar 25, 2026
7 checks passed
@abcampo-iry abcampo-iry deleted the issues/1238-trigger-remix-when-saving-for-the-first-time branch March 25, 2026 11:37
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.

4 participants