fix(scratch-vm): repair missing variable and broadcast definitions on project load#555
Merged
cwillisf merged 4 commits intohotfix/scratch-blocks-release-triagefrom Apr 30, 2026
Conversation
… load Projects saved while affected by the missing-definitions bug carry dangling variable, list, or broadcast references that point at ids not defined anywhere in the project. PR #554 stops new occurrences but doesn't repair already-saved projects, because installTargets skips reconciliation when wholeProject=true. Extracts a new public method `Target.reconcileVariableReferences` that runs only the missing-definitions phase of `fixUpVariableReferences`: walk every variable / list / broadcast reference; for each id not found anywhere, look up by name and type on the stage and either remap to an existing global or create a fresh one. The disambiguation phases of `fixUpVariableReferences` (rename sprite-local that name-collides with a stage global, rename unreferenced colliding locals) are left in `fixUpVariableReferences` itself, which now delegates the repair phase to the new helper. This separation matters because running the rename branches on clean projects would rename legitimately-existing local-vs-global name collisions, which has been a valid Scratch configuration since forever. `installTargets` now calls `reconcileVariableReferences` on every target during whole-project load, so corrupted projects self-heal on next open. Logs a warning per definition created or remapped so the repair surfaces in dev tools and in telemetry. Quiet on clean loads. Refs #533
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a conservative “self-heal on load” path in scratch-vm to repair historically corrupted projects that contain variable/list/broadcast block fields referencing IDs that aren’t defined anywhere in the project, without renaming legitimate local-vs-global name collisions.
Changes:
- Introduce
Target.reconcileVariableReferences()to repair missing variable/list/broadcast definitions (create on stage or remap by same name/type) without renaming. - Refactor
Target.fixUpVariableReferences()to call reconciliation first, then apply import/backpack-only disambiguation renames for sprite-local collisions. - Wire whole-project loading (
VirtualMachine.installTargets(..., wholeProject=true)) to reconcile every target on load; add unit coverage for the new behavior and regression guards.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/scratch-vm/src/engine/target.js | Splits reconciliation from renaming; adds logging on create/remap and updates import logic to reuse the helper. |
| packages/scratch-vm/src/virtual-machine.js | Runs reconciliation across all targets during whole-project load to repair corrupted projects without renames. |
| packages/scratch-vm/test/unit/engine_target.js | Adds unit tests for reconciliation behavior and log.warn emission/silence cases. |
| packages/scratch-vm/test/unit/virtual-machine.js | Adds integration tests to ensure whole-project loads reconcile dangling refs and don’t rename valid local-vs-global collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
When two dangling references in one project share an original name and type and the name has to be bumped (because some other target already owns that name), the second reference must remap to the stage variable the first reference created rather than create a second variable. A Scratcher who pasted scripts referencing what they called the same name almost certainly meant one variable. Tracks created stage variables by their original (pre-bump) name+type within the reconciliation pass and remaps subsequent same-name dangling references to the earlier-created variable. Also pushes the fresh name into the cached name list so unusedName accounts for it on later calls.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a project-load-time repair path in scratch-vm to self-heal projects that already contain dangling variable/list/broadcast references (missing definitions), without triggering the import-time renaming/disambiguation behavior that would be a regression for valid local-vs-global name collisions.
Changes:
- Introduces
Target.reconcileVariableReferences()to repair missing variable/list/broadcast definitions (create/remap on stage) without renaming existing variables. - Refactors
Target.fixUpVariableReferences()to callreconcileVariableReferences()first, then run the existing import-time local/global disambiguation renames. - Wires
VirtualMachine.installTargets(..., wholeProject=true)to run reconciliation across all targets on full project load, and adds unit tests covering load behavior and regressions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/scratch-vm/src/engine/target.js | Adds reconcileVariableReferences, refactors fixUpVariableReferences to delegate repair vs rename phases, and emits log.warn on repairs. |
| packages/scratch-vm/src/virtual-machine.js | Runs reconciliation for every target during whole-project load to repair historical dangling refs. |
| packages/scratch-vm/test/unit/engine_target.js | Adds unit tests for reconciliation behavior, idempotency, collision non-renaming, and logging behavior. |
| packages/scratch-vm/test/unit/virtual-machine.js | Adds unit tests ensuring whole-project load repairs dangling refs and does not rename valid local-vs-global collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…onciliation When whole-project load reconciles every target in turn, an earlier target's pass can create a stage variable with a bumped name. A later target with a block referencing the same id by lookup would otherwise keep its original displayed name, leaving the same variable rendered with different names in different sprites' blocks. In `reconcileVariableReferences`, when `lookupVariableById` succeeds, queue a name update if the field's displayed value doesn't match the resolved variable's current name. The existing `conflictNamesToReplace` pass applies the update.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a conservative “repair-only” reconciliation pass for variable/list/broadcast references during whole-project load, allowing historically corrupted projects (dangling ids) to self-heal on next open without renaming legitimate local-vs-global name collisions.
Changes:
- Introduces
Target.reconcileVariableReferences()to repair missing variable/list/broadcast definitions and remap dangling ids to existing stage globals when possible. - Refactors
Target.fixUpVariableReferences()to delegate the missing-definition phase toreconcileVariableReferences()while keeping import/paste disambiguation renames. - Updates
VirtualMachine.installTargets()to run reconciliation across all targets whenwholeProject=true, plus adds unit tests for the new helper and wiring.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/scratch-vm/src/engine/target.js | Adds reconcileVariableReferences, updates JSDoc, and refactors fixUpVariableReferences to call reconcile first. |
| packages/scratch-vm/src/virtual-machine.js | Runs reconcileVariableReferences() across targets during whole-project installs to repair dangling references on load. |
| packages/scratch-vm/test/unit/engine_target.js | Adds unit tests for reconcileVariableReferences, including logging and regression guards. |
| packages/scratch-vm/test/unit/virtual-machine.js | Adds tests ensuring whole-project installs repair dangling references without renaming valid local-vs-global collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds two Tap tests for the list-reference path through reconcileVariableReferences (creation on stage and remap to an existing same-name stage list), filling a coverage gap noted in review. Reword the warning emitted when reconciliation only normalizes a stale displayed name; the previous wording said "dangling reference" but in that branch the id resolves to a real variable.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a conservative “repair on load” path in scratch-vm to self-heal projects which contain dangling variable/list/broadcast IDs (from historical import/backpack bugs) without renaming legitimate local-vs-global name collisions.
Changes:
- Introduces
Target.reconcileVariableReferences()to repair missing variable/list/broadcast definitions by remapping to existing stage globals or creating new stage definitions (no renames). - Refactors
Target.fixUpVariableReferences()to call reconciliation first, then keep the import/backpack-specific rename/disambiguation behavior. - Wires whole-project loads (
VirtualMachine.installTargets(..., wholeProject=true)) to reconcile every target on install; adds unit tests covering reconciliation behavior and installTargets wiring.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/scratch-vm/src/engine/target.js | Adds reconciliation-only helper and refactors fix-up to delegate missing-definition repair. |
| packages/scratch-vm/src/virtual-machine.js | Runs reconciliation across all targets during whole-project load. |
| packages/scratch-vm/test/unit/engine_target.js | Adds unit tests for reconciliation cases and log.warn behavior. |
| packages/scratch-vm/test/unit/virtual-machine.js | Adds unit tests ensuring whole-project load repairs dangling refs without renaming clean collisions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cwillisf
merged commit b437181
into
hotfix/scratch-blocks-release-triage
17 checks passed
This file contains hidden or 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
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.
Resolves
Companion to #554. That PR stops new occurrences of the missing-definitions bug (backpack paste / sprite import leaving variable, list, or broadcast references that point at ids not defined anywhere in the project). This PR repairs already-corrupted projects on next load, so a Scratcher whose project hit the bug between the unfork release and #554's deploy gets a clean workspace the next time they open it.
Proposed Changes
Targetgets a new public methodreconcileVariableReferencesthat runs only the missing-definitions phase offixUpVariableReferences: walk every variable, list, and broadcast reference; for each id not found anywhere, look up by name and type on the stage and either remap the field id to an existing global or create a fresh one. When two dangling references in the same target share an original name+type and the name has to be bumped, they coalesce to one stage variable rather than producing two with similar names. When a previous target's pass on the same load created a stage variable with a bumped name, later targets referencing the same id by lookup have their displayed names normalized to match.fixUpVariableReferencesnow delegates the repair phase to the new helper, keeping its disambiguation phases (rename sprite-local that name-collides with a stage global, rename unreferenced colliding locals) for the sprite-import and backpack-paste callers.VirtualMachine.installTargetsnow callsreconcileVariableReferenceson every target during whole-project load (wholeProject=true). Idempotent on clean projects; self-heals corrupted ones on next open.A
log.warnfires whenever reconciliation modifies project state: when it creates a definition on the stage, remaps a reference to an existing global, coalesces two same-name dangling references, or normalizes a stale displayed name on a block field. Quiet on clean loads. Useful for surfacing the repair in dev tools and telemetry.Reason for Changes
The repair-only path matters because running the full
fixUpVariableReferenceson every loaded sprite would also rename any sprite-local variable whose name and type match a stage global. That configuration has been a valid Scratch setup since forever; renaming such variables on project load would be a far worse regression than the bug we're fixing. Splitting the function lets the load path stay conservative while keeping the import path's disambiguation behavior intact.A reference whose id is missing but whose name+type matches an unrelated stage variable will be remapped to that stage variable rather than create a new one. Same trade-off as sprite import: we can't tell from the saved field whether the original was meant to be a different variable. Flagging this as a known corner case rather than working around it.
Test Coverage
packages/scratch-vm/test/unit/engine_target.js: new Tap tests covering the reconciliation cases for variables, lists, and broadcasts (creation, same-name remap, idempotency); the critical regression that local-vs-global name collisions are NOT renamed; the stage-as-thiscase; cross-target displayed-name normalization after a bump; same-original-name coalescing within a target; andlog.warnassertions (fires on create, fires on remap, silent on clean references).packages/scratch-vm/test/unit/virtual-machine.js: new Tap tests for theinstallTargetswiring on whole-project loads. One covers repair of dangling variable + broadcast references; one is a regression guard that legitimate local-vs-global name collisions survive a whole-project load unchanged.npm test --workspace=packages/scratch-vmpasses (3761 / 3761).npm run buildandnpm testfrom the repo root pass on every workspace.