From dac0f034da8dc3495c185b1a8d5c4bf9cb69fe56 Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 04:57:33 +0900 Subject: [PATCH 1/8] fix: use posixPath for webdriver file:// URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generators and compile webdriver scripts built `file://` URLs by concatenating `__dirname` directly. On Windows `__dirname` contains backslashes, which yields a malformed URL like `file://S:\source\blockly\.../tests/compile/index.html` that Chrome refuses to load — making the `generators` and `advanced_compile_in_browser` test steps fail. Route both URLs through the existing `posixPath()` helper in `scripts/helpers.js`, matching the pattern already used in `tests/mocha/webdriver.js`. The helper is a no-op on POSIX so this keeps existing Linux/macOS behaviour unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/blockly/tests/compile/webdriver.js | 3 ++- packages/blockly/tests/generators/webdriver.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/blockly/tests/compile/webdriver.js b/packages/blockly/tests/compile/webdriver.js index 3e07ca23ec2..26332479002 100644 --- a/packages/blockly/tests/compile/webdriver.js +++ b/packages/blockly/tests/compile/webdriver.js @@ -9,6 +9,7 @@ * Chrome, via webdriver. */ const webdriverio = require('webdriverio'); +const {posixPath} = require('../../scripts/helpers'); /** @@ -51,7 +52,7 @@ async function runCompileCheckInBrowser() { }; } - const url = 'file://' + __dirname + '/index.html'; + const url = 'file://' + posixPath(__dirname) + '/index.html'; console.log('Starting webdriverio...'); const browser = await webdriverio.remote(options); diff --git a/packages/blockly/tests/generators/webdriver.js b/packages/blockly/tests/generators/webdriver.js index 004b5c878a2..3241953701c 100644 --- a/packages/blockly/tests/generators/webdriver.js +++ b/packages/blockly/tests/generators/webdriver.js @@ -10,6 +10,7 @@ var webdriverio = require('webdriverio'); var fs = require('fs'); var path = require('path'); +var {posixPath} = require('../../scripts/helpers'); /** @@ -60,7 +61,7 @@ async function runGeneratorsInBrowser(outputDir) { options.capabilities['goog:chromeOptions'].args.push('--disable-gpu'); } - var url = 'file://' + __dirname + '/index.html'; + var url = 'file://' + posixPath(__dirname) + '/index.html'; var prefix = path.join(outputDir, 'generated'); console.log('Starting webdriverio...'); From bf66b7886fa8a5c0208f257bb1f734045c7fa377 Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 04:57:49 +0900 Subject: [PATCH 2/8] chore: force LF line endings via .gitattributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `eslint-plugin-prettier` inherits Prettier's `endOfLine: 'lf'` default, so any source file checked out with CRLF on a Windows machine that has `core.autocrlf=true` fails lint with `Delete \`␍\`` errors. Setting `* text=auto eol=lf` makes git always check out detected text files with LF regardless of the user's local autocrlf setting, removing the class of failures at its source. Co-Authored-By: Claude Opus 4.6 (1M context) --- .gitattributes | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.gitattributes b/.gitattributes index 176a458f94e..81038ee4e15 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,4 @@ -* text=auto +# Force LF on checkout for all detected text files. Without this, Windows +# users with core.autocrlf=true get CRLF in source files, which makes +# Prettier (run as an ESLint rule) fail with "Delete `␍`" errors. +* text=auto eol=lf From 37b6c8624e3136124440dee365be95dbffa914d8 Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 04:58:23 +0900 Subject: [PATCH 3/8] fix: invoke validate-renamings.mjs via node The renamings test step ran the `.mjs` file directly via execSync, relying on its `#!/usr/bin/env node` shebang. Windows `cmd.exe` does not honor shebangs and `.mjs` has no default file association, so the step failed with "is not recognized as an internal or external command". Prefix the invocation with `node` so it works on every platform. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/blockly/scripts/gulpfiles/test_tasks.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/blockly/scripts/gulpfiles/test_tasks.mjs b/packages/blockly/scripts/gulpfiles/test_tasks.mjs index 37f9884440d..8b0270af708 100644 --- a/packages/blockly/scripts/gulpfiles/test_tasks.mjs +++ b/packages/blockly/scripts/gulpfiles/test_tasks.mjs @@ -153,7 +153,7 @@ function build() { * @return {Promise} Asynchronous result. */ function renamings() { - return runTestCommand('renamings', 'tests/migration/validate-renamings.mjs'); + return runTestCommand('renamings', 'node tests/migration/validate-renamings.mjs'); } /** From 26febe9b82efd6ad707b6e3c9668def0477340ef Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 04:59:02 +0900 Subject: [PATCH 4/8] fix: auto-create blockly-test junction for node tests on Windows `tests/node/node_modules/blockly-test` is a git symlink (mode 120000) pointing at `../../../dist`. With `core.symlinks` disabled (the default on Windows) git checks it out as a 13-byte text file containing the link target literally, so `import 'blockly-test'` in `tests/node/run_node_test.mjs` fails with `ERR_MODULE_NOT_FOUND` and the `node` test step never runs. Add an `ensureBlocklyTestLink` helper that runs immediately before the node test step. It detects the broken state via `lstatSync`, replaces the file with a junction (which works on Windows without admin rights and is treated as a regular symlink on POSIX), then best-effort marks the path `skip-worktree` so the resulting "deleted symlink" diff stays out of `git status`. Unexpected failures of the skip-worktree step are surfaced as a warning rather than silently swallowed. The helper is idempotent and a no-op on Linux/macOS, where the git checkout already produces a usable symlink. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../blockly/scripts/gulpfiles/test_tasks.mjs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/blockly/scripts/gulpfiles/test_tasks.mjs b/packages/blockly/scripts/gulpfiles/test_tasks.mjs index 8b0270af708..a47c0918d09 100644 --- a/packages/blockly/scripts/gulpfiles/test_tasks.mjs +++ b/packages/blockly/scripts/gulpfiles/test_tasks.mjs @@ -353,11 +353,48 @@ export async function generators() { }); } +/** + * Ensure tests/node/node_modules/blockly-test resolves to the packaged + * blockly bundle in dist/. + * + * The repository stores this path as a git symlink (mode 120000) pointing at + * ../../../dist. On Windows without core.symlinks enabled (the default), git + * checks it out as a plain text file containing the link target, which makes + * `import 'blockly-test'` in tests/node/run_node_test.mjs fail with + * ERR_MODULE_NOT_FOUND. Detect that case and replace it with a junction so the + * node tests can resolve the packaged blockly bundle. Also mark the path as + * skip-worktree so the resulting "deleted symlink" diff doesn't pollute + * `git status`. + */ +function ensureBlocklyTestLink() { + const linkPath = path.join('tests', 'node', 'node_modules', 'blockly-test'); + // throwIfNoEntry:false → returns undefined instead of throwing on ENOENT, + // so unrelated errors (EACCES, etc.) still propagate normally. + const stat = fs.lstatSync(linkPath, {throwIfNoEntry: false}); + if (stat && (stat.isSymbolicLink() || stat.isDirectory())) return; + if (stat) fs.rmSync(linkPath, {force: true}); + fs.mkdirSync(path.dirname(linkPath), {recursive: true}); + fs.symlinkSync(path.resolve(RELEASE_DIR), linkPath, 'junction'); + // Best-effort: silence the resulting "deleted symlink" diff in git status. + // Expected failure modes (not a git checkout, git not on PATH, file not in + // index) are non-fatal — the junction itself is enough to make the test + // pass. Surface anything unexpected as a warning so it isn't fully hidden. + try { + execSync(`git update-index --skip-worktree ${linkPath}`, {stdio: 'pipe'}); + } catch (e) { + console.warn( + `ensureBlocklyTestLink: could not mark ${linkPath} skip-worktree ` + + `(${e.stderr?.toString().trim() || e.message}). ` + + `'git status' may show this file as deleted.`); + } +} + /** * Run Node tests. * @return {Promise} Asynchronous result. */ function node() { + ensureBlocklyTestLink(); return runTestCommand('node', 'mocha tests/node --config tests/node/.mocharc.js'); } From 1d31b0ba8eb1a3f72d4183c2bc50e42afb5982ad Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 03:44:18 +0900 Subject: [PATCH 5/8] feat: allow variable fields on shadow blocks Drop the throw in FieldVariable.setSourceBlock that prevented a FieldVariable from attaching to a shadow block. This is the minimum precondition for using variables_get as a shadow under any value input; without it the field cannot even be constructed in a shadow context. This commit intentionally does NOT touch the variable deletion cascade, respawn machinery, or any other related code path. Those concerns are exercised by the accompanying test suite (tests/mocha/shadow_variable_fields_test.js) and will be addressed by a follow-up redesign. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/blockly/core/field_variable.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/blockly/core/field_variable.ts b/packages/blockly/core/field_variable.ts index aa4fdfe310f..cd44949d9e9 100644 --- a/packages/blockly/core/field_variable.ts +++ b/packages/blockly/core/field_variable.ts @@ -14,7 +14,6 @@ // Unused import preserved for side-effects. Remove if unneeded. import './events/events_block_change.js'; -import type {Block} from './block.js'; import {Field, FieldConfig, UnattachedFieldError} from './field.js'; import { FieldDropdown, @@ -279,18 +278,6 @@ export class FieldVariable extends FieldDropdown { this.setValue(variable.getId()); } - /** - * Attach this field to a block. - * - * @param block The block containing this field. - */ - override setSourceBlock(block: Block) { - if (block.isShadow()) { - throw Error('Variable fields are not allowed to exist on shadow blocks.'); - } - super.setSourceBlock(block); - } - /** * Get the variable's ID. * From 96ba78534e70a30fb0a78ca662c41558aa000c2c Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 03:52:26 +0900 Subject: [PATCH 6/8] test(playground): add Shadow Variables toolboxes for manual verification Add "Shadow Variables" and "Shadow Variables (typed)" toolbox options to tests/playground.html that mirror the block types defined inline in tests/mocha/shadow_variable_fields_test.js (shadow_var_parent, shadow_var_wrapper, shadow_var_two_field) plus a small statement-style scaffold (shadow_var_host), so an operator can reach every scenario the spec exercises from the playground UI: - shadow_var_parent + shadow variables_get basic / Case 1 / Case 2 / rename / delete cascade / save-load - shadow_var_parent + shadow shadow_var_wrapper + shadow variables_get nested shadow - shadow_var_parent + shadow shadow_var_two_field mixed Case 1 / Case 2 - shadow_var_host + shadow_var_parent + shadow variables_get shadow_var_parent attached to a real statement parent The typed variant routes the same shapes through variables_get_dynamic and the dynamic Variables category, exercising the FieldVariable shadow patch on the type-enforcing onchange path. The block types are defined inline via Blockly.defineBlocksWithJsonArray in the existing module script of playground.html; no new files are introduced. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/blockly/tests/playground.html | 187 +++++++++++++++++++++++++ 1 file changed, 187 insertions(+) diff --git a/packages/blockly/tests/playground.html b/packages/blockly/tests/playground.html index 4ae723ce0f6..a8e3c85bc32 100644 --- a/packages/blockly/tests/playground.html +++ b/packages/blockly/tests/playground.html @@ -19,6 +19,68 @@ await loadScript('playgrounds/screenshot.js'); await loadScript('../node_modules/@blockly/dev-tools/dist/index.js'); + // Test-only blocks for the Shadow Variables toolboxes. + // + // shadow_var_parent / shadow_var_wrapper / shadow_var_two_field mirror + // the three block types defined inline in + // tests/mocha/shadow_variable_fields_test.js so an operator can reach + // the same scenarios as that spec from the playground UI. + // + // shadow_var_host is playground-only scaffolding: a statement block + // with one value input that gives shadow_var_parent (a value block) + // somewhere to plug in so the workspace tree has a proper top-level + // statement parent instead of dangling value blocks. It is not part of + // the spec. + Blockly.defineBlocksWithJsonArray([ + { + 'type': 'shadow_var_host', + 'message0': 'host %1', + 'args0': [ + { + 'type': 'input_value', + 'name': 'VALUE', + }, + ], + 'previousStatement': null, + 'nextStatement': null, + 'colour': 230, + }, + { + 'type': 'shadow_var_parent', + 'message0': 'parent %1', + 'args0': [ + { + 'type': 'input_value', + 'name': 'VALUE', + }, + ], + 'output': null, + 'colour': 330, + }, + { + 'type': 'shadow_var_wrapper', + 'message0': 'wrap %1', + 'args0': [ + { + 'type': 'input_value', + 'name': 'INNER', + }, + ], + 'output': null, + 'colour': 330, + }, + { + 'type': 'shadow_var_two_field', + 'message0': 'two %1 %2', + 'args0': [ + {'type': 'field_variable', 'name': 'VAR1', 'variable': 'item'}, + {'type': 'field_variable', 'name': 'VAR2', 'variable': 'item'}, + ], + 'output': null, + 'colour': 330, + }, + ]); + var workspace = null; function start() { @@ -139,6 +201,8 @@ 'toolbox-categories-typed-variables', 'toolbox-simple', 'toolbox-test-blocks', + 'toolbox-shadow-variables', + 'toolbox-shadow-variables-typed', ]; var toolboxSuffix = getToolboxSuffix(); document.forms.options.elements.toolbox.selectedIndex = @@ -548,6 +612,8 @@

Blockly Playground

+ +

@@ -1276,5 +1342,126 @@

Blockly Playground

categorystyle="procedure_category" custom="PROCEDURE"> + + + + + + From f7e55b26ca5f4cb142ffae4343c6ba53a9626271 Mon Sep 17 00:00:00 2001 From: B_head Date: Thu, 9 Apr 2026 03:52:18 +0900 Subject: [PATCH 7/8] test: add executable spec for shadow variables_get Add tests/mocha/shadow_variable_fields_test.js (and register it in tests/mocha/index.html) as an executable specification for the "shadow variables_get" feature. The suite encodes the quality requirements that were distilled from the design discussion driving this work: #1 Public API side-effect freedom: attaching a shadow variables_get does not fire VarCreate or change getAllVariables(). #2 Symmetric behavior: shadow and non-shadow variables_get on the same id share the same VariableModel; rename and code generation flow through the same path; serialization round-trips preserve the field id. #3 Variable delete cascade processes shadow uses (no special skipping by isShadow()). #4 Undo restores both the variable and the shadow display, with the shadow block id preserved across delete/undo cycles. #5 Case 2 (parent template references the deleted variable, reached by save / load) does not crash, throw, or leave the variable map inconsistent. #6 deleteVariable does not throw for inputs the patch enables. Coverage areas (29 active tests + 4 acceptance criteria as test.skip): - basic shadow attachment and identity sharing with non-shadow - no spurious VarCreate / getAllVariables churn at attachment time - Case 1 cascade reset (live shadow value differs from template) - Case 2 cascade behavior (parent template references the deleted variable, reached via save / load round-trip) - symmetry with non-shadow variables_get for rename / cascade / code generation / save-load - rename propagation and undo - load auto-creates a missing variable (parity with non-shadow) - deletion of an unrelated variable leaves the shadow alone - chained deleteVariable calls without intermediate undo - BlockChange event recording for cascade resets - nested deleteVariable from a change listener - workspace.clear() with shadow variables_get blocks - non-default variable type round-trip - mixed Case 1 / Case 2 fields in one shadow - nested shadow blocks (shadow inside shadow inside parent) The "acceptance criteria" suite collects four `test.skip` tests that encode the contract the implementation must meet. They appear as pending in mocha output and serve as the explicit acceptance gate for the redesign work that will follow this spec branch. Many of the active tests fail today (most fail with "shadow child must be attached: expected null to exist", because the default delete cascade disposes shadow uses without preserving them). The failing tests are the red side of the spec; making them green is the redesign session's charter. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/blockly/tests/mocha/index.html | 1 + .../mocha/shadow_variable_fields_test.js | 1165 +++++++++++++++++ 2 files changed, 1166 insertions(+) create mode 100644 packages/blockly/tests/mocha/shadow_variable_fields_test.js diff --git a/packages/blockly/tests/mocha/index.html b/packages/blockly/tests/mocha/index.html index 8dd5417ebe0..efba1afb83e 100644 --- a/packages/blockly/tests/mocha/index.html +++ b/packages/blockly/tests/mocha/index.html @@ -237,6 +237,7 @@ import './registry_test.js'; import './render_management_test.js'; import './serializer_test.js'; + import './shadow_variable_fields_test.js'; import './shortcut_items_test.js'; import './shortcut_registry_test.js'; import './touch_test.js'; diff --git a/packages/blockly/tests/mocha/shadow_variable_fields_test.js b/packages/blockly/tests/mocha/shadow_variable_fields_test.js new file mode 100644 index 00000000000..3fc2b18bb19 --- /dev/null +++ b/packages/blockly/tests/mocha/shadow_variable_fields_test.js @@ -0,0 +1,1165 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {javascriptGenerator} from '../../build/src/generators/javascript.js'; +import {assert} from '../../node_modules/chai/index.js'; +import { + assertEventFired, + assertEventNotFired, + createChangeListenerSpy, +} from './test_helpers/events.js'; +import { + sharedTestSetup, + sharedTestTeardown, +} from './test_helpers/setup_teardown.js'; + +/** + * Tests for the patch that allows FieldVariable on shadow blocks + * (a shadow `variables_get` under any value input). + * + * Quality requirements distilled from the design discussion that drove + * this PR: + * - #1 No new public-API side effects (no extra VarCreate events fired + * automatically, no extra entries in getAllVariables) attributable to + * the patch itself - only application-driven calls should mutate the + * variable map. + * - #2 Shadow / non-shadow `variables_get` must behave identically modulo + * "being a shadow": same lookup, same dropdown, same rename / delete + * cascade, same code generation. + * - #3 Variable delete cascade must include shadow uses (no special + * skipping by isShadow()). + * - #4 Undo of deleteVariable must fully restore both the variable and + * the visible shadow state, with the shadow block keeping its + * original id. + * - #5 Case 2 (parent template references the deleted variable) must + * not crash, throw, or leave the variable map in an inconsistent + * state - this scenario is enabled by the patch and is therefore the + * patch's responsibility. + * - #6 deleteVariable must not throw / refuse for inputs the patch + * enables. + */ +suite('Shadow variable fields', function () { + setup(function () { + sharedTestSetup.call(this); + this.workspace = new Blockly.Workspace(); + + // Define a parent block with a single value input. We use a fresh + // type so that nothing in the existing tests can interfere. + Blockly.defineBlocksWithJsonArray([ + { + 'type': 'shadow_var_parent', + 'message0': 'parent %1', + 'args0': [ + { + 'type': 'input_value', + 'name': 'VALUE', + }, + ], + 'output': null, + }, + // A second wrapper block, used to construct nested-shadow scenarios. + { + 'type': 'shadow_var_wrapper', + 'message0': 'wrap %1', + 'args0': [ + { + 'type': 'input_value', + 'name': 'INNER', + }, + ], + 'output': null, + }, + // A block with two FieldVariable fields, used to construct mixed + // Case 1 / Case 2 shadows. + { + 'type': 'shadow_var_two_field', + 'message0': '%1 %2', + 'args0': [ + {'type': 'field_variable', 'name': 'VAR1', 'variable': 'item'}, + {'type': 'field_variable', 'name': 'VAR2', 'variable': 'item'}, + ], + 'output': null, + }, + ]); + }); + + teardown(function () { + sharedTestTeardown.call(this); + delete Blockly.Blocks['shadow_var_parent']; + delete Blockly.Blocks['shadow_var_wrapper']; + delete Blockly.Blocks['shadow_var_two_field']; + }); + + /** + * Build a parent block whose VALUE input has a shadow `variables_get` + * referencing the given variable id and name. The shadow inherits its + * template from the loaded JSON, mirroring how a toolbox-defined + * shadow ends up after being dragged out. + * + * @param {!Blockly.Workspace} workspace The workspace to load into. + * @param {string} varId The id of the variable the shadow should reference. + * @param {string} varName The visible name of that variable. + * @returns {!Blockly.Block} The newly created parent block. + */ + function makeParentWithShadowVariable(workspace, varId, varName) { + return Blockly.serialization.blocks.append( + { + 'type': 'shadow_var_parent', + 'inputs': { + 'VALUE': { + 'shadow': { + 'type': 'variables_get', + 'fields': { + 'VAR': { + 'id': varId, + 'name': varName, + 'type': '', + }, + }, + }, + }, + }, + }, + workspace, + ); + } + + /** + * Returns the shadow `variables_get` block hanging under the given + * parent's VALUE input. + * + * @param {!Blockly.Block} parent The parent block. + * @returns {!Blockly.Block} The shadow child block. + */ + function getShadowChild(parent) { + const child = parent.getInputTargetBlock('VALUE'); + assert.exists(child, 'shadow child must be attached'); + assert.isTrue(child.isShadow(), 'expected child to be a shadow'); + return child; + } + + // ---------- Setup-only sanity ---------- + + suite('basic shadow attachment', function () { + test('a shadow variables_get can be attached and renders the variable name', function () { + this.workspace.createVariable('item', '', 'item_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const shadow = getShadowChild(parent); + assert.equal(shadow.type, 'variables_get'); + assert.equal(shadow.getField('VAR').getText(), 'item'); + assert.equal(shadow.getField('VAR').getValue(), 'item_id'); + }); + + test('shadow and non-shadow variables_get on the same id share the same VariableModel', function () { + // Quality #2: symmetric behavior - same lookup path. + this.workspace.createVariable('item', '', 'item_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const shadow = getShadowChild(parent); + + const nonShadow = this.workspace.newBlock('variables_get'); + nonShadow.getField('VAR').setValue('item_id'); + + assert.strictEqual( + shadow.getField('VAR').getVariable(), + nonShadow.getField('VAR').getVariable(), + 'both fields should resolve to the same variable model instance', + ); + }); + }); + + // ---------- T8/T9: API side-effect freedom ---------- + + suite('no spurious side effects on attachment', function () { + test('attaching a shadow variables_get does not fire any VarCreate event', function () { + // Quality #1: no API side effect attributable to the patch. + // Pre-register the variable BEFORE installing the listener so the + // initial VarCreate is excluded from the spy. + this.workspace.createVariable('item', '', 'item_id'); + const spy = createChangeListenerSpy(this.workspace); + makeParentWithShadowVariable(this.workspace, 'item_id', 'item'); + assertEventNotFired(spy, Blockly.Events.VarCreate, {}, this.workspace.id); + }); + + test('attaching a shadow variables_get does not change getAllVariables()', function () { + // Quality #1: getAllVariables must reflect only application calls. + this.workspace.createVariable('item', '', 'item_id'); + const before = this.workspace + .getAllVariables() + .map((v) => v.getId()) + .sort(); + makeParentWithShadowVariable(this.workspace, 'item_id', 'item'); + const after = this.workspace + .getAllVariables() + .map((v) => v.getId()) + .sort(); + assert.deepEqual(after, before); + }); + }); + + // ---------- T1 / T2: Case 1 delete + undo ---------- + + suite('Case 1 - shadow value differs from template default', function () { + setup(function () { + // Toolbox-style template references "item". The user then changes the + // shadow's field to point at "foo". The parent's stored shadowState + // is unchanged (still "item"), because edits do not propagate. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + this.parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + this.shadow = getShadowChild(this.parent); + this.originalShadowId = this.shadow.id; + // User picks foo from the dropdown. + this.shadow.getField('VAR').setValue('foo_id'); + }); + + test('deleteVariable resets the shadow field to the template default', function () { + // Quality #3 + #2: cascade processes the shadow. + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + + // Variable map: foo gone. + assert.notExists( + this.workspace.getVariableMap().getVariableById('foo_id'), + ); + // Shadow still attached at the same id; field reset to "item". + const shadow = getShadowChild(this.parent); + assert.equal(shadow.id, this.originalShadowId); + assert.equal(shadow.getField('VAR').getValue(), 'item_id'); + }); + + test('deleteVariable does not throw', function () { + // Quality #6: API must not throw for inputs the patch enables. + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + assert.doesNotThrow(() => + this.workspace.getVariableMap().deleteVariable(foo), + ); + }); + + test('undo restores foo and the shadow display, preserving block id', function () { + // Quality #4: full undo restoration. + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + this.workspace.undo(false); + + const restored = this.workspace + .getVariableMap() + .getVariableById('foo_id'); + assert.exists(restored, 'foo variable must be restored'); + assert.equal(restored.getName(), 'foo'); + + const shadow = getShadowChild(this.parent); + assert.equal( + shadow.id, + this.originalShadowId, + 'shadow block id must not change across delete/undo', + ); + assert.equal( + shadow.getField('VAR').getValue(), + 'foo_id', + 'shadow field must be back to foo', + ); + }); + + test('redo re-applies the deletion', function () { + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + this.workspace.undo(false); + this.workspace.undo(true); // redo + + assert.notExists( + this.workspace.getVariableMap().getVariableById('foo_id'), + 'foo should be gone again after redo', + ); + const shadow = getShadowChild(this.parent); + assert.equal(shadow.getField('VAR').getValue(), 'item_id'); + }); + + test('repeated delete-undo cycles do not accumulate variables', function () { + // Quality #4 (#11 in the test plan): no leak across cycles. + const initialCount = this.workspace.getAllVariables().length; + for (let i = 0; i < 3; i++) { + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + this.workspace.undo(false); + } + assert.equal( + this.workspace.getAllVariables().length, + initialCount, + 'variable count should be stable across delete/undo cycles', + ); + }); + }); + + // ---------- T3 / T4: Case 2 delete (save/load round-trip) ---------- + + suite( + 'Case 2 - parent template references the deleted variable', + function () { + setup(function () { + // Simulate "save with foo selected, then load again". After load, + // serializeShadow on the connection captures the live shadow state, + // so the parent's shadowState now references foo (not the original + // toolbox default). The most direct way to reach this state in a + // headless test is to round-trip through the serializer. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + const original = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + // User picks foo on the original parent, then we save & reload. + getShadowChild(original).getField('VAR').setValue('foo_id'); + + const state = Blockly.serialization.workspaces.save(this.workspace); + this.workspace.clear(); + // Re-create the application-side variables before reloading the + // blocks, the same way an embedding application would. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + Blockly.serialization.workspaces.load(state, this.workspace); + + this.parent = this.workspace + .getAllBlocks(false) + .find((b) => b.type === 'shadow_var_parent'); + this.shadow = getShadowChild(this.parent); + this.originalShadowId = this.shadow.id; + + // Sanity: parent's shadowState now references foo, confirming + // we have actually reached Case 2. + const shadowState = this.parent + .getInput('VALUE') + .connection.getShadowState(); + assert.equal( + shadowState && shadowState.fields && shadowState.fields.VAR.id, + 'foo_id', + 'precondition: parent shadowState must reference foo (Case 2)', + ); + }); + + test('deleteVariable does not throw', function () { + // Quality #6: even Case 2 must not throw. + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + assert.doesNotThrow(() => + this.workspace.getVariableMap().deleteVariable(foo), + ); + }); + + test('foo is regenerated at the same id by the cascade', function () { + // Quality #6: when the parent's stored shadowState references + // the variable being deleted, the cascade replays the template + // through getOrCreateVariablePackage, which re-creates a fresh + // VariableModel at the same id and same name. (The user-visible + // outcome is "the shadow keeps showing the variable name".) + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + const regenerated = this.workspace + .getVariableMap() + .getVariableById('foo_id'); + assert.exists( + regenerated, + 'Case 2 cascade should regenerate foo at the same id', + ); + assert.notStrictEqual( + regenerated, + foo, + 'regenerated VariableModel should be a fresh instance', + ); + assert.equal(regenerated.getName(), 'foo'); + }); + + test('parent block survives the cascade', function () { + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + assert.isFalse( + this.parent.isDeadOrDying(), + 'parent block must not be disposed by the cascade', + ); + }); + + test('undo restores foo and parent / shadow remain alive', function () { + // Quality #4: undo restores the variable. (B' prototype: shadow + // continues to display foo via its cached VariableModel reference; + // the variable map entry is the only data the user can verify.) + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + this.workspace.undo(false); + + assert.exists( + this.workspace.getVariableMap().getVariableById('foo_id'), + 'foo variable must be restored on undo', + ); + assert.isFalse(this.parent.isDeadOrDying()); + const shadow = getShadowChild(this.parent); + assert.equal( + shadow.id, + this.originalShadowId, + 'shadow block id must not change across delete/undo', + ); + }); + + test('repeated delete-undo cycles leave a stable variable map', function () { + // Quality #4 (#11): no accumulation even in Case 2. + const initialNames = this.workspace + .getAllVariables() + .map((v) => v.getName()) + .sort(); + for (let i = 0; i < 3; i++) { + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + this.workspace.undo(false); + } + const finalNames = this.workspace + .getAllVariables() + .map((v) => v.getName()) + .sort(); + assert.deepEqual(finalNames, initialNames); + }); + }, + ); + + // ---------- T5 / T6: shared rename / cascade with non-shadow ---------- + + suite('symmetric behavior with non-shadow variables_get', function () { + setup(function () { + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + this.parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + this.shadow = getShadowChild(this.parent); + this.shadow.getField('VAR').setValue('foo_id'); + + this.nonShadow = this.workspace.newBlock('variables_get'); + this.nonShadow.getField('VAR').setValue('foo_id'); + }); + + test('rename updates both shadow and non-shadow displays', function () { + // Quality #2: renames flow through the same path for both. + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().renameVariable(foo, 'foo2'); + assert.equal(this.shadow.getField('VAR').getText(), 'foo2'); + assert.equal(this.nonShadow.getField('VAR').getText(), 'foo2'); + }); + + test('deleteVariable processes the non-shadow as a use', function () { + // Quality #3: cascade also reaches non-shadow uses. + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + assert.isTrue( + this.nonShadow.isDeadOrDying(), + 'non-shadow use should be disposed by cascade', + ); + }); + + test('VarDelete is fired exactly once even with mixed shadow / non-shadow uses', function () { + // Quality #1: no spurious extra VarCreate / VarDelete from the + // shadow path. + const spy = createChangeListenerSpy(this.workspace); + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().deleteVariable(foo); + + assertEventFired( + spy, + Blockly.Events.VarDelete, + {varId: 'foo_id'}, + this.workspace.id, + ); + assertEventNotFired(spy, Blockly.Events.VarCreate, {}, this.workspace.id); + }); + }); + + // ---------- T12: Save / Load round-trip ---------- + + suite('save / load round-trip', function () { + test('a parent containing a shadow variables_get round-trips through JSON', function () { + // Quality #2: serialization parity with non-shadow. + this.workspace.createVariable('item', '', 'item_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const beforeShadowId = getShadowChild(parent).getField('VAR').getValue(); + + const state = Blockly.serialization.workspaces.save(this.workspace); + this.workspace.clear(); + this.workspace.createVariable('item', '', 'item_id'); + Blockly.serialization.workspaces.load(state, this.workspace); + + const reloadedParent = this.workspace + .getAllBlocks(false) + .find((b) => b.type === 'shadow_var_parent'); + const reloadedShadow = getShadowChild(reloadedParent); + assert.equal( + reloadedShadow.getField('VAR').getValue(), + beforeShadowId, + 'shadow field id must survive save / load', + ); + }); + }); + + // ---------- Observation #5: rename ---------- + + suite('rename propagation', function () { + test('rename updates the shadow field display via standard updateVarName', function () { + // Quality #2: rename flows through the same path for both shadow + // and non-shadow uses. + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'foo_id', + 'foo', + ); + const shadow = getShadowChild(parent); + + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().renameVariable(foo, 'foo_renamed'); + + assert.equal(shadow.getField('VAR').getText(), 'foo_renamed'); + }); + + test('rename undo restores the shadow field display', function () { + // Quality #4: rename undo applies to shadow uses. + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'foo_id', + 'foo', + ); + const shadow = getShadowChild(parent); + + const foo = this.workspace.getVariableMap().getVariableById('foo_id'); + this.workspace.getVariableMap().renameVariable(foo, 'foo_renamed'); + this.workspace.undo(false); + + assert.equal(shadow.getField('VAR').getText(), 'foo'); + }); + }); + + // ---------- Observation #6: load with missing variable ---------- + + suite('loading shadow with unregistered variable', function () { + test('loading auto-creates the variable, like non-shadow variables_get', function () { + // Quality #2: parity with non-shadow load behavior. + // The load path goes through getOrCreateVariablePackage either way, + // so a missing variable is created on demand. + const state = { + 'blocks': { + 'languageVersion': 0, + 'blocks': [ + { + 'type': 'shadow_var_parent', + 'inputs': { + 'VALUE': { + 'shadow': { + 'type': 'variables_get', + 'fields': { + 'VAR': { + 'id': 'auto_id', + 'name': 'auto_name', + 'type': '', + }, + }, + }, + }, + }, + }, + ], + }, + }; + assert.doesNotThrow(() => + Blockly.serialization.workspaces.load(state, this.workspace), + ); + const created = this.workspace + .getVariableMap() + .getVariableById('auto_id'); + assert.exists( + created, + 'load should auto-create the missing variable on demand', + ); + assert.equal(created.getName(), 'auto_name'); + }); + }); + + // ---------- Observation #16: flyout-drop reproduction ---------- + // Reproduction scenario from manual flyout drag testing: dragging a + // parent block with a shadow `variables_get(item)` directly out of + // the flyout produces a parent whose stored shadowState references + // `item` directly (no save/load round-trip needed). Deleting `item` + // must keep the shadow attached at the same parent input. + + suite('flyout-drop scenario', function () { + test('parent dropped from flyout with item shadow keeps the shadow on item delete', function () { + this.workspace.createVariable('item', '', 'item_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const originalShadowId = getShadowChild(parent).id; + + const item = this.workspace.getVariableMap().getVariableById('item_id'); + this.workspace.getVariableMap().deleteVariable(item); + + // Shadow must still be attached at the same id (the cascade + // regenerates a same-id same-name VariableModel for the field). + const shadow = parent.getInputTargetBlock('VALUE'); + assert.exists( + shadow, + 'shadow should not vanish from a freshly-dropped parent after item delete', + ); + assert.equal( + shadow.id, + originalShadowId, + 'shadow block id should be preserved across the cascade', + ); + assert.equal(shadow.getField('VAR').getValue(), 'item_id'); + // The regenerated variable should be a fresh VariableModel that + // the field's getVariable() resolves to. + const regenerated = this.workspace + .getVariableMap() + .getVariableById('item_id'); + assert.exists(regenerated); + assert.strictEqual(shadow.getField('VAR').getVariable(), regenerated); + }); + }); + + // ---------- Observation #7: deletion of unrelated variable ---------- + + suite('deletion of an unrelated variable', function () { + test('deleting a variable the shadow does not currently reference is a no-op for the shadow', function () { + // Quality #3: cascade only touches uses; unrelated shadows survive. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + // The user changes the live shadow value to foo. The parent's + // stored shadowState still references item. + getShadowChild(parent).getField('VAR').setValue('foo_id'); + + // Delete item (the original template default), not foo. + const item = this.workspace.getVariableMap().getVariableById('item_id'); + assert.doesNotThrow(() => + this.workspace.getVariableMap().deleteVariable(item), + ); + + // Shadow's live value is still foo, foo still exists. + const shadow = getShadowChild(parent); + assert.equal(shadow.getField('VAR').getValue(), 'foo_id'); + assert.exists(this.workspace.getVariableMap().getVariableById('foo_id')); + assert.notExists( + this.workspace.getVariableMap().getVariableById('item_id'), + ); + }); + }); + + // ---------- Observation #9: chained delete without undo ---------- + + suite('chained delete without undo', function () { + test('two consecutive deleteVariable calls each succeed and shadow displays settle on template defaults', function () { + // Quality #4 / #6: cascades chain cleanly without intermediate undo. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + this.workspace.createVariable('bar', '', 'bar_id'); + const parent1 = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const parent2 = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + // Both parents now have shadowState referencing item. Switch live + // values to foo / bar respectively (Case 1 setup). + getShadowChild(parent1).getField('VAR').setValue('foo_id'); + getShadowChild(parent2).getField('VAR').setValue('bar_id'); + + const map = this.workspace.getVariableMap(); + map.deleteVariable(map.getVariableById('foo_id')); + map.deleteVariable(map.getVariableById('bar_id')); + + assert.notExists(map.getVariableById('foo_id')); + assert.notExists(map.getVariableById('bar_id')); + assert.equal( + getShadowChild(parent1).getField('VAR').getValue(), + 'item_id', + ); + assert.equal( + getShadowChild(parent2).getField('VAR').getValue(), + 'item_id', + ); + }); + }); + + // ---------- Observation #15: BlockChange event for Case 1 reset ---------- + + suite('BlockChange event recording for Case 1 reset', function () { + test('Case 1 reset fires a BlockChange event the shadow can undo', function () { + // Quality #4: BlockChange events are correctly recorded for shadow + // field changes triggered by the cascade. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const shadow = getShadowChild(parent); + shadow.getField('VAR').setValue('foo_id'); + + const spy = createChangeListenerSpy(this.workspace); + this.workspace + .getVariableMap() + .deleteVariable( + this.workspace.getVariableMap().getVariableById('foo_id'), + ); + + assertEventFired( + spy, + Blockly.Events.BlockChange, + { + element: 'field', + name: 'VAR', + oldValue: 'foo_id', + newValue: 'item_id', + }, + this.workspace.id, + shadow.id, + ); + }); + }); + + // ---------- Observation #4: nested cascade via change listener ---------- + + suite('nested deleteVariable from a change listener', function () { + test('a deleteVariable triggered by a VAR_DELETE listener does not corrupt suppress state', function () { + // Quality #1: previousSuppress save/restore must allow nested cascades. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + this.workspace.createVariable('bar', '', 'bar_id'); + + const parent1 = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + const parent2 = makeParentWithShadowVariable( + this.workspace, + 'item_id', + 'item', + ); + getShadowChild(parent1).getField('VAR').setValue('foo_id'); + getShadowChild(parent2).getField('VAR').setValue('bar_id'); + + // The first VAR_DELETE triggers a nested deleteVariable call. + const map = this.workspace.getVariableMap(); + const listener = (e) => { + if (e.type === Blockly.Events.VAR_DELETE && e.varId === 'foo_id') { + this.workspace.removeChangeListener(listener); + const bar = map.getVariableById('bar_id'); + if (bar) map.deleteVariable(bar); + } + }; + this.workspace.addChangeListener(listener); + + assert.doesNotThrow(() => + map.deleteVariable(map.getVariableById('foo_id')), + ); + assert.notExists(map.getVariableById('foo_id')); + assert.notExists(map.getVariableById('bar_id')); + }); + }); + + // ---------- Observation #12: workspace.clear() with shadow vars ---------- + + suite('workspace.clear() with shadow variables_get', function () { + test('clear() does not throw when shadow variables_get blocks are present', function () { + // Quality #1: dispose paths other than deleteVariable also handle + // shadow variables_get cleanly. + this.workspace.createVariable('foo', '', 'foo_id'); + makeParentWithShadowVariable(this.workspace, 'foo_id', 'foo'); + makeParentWithShadowVariable(this.workspace, 'foo_id', 'foo'); + + assert.doesNotThrow(() => this.workspace.clear()); + assert.equal(this.workspace.getAllBlocks(false).length, 0); + assert.equal(this.workspace.getAllVariables().length, 0); + }); + }); + + // ---------- Observation #14: non-default variable type ---------- + + suite('non-default variable type', function () { + test('shadow variables_get works end-to-end with a non-default variable type', function () { + // Quality #2: type-tagged variables follow the same code path. + this.workspace.createVariable('typed_item', 'TypeA', 'typed_item_id'); + this.workspace.createVariable('typed_foo', 'TypeA', 'typed_foo_id'); + const parent = Blockly.serialization.blocks.append( + { + 'type': 'shadow_var_parent', + 'inputs': { + 'VALUE': { + 'shadow': { + 'type': 'variables_get', + 'fields': { + 'VAR': { + 'id': 'typed_item_id', + 'name': 'typed_item', + 'type': 'TypeA', + }, + }, + }, + }, + }, + }, + this.workspace, + ); + const shadow = getShadowChild(parent); + shadow.getField('VAR').setValue('typed_foo_id'); + + const map = this.workspace.getVariableMap(); + assert.doesNotThrow(() => + map.deleteVariable(map.getVariableById('typed_foo_id')), + ); + assert.equal( + getShadowChild(parent).getField('VAR').getValue(), + 'typed_item_id', + ); + }); + }); + + // ---------- Observation #2: mixed Case 1 / Case 2 in one shadow ---------- + + suite('mixed Case 1 / Case 2 fields in one shadow', function () { + /** + * Build a parent whose VALUE input contains a shadow_var_two_field + * shadow with two variable fields. After save/load this is the only + * way to reach a shadowState whose two fields point at different + * variables. + * + * @param {!Blockly.Workspace} workspace The workspace to load into. + * @param {string} var1Id Variable id for VAR1's template default. + * @param {string} var2Id Variable id for VAR2's template default. + * @returns {!Blockly.Block} The newly created parent block. + */ + function makeParentWithTwoFieldShadow(workspace, var1Id, var2Id) { + return Blockly.serialization.blocks.append( + { + 'type': 'shadow_var_parent', + 'inputs': { + 'VALUE': { + 'shadow': { + 'type': 'shadow_var_two_field', + 'fields': { + 'VAR1': {'id': var1Id, 'name': var1Id, 'type': ''}, + 'VAR2': {'id': var2Id, 'name': var2Id, 'type': ''}, + }, + }, + }, + }, + }, + workspace, + ); + } + + test('Case 1 fields in a mixed shadow are still reset', function () { + // The current B' implementation pushes Case 1 entries to the + // case1Resets list even when the same shadow has a Case 2 field. + // The trailing `if (anyCase2) continue;` only skips loop iterations, + // it does NOT undo the case1Resets push. Pin this behavior. + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + + // Both fields' templates point at foo. After live editing, VAR1 + // moves to item; VAR2 stays on foo. So when foo is deleted: + // VAR1 has live=foo (matches deleted), template=foo (Case 2) + // VAR2 has live=foo (matches deleted), template=foo (Case 2) + // Hmm, that gives Case 2 / Case 2. To get a mix we need the + // templates to differ. Use save/load to lock template = live. + const original = makeParentWithTwoFieldShadow( + this.workspace, + 'foo_id', + 'foo_id', + ); + // Live state matches template both ways at this point. + // Now switch VAR1 to item so live differs from template. + getShadowChild(original).getField('VAR1').setValue('item_id'); + + // Round-trip through serializer. saveConnection captures live state, + // so the reloaded parent's shadowState will have VAR1=item, VAR2=foo. + const state = Blockly.serialization.workspaces.save(this.workspace); + this.workspace.clear(); + this.workspace.createVariable('item', '', 'item_id'); + this.workspace.createVariable('foo', '', 'foo_id'); + Blockly.serialization.workspaces.load(state, this.workspace); + + const reloaded = this.workspace + .getAllBlocks(false) + .find((b) => b.type === 'shadow_var_parent'); + const shadow = getShadowChild(reloaded); + // Restore the live values to the mixed state we want for the test: + // both fields point at foo (the variable we are about to delete), + // but the parent template now has VAR1=item / VAR2=foo. + shadow.getField('VAR1').setValue('foo_id'); + shadow.getField('VAR2').setValue('foo_id'); + + const map = this.workspace.getVariableMap(); + map.deleteVariable(map.getVariableById('foo_id')); + + // VAR1 (Case 1: template = item) should have been reset. + assert.equal( + shadow.getField('VAR1').getValue(), + 'item_id', + 'VAR1 should be reset to template default item', + ); + // VAR2 (Case 2: template = foo) is left untouched. It still + // references foo, which is now an orphan. + assert.equal( + shadow.getField('VAR2').getValue(), + 'foo_id', + 'VAR2 should retain its orphaned reference (Case 2 limitation)', + ); + }); + }); + + // ---------- Observation #10: nested shadow ---------- + + suite('nested shadow blocks', function () { + test('a shadow variables_get nested inside a shadow wrapper does not crash on delete', function () { + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = Blockly.serialization.blocks.append( + { + 'type': 'shadow_var_parent', + 'inputs': { + 'VALUE': { + 'shadow': { + 'type': 'shadow_var_wrapper', + 'inputs': { + 'INNER': { + 'shadow': { + 'type': 'variables_get', + 'fields': { + 'VAR': {'id': 'foo_id', 'name': 'foo', 'type': ''}, + }, + }, + }, + }, + }, + }, + }, + }, + this.workspace, + ); + + const wrapper = parent.getInputTargetBlock('VALUE'); + assert.isTrue(wrapper.isShadow()); + const inner = wrapper.getInputTargetBlock('INNER'); + assert.isTrue(inner.isShadow()); + assert.equal(inner.type, 'variables_get'); + + const map = this.workspace.getVariableMap(); + assert.doesNotThrow(() => + map.deleteVariable(map.getVariableById('foo_id')), + ); + // Parent and wrapper should still exist. The Case 2 cascade + // regenerates foo at the same id (the inner shadow's template + // references foo directly), so the variable map still has foo + // afterwards — the regenerated VariableModel is a fresh instance. + assert.isFalse(parent.isDeadOrDying()); + assert.isFalse(wrapper.isDeadOrDying()); + const regenerated = map.getVariableById('foo_id'); + assert.exists( + regenerated, + 'Case 2 cascade should regenerate foo at the same id', + ); + assert.equal(regenerated.getName(), 'foo'); + }); + }); + + // ---------- DESIRED behavior tests (acceptance criteria) ---------- + // These tests express what a correct implementation should do. + // They are marked `test.skip` so they appear as pending in mocha + // output; lifting the skip is the acceptance criterion for the + // implementation work that follows this spec branch. + + suite('acceptance criteria (currently pending)', function () { + /** + * Build a parent whose stored shadowState references the variable + * we are about to delete (Case 2). Reached by going through a + * save / load round-trip, which is how a real application loads a + * workspace whose user has previously edited the shadow. + * + * @param {!Blockly.Workspace} workspace The workspace to populate. + * @param {string} varId The id of the variable to point at. + * @param {string} varName The visible name of that variable. + * @returns {!Blockly.Block} The reloaded parent block. + */ + function makeCase2Parent(workspace, varId, varName) { + makeParentWithShadowVariable(workspace, varId, varName); + const state = Blockly.serialization.workspaces.save(workspace); + workspace.clear(); + workspace.createVariable(varName, '', varId); + Blockly.serialization.workspaces.load(state, workspace); + return workspace + .getAllBlocks(false) + .find((b) => b.type === 'shadow_var_parent'); + } + + test('Case 2 delete + undo restores the live shadow display', function () { + // After deleteVariable + undo, the shadow's FieldVariable should + // resolve to the SAME VariableModel instance that the workspace + // map currently holds, so subsequent renames / dropdowns / + // generators all see the same identity. + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = makeCase2Parent(this.workspace, 'foo_id', 'foo'); + // Capture the shadow's pre-delete id; after delete + undo we + // re-fetch by walking from the parent because the cascade may + // re-create the underlying JS object (the spec contract is + // "shadow id unchanged across delete/undo", not "JS instance + // unchanged"). See also test 380 which uses the same pattern. + const originalShadowId = getShadowChild(parent).id; + + const map = this.workspace.getVariableMap(); + map.deleteVariable(map.getVariableById('foo_id')); + this.workspace.undo(false); + + const shadow = getShadowChild(parent); + assert.equal( + shadow.id, + originalShadowId, + 'shadow block id must not change across delete/undo', + ); + // The field should resolve to the SAME VariableModel instance + // that the workspace map currently holds, so subsequent renames + // / dropdowns / generators all see the same identity. + const fromField = shadow.getField('VAR').getVariable(); + const fromMap = this.workspace.getVariableMap().getVariableById('foo_id'); + assert.strictEqual( + fromField, + fromMap, + 'shadow field must reference the live workspace VariableModel', + ); + }); + + test('Case 2 deleteVariable produces a consistent variable map / field state', function () { + // After Case 2 deleteVariable (no undo), either: + // (a) the shadow has been respawned and references a fresh + // variable that the cascade also created, OR + // (b) the shadow has been disposed cleanly. + // In either case, no FieldVariable in the workspace should be + // pointing at a VariableModel that the workspace map does not + // know about. + this.workspace.createVariable('foo', '', 'foo_id'); + makeCase2Parent(this.workspace, 'foo_id', 'foo'); + + const map = this.workspace.getVariableMap(); + map.deleteVariable(map.getVariableById('foo_id')); + + const allBlocks = this.workspace.getAllBlocks(false); + for (const block of allBlocks) { + for (const input of block.inputList) { + for (const field of input.fieldRow) { + if (typeof field.getVariable !== 'function') continue; + const v = field.getVariable(); + if (!v) continue; + const onWorkspace = this.workspace + .getVariableMap() + .getVariableById(v.getId()); + assert.strictEqual( + v, + onWorkspace, + `field ${field.name} on block ${block.id} must reference a live VariableModel`, + ); + } + } + } + }); + + test('Case 2 generator output never references an orphaned variable id', function () { + // After Case 2 deleteVariable, the JavaScript generator should + // not emit the deleted variable id as a fallback. Either the + // shadow has been removed from the parent input, or it has been + // re-bound to a live variable. + this.workspace.createVariable('foo', '', 'foo_id'); + makeCase2Parent(this.workspace, 'foo_id', 'foo'); + + const map = this.workspace.getVariableMap(); + map.deleteVariable(map.getVariableById('foo_id')); + + const previousParentHandler = + javascriptGenerator.forBlock['shadow_var_parent']; + javascriptGenerator.forBlock['shadow_var_parent'] = function ( + block, + gen, + ) { + const value = gen.valueToCode(block, 'VALUE', 0) || "''"; + return value + ';\n'; + }; + let code; + try { + code = javascriptGenerator.workspaceToCode(this.workspace); + } finally { + if (previousParentHandler) { + javascriptGenerator.forBlock['shadow_var_parent'] = + previousParentHandler; + } else { + delete javascriptGenerator.forBlock['shadow_var_parent']; + } + } + // Generator output should not contain the orphaned id. + assert.notInclude( + code, + 'foo_id', + 'generator must not emit orphaned variable ids', + ); + }); + + test('rename after Case 2 delete-undo updates the shadow display', function () { + // After Case 2 delete-undo, a rename of the restored variable + // should propagate to the shadow's display through the same + // updateVarName path that non-shadow variables_get follows. + this.workspace.createVariable('foo', '', 'foo_id'); + const parent = makeCase2Parent(this.workspace, 'foo_id', 'foo'); + + const map = this.workspace.getVariableMap(); + map.deleteVariable(map.getVariableById('foo_id')); + this.workspace.undo(false); + + const restored = this.workspace + .getVariableMap() + .getVariableById('foo_id'); + this.workspace.getVariableMap().renameVariable(restored, 'foo_renamed'); + + // Re-fetch the shadow after undo (the cascade may re-create the + // underlying JS object — see test 380 / 966 for the same pattern). + const shadow = getShadowChild(parent); + assert.equal( + shadow.getField('VAR').getText(), + 'foo_renamed', + 'rename of the restored variable should reach the shadow display', + ); + }); + }); +}); From 31e0a8d8c8e9cc8b59853ca1a20031951dba5b60 Mon Sep 17 00:00:00 2001 From: B_head Date: Fri, 10 Apr 2026 01:02:09 +0900 Subject: [PATCH 8/8] feat: cascade deleteVariable through shadow variable_get fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VariableMap.deleteVariable now keeps shadow variable_get blocks attached at the same parent input by replaying each shadow's parent connection shadowState through getOrCreateVariablePackage. The helper either returns an existing live variable (template references a different, still-live variable — Case 1) or re-creates a same-id same-name variable (template references the variable being deleted — Case 2), and setValue rebinds the field to the result. The two cases run on opposite sides of the variable map removal so the resulting event group unwinds cleanly: (a) Case 1 setValue replay (BlockChange events) (b) non-shadow uses dispose, with workspace.suppressShadowRespawn set so Connection.disconnectInternal does not auto-spawn a fresh shadow from a stale shadowState that still references the doomed variable (c) variable map removal + VarDelete fire (d) Case 2 setValue replay (re-creates the variable, fires VarCreate for the fresh VariableModel) Undo unwinds in reverse: - Case 2 VarCreate is undone (the regenerated VariableModel goes away — the surviving field still holds a reference to it but the block id is preserved) - VarDelete is undone (the original variable comes back) - non-shadow dispose events undo (their BlockCreate events restore the disposed blocks) - Case 1 BlockChange events undo (the field validator now finds the restored variable on the workspace, so setValue accepts the old id) The cascade also dedupes uses by block id (a single block with two matching fields would otherwise appear twice in the raw uses list) and falls back to the non-shadow dispose path for shadows that cannot be classified (no reachable parent, no stored shadowState, no matching field). Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/blockly/core/connection.ts | 9 ++ packages/blockly/core/variable_map.ts | 199 +++++++++++++++++++++++++- packages/blockly/core/workspace.ts | 11 ++ 3 files changed, 214 insertions(+), 5 deletions(-) diff --git a/packages/blockly/core/connection.ts b/packages/blockly/core/connection.ts index a79b7b9b143..585d23d6a87 100644 --- a/packages/blockly/core/connection.ts +++ b/packages/blockly/core/connection.ts @@ -345,6 +345,15 @@ export class Connection { */ protected respawnShadow_() { // Have to keep respawnShadow_ for backwards compatibility. + const ws = this.getSourceBlock()?.workspace; + if (ws?.suppressShadowRespawn) { + // VariableMap.deleteVariable has set the suppress flag for the + // duration of its non-shadow dispose loop. Skipping respawn here + // prevents the cascade from re-creating the variable being deleted + // through `getOrCreateVariablePackage` triggered by a stale + // shadowState template. + return; + } this.createShadowBlock(true); } diff --git a/packages/blockly/core/variable_map.ts b/packages/blockly/core/variable_map.ts index 88c28cbc321..a268793dd09 100644 --- a/packages/blockly/core/variable_map.ts +++ b/packages/blockly/core/variable_map.ts @@ -19,15 +19,58 @@ import './events/events_var_rename.js'; import type {Block} from './block.js'; import {EventType} from './events/type.js'; import * as eventUtils from './events/utils.js'; +import {FieldVariable} from './field_variable.js'; import type {IVariableMap} from './interfaces/i_variable_map.js'; import {IVariableModel, IVariableState} from './interfaces/i_variable_model.js'; import {Names} from './names.js'; import * as registry from './registry.js'; +import type {State as BlockState} from './serialization/blocks.js'; import * as deprecation from './utils/deprecation.js'; import * as idGenerator from './utils/idgenerator.js'; -import {deleteVariable, getVariableUsesById} from './variables.js'; +import { + deleteVariable, + getOrCreateVariablePackage, + getVariableUsesById, +} from './variables.js'; import type {Workspace} from './workspace.js'; +/** + * One field on a shadow block whose current value points at the variable + * being deleted. The cascade replays the parent connection's stored + * shadowState through `getOrCreateVariablePackage` for each entry, which + * either returns an existing live variable (the template references a + * different, still-live variable — "Case 1") or re-creates a same-id + * same-name variable (the template references the variable being deleted + * — "Case 2") and binds the field to the result. + * + * The two cases must replay around the variable map removal in opposite + * order so the resulting event group can be undone correctly: + * - Case 1 entries fire BlockChange BEFORE VarDelete, so undo runs the + * VarDelete reverse first (restoring the old variable), then the + * BlockChange reverse (which calls field.setValue with the old id — + * field validation requires the variable to exist on the workspace). + * - Case 2 entries fire AFTER the variable map removal, so + * getOrCreateVariablePackage actually re-creates the variable (and + * fires VarCreate) instead of returning the soon-to-be-deleted one. + */ +interface ShadowFieldPlan { + field: FieldVariable; + /** The matching field entry in the parent connection's shadowState. */ + templateField: AnyDuringMigration; + /** + * True iff `templateField['id']` equals the id of the variable being + * deleted. Determines which side of the variable map removal the + * setValue replay runs on (see ShadowFieldPlan jsdoc). + */ + isSelfTemplate: boolean; +} + +/** Plan for a single shadow block use of the variable being deleted. */ +interface ShadowPlan { + shadow: Block; + entries: ShadowFieldPlan[]; +} + /** * Class for a variable map. This contains a dictionary data structure with * variable types as keys and lists of variables as values. The list of @@ -302,10 +345,60 @@ export class VariableMap /** * Delete a variable and all of its uses without confirmation. * + * Shadow uses are kept alive at the same parent input so the user can + * still pick a different variable. After the variable map removal, the + * cascade replays the parent connection's stored shadowState through + * `getOrCreateVariablePackage` for every matching field. The helper + * either returns an existing live variable (when the template + * references a different, still-live variable) or re-creates the + * variable from the template at the same id and same name (when the + * template references the variable being deleted) — in the latter + * case the cascade fires a `VarCreate` because the variable that gets + * created is a real variable the user can interact with. + * + * Non-shadow uses are disposed normally, but the cascade temporarily + * sets `workspace.suppressShadowRespawn` so that the auto-respawn fired + * by `Connection.disconnectInternal` does not re-create the deleted + * variable through `getOrCreateVariablePackage` triggered by a stale + * shadowState template. + * * @param variable Variable to delete. */ deleteVariable(variable: IVariableModel) { - const uses = getVariableUsesById(this.workspace, variable.getId()); + // Dedupe by block id (a single block with two FieldVariable fields + // pointing at the same variable would otherwise appear twice in the + // raw uses list). + const seen = new Set(); + const uniqueUses: Block[] = []; + for (const use of getVariableUsesById(this.workspace, variable.getId())) { + if (!seen.has(use.id)) { + seen.add(use.id); + uniqueUses.push(use); + } + } + + // Split shadow vs non-shadow uses. + const nonShadowUses: Block[] = []; + const shadowUses: Block[] = []; + for (const use of uniqueUses) { + if (use.isShadow()) shadowUses.push(use); + else nonShadowUses.push(use); + } + + // Plan each shadow's reset by snapshotting the parent connection's + // stored shadowState. An unclassifiable shadow (no reachable parent, + // no stored shadowState, no matching field, no template id) falls + // back to the non-shadow dispose path. + const shadowPlans: ShadowPlan[] = []; + for (const shadow of shadowUses) { + const plan = this.classifyShadow(shadow, variable.getId()); + if (plan) { + shadowPlans.push(plan); + } else { + nonShadowUses.push(shadow); + } + } + let existingGroup = ''; if (!this.potentialMap) { existingGroup = eventUtils.getGroup(); @@ -314,11 +407,47 @@ export class VariableMap } } try { - for (let i = 0; i < uses.length; i++) { - if (uses[i].isDeadOrDying()) continue; + // (a) Replay Case 1 entries — those whose template references a + // DIFFERENT, still-live variable. setValue fires a BlockChange + // in the current event group with oldValue=deletedId, + // newValue=tmplId. Doing this BEFORE the variable map removal + // means the undo replays in the order: + // VarDelete.run(false) // restores the deleted variable + // BlockChange.run(false) // setValue(deletedId), which + // // now passes field validation + // // because the variable is back. + for (const plan of shadowPlans) { + for (const entry of plan.entries) { + if (entry.isSelfTemplate) continue; + const v = getOrCreateVariablePackage( + this.workspace, + entry.templateField['id'] as string, + entry.templateField['name'] as string | undefined, + (entry.templateField['type'] as string) || '', + ); + entry.field.setValue(v.getId()); + } + } - uses[i].dispose(true); + // (b) Dispose non-shadow uses with respawn suppressed so that the + // auto-respawn fired by Connection.disconnectInternal does not + // re-create the deleted variable through a stale shadowState. + const previousSuppress = this.workspace.suppressShadowRespawn; + this.workspace.suppressShadowRespawn = true; + try { + for (const use of nonShadowUses) { + if (use.isDeadOrDying()) continue; + use.dispose(true); + } + } finally { + this.workspace.suppressShadowRespawn = previousSuppress; } + + // (c) Remove the variable from the map and fire VarDelete. Has to + // happen between (a) and (d): (a)'s setValue replay needs the + // variable still in the map to validate the new id is real, + // and (d) needs the variable already removed so that + // getOrCreateVariablePackage actually re-creates it. const variables = this.variableMap.get(variable.getType()); if (!variables || !variables.has(variable.getId())) return; variables.delete(variable.getId()); @@ -328,6 +457,27 @@ export class VariableMap if (variables.size === 0) { this.variableMap.delete(variable.getType()); } + + // (d) Replay Case 2 entries — those whose template references the + // variable being deleted. getOrCreateVariablePackage now + // re-creates the variable at the same id and same name and + // fires a VarCreate (a real variable the user can interact + // with, so the event is appropriate). setValue rebinds the + // field's `variable` reference to the fresh VariableModel via + // doValueUpdate_; the new id equals the old id so no + // BlockChange is fired. + for (const plan of shadowPlans) { + for (const entry of plan.entries) { + if (!entry.isSelfTemplate) continue; + const v = getOrCreateVariablePackage( + this.workspace, + entry.templateField['id'] as string, + entry.templateField['name'] as string | undefined, + (entry.templateField['type'] as string) || '', + ); + entry.field.setValue(v.getId()); + } + } } finally { if (!this.potentialMap) { eventUtils.setGroup(existingGroup); @@ -335,6 +485,45 @@ export class VariableMap } } + /** + * Snapshot a shadow use of the variable being deleted into a plan the + * cascade can replay against `getOrCreateVariablePackage`. + * + * Returns null when the shadow cannot be classified — no reachable + * parent connection, no stored shadowState, no matching `FieldVariable` + * with a name, or no template entry with an id. The caller falls back + * to disposing the shadow as if it were a regular non-shadow use. + */ + private classifyShadow( + shadow: Block, + deletedVariableId: string, + ): ShadowPlan | null { + const parentConn = + shadow.outputConnection?.targetConnection || + shadow.previousConnection?.targetConnection; + if (!parentConn) return null; + + const templateState = parentConn.getShadowState() as BlockState | null; + if (!templateState || !templateState.fields) return null; + const templateFields = templateState.fields as AnyDuringMigration; + + const entries: ShadowFieldPlan[] = []; + for (const input of shadow.inputList) { + for (const field of input.fieldRow) { + if (!(field instanceof FieldVariable)) continue; + if (field.getVariable()?.getId() !== deletedVariableId) continue; + const fieldName = field.name; + if (!fieldName) continue; + const tmplField = templateFields[fieldName]; + if (!tmplField || !tmplField['id']) continue; + const isSelfTemplate = tmplField['id'] === deletedVariableId; + entries.push({field, templateField: tmplField, isSelfTemplate}); + } + } + if (!entries.length) return null; + return {shadow, entries}; + } + /** * Delete a variables by the passed in ID and all of its uses from this * workspace. May prompt the user for confirmation. diff --git a/packages/blockly/core/workspace.ts b/packages/blockly/core/workspace.ts index 88745f4205c..dc409d742aa 100644 --- a/packages/blockly/core/workspace.ts +++ b/packages/blockly/core/workspace.ts @@ -95,6 +95,17 @@ export class Workspace { */ isClearing = false; + /** + * If true, `Connection.respawnShadow_` becomes a no-op. Set scoped to a + * try/finally inside `VariableMap.deleteVariable` so that the cascade's + * non-shadow dispose loop does not auto-spawn a stale shadow whose + * stored state may reference the variable being deleted (which would + * silently re-create the variable through `getOrCreateVariablePackage`). + * + * @internal + */ + suppressShadowRespawn = false; + /** * Maximum number of undo events in stack. `0` turns off undo, `Infinity` * sets it to unlimited.