Skip to content

Fix if_clause block validation during undo/redo and field changes #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tracygardner merged 5 commits into from
Mar 21, 2026
Merged

Fix if_clause block validation during undo/redo and field changes #447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60df353
fix: heal block stack on delete for regular and if_clause blocks
claude Mar 21, 2026
b9b9c9c
fix: revert _isHealingStack bypass that broke if_clause undo
claude Mar 21, 2026
d58e40d
fix: disable invalid if_clause blocks in-place instead of popping them
claude Mar 21, 2026
42b9887
fix: add idempotence guard, BLOCK_CHANGE/MODE listener, and else disc…
claude Mar 21, 2026
de13308
fix: disable else in-place instead of disconnecting on MODE field change
claude Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 1 addition & 65 deletions blocks/control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,8 +573,7 @@ export function defineControlBlocks() {
}
}

// Validate and disconnect invalid subsequent blocks
this.validateAndDisconnectInvalidChain_(mode);
// Recompute validity (disables invalid blocks in-place; does not disconnect).
this.recomputeIfClauseValidity_();

if (this.rendered) this.render();
Expand Down Expand Up @@ -678,69 +677,6 @@ export function defineControlBlocks() {
}
}
},

validateAndDisconnectInvalidChain_: function (currentMode) {
// Skip validation if we're not in a workspace or during restore
if (!this.workspace || this._isRestoring) {
return;
}

// Walk through the chain and find the first invalid block
let current = this;
let chainModes = [currentMode];

while (current) {
const nextConn = current.nextConnection;
if (!nextConn) break;

const nextBlock = nextConn.targetBlock();

if (!nextBlock || nextBlock.type !== "if_clause") {
// End of if_clause chain, all valid
break;
}

const nextMode = nextBlock.getFieldValue?.("MODE");
if (!nextMode) break;

const lastMode = chainModes[chainModes.length - 1];

// Check if this next block is valid given what came before
const isInvalid = this.isInvalidInChain_(lastMode, nextMode);

if (isInvalid) {
// The nextBlock is invalid after lastMode
// Disconnect from the current block synchronously
if (nextBlock && !nextBlock.isDisposed() && nextConn.isConnected()) {
// Disconnect from the parent side
nextConn.disconnect();

// Bump the disconnected chain to a new location
if (nextBlock.rendered) {
nextBlock.bumpNeighbours();
}
}
break; // Stop checking since we're disconnecting from here
}

chainModes.push(nextMode);
current = nextBlock;
}
},

isInvalidInChain_: function (previousMode, nextMode) {
// ELSE cannot have anything after it
if (previousMode === MODE.ELSE) {
return true;
}

// IF and ELSEIF can be followed by:
// - ELSEIF (valid)
// - ELSE (valid)
// - IF (valid - starts a new chain)
// So nothing is invalid after IF or ELSEIF
return false;
},
};
}

Expand Down
114 changes: 107 additions & 7 deletions main/blocklyinit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,30 +225,37 @@ function initializeIfClauseConnectionChecker(workspace) {
if (targetIsIfClause) {
const targetMode = targetBlock.getFieldValue("MODE");

// Rule 1: Nothing can connect after ELSE
// Rule 1: Nothing can connect after ELSE.
// During drag-and-drop reject to give visual feedback.
// During healing / field-changes (not dragging) allow the connection;
// validateIfClausePositions will disable the block in-place.
if (targetMode === MODE.ELSE) {
return false;
if (workspace.isDragging()) return false;
}

// Rule 2: ELSE cannot connect if it has if_clause blocks after it
if (movingMode === MODE.ELSE && movingHasIfClauseBelow) {
return false;
}

// Rule 3: ELSE cannot be inserted in middle of chain
// Rule 3: ELSE cannot be inserted in middle of chain (drag only).
// When not dragging (e.g. a MODE field change), keep the connection
// and let validateIfClausePositions disable the block in-place.
const targetHasNext = realNext(targetBlock);
if (
targetHasNext &&
targetHasNext.type === "if_clause" &&
movingMode === MODE.ELSE
) {
return false;
if (workspace.isDragging()) return false;
}
} else {
// Target is NOT if_clause
// ELSEIF and ELSE cannot connect after non-if_clause blocks
// Target is NOT if_clause.
// During drag-and-drop reject to give visual feedback.
// During healing / undo-redo (not dragging) allow the connection;
// validateIfClausePositions will disable the block in-place.
if (movingMode === MODE.ELSEIF || movingMode === MODE.ELSE) {
return false;
if (workspace.isDragging()) return false;
}
}
} else {
Expand Down Expand Up @@ -310,6 +317,68 @@ function initializeIfClauseConnectionChecker(workspace) {

return true;
};

// Disable reason used to mark if_clause blocks that are structurally
// connected but in an invalid position (e.g. ELSEIF after a regular block).
const INVALID_IF_CLAUSE_REASON = "INVALID_IF_CLAUSE_POSITION";

// Scan all if_clause blocks and disable/enable them based on whether their
// predecessor is a valid if_clause. Runs with events disabled so the
// enable/disable state is derived (not recorded in the undo stack).
function validateIfClausePositions() {
Blockly.Events.disable();
try {
for (const block of workspace.getAllBlocks(false)) {
if (block.type !== "if_clause") continue;

const mode = block.getFieldValue("MODE");

// IF blocks can start a chain anywhere — always positionally valid.
if (mode === MODE.IF) {
block.setDisabledReason(false, INVALID_IF_CLAUSE_REASON);
continue;
}

// ELSEIF / ELSE: valid only when the immediately preceding connected
// block is an if_clause whose mode is IF or ELSEIF (not ELSE).
const prevBlock = realPrev(block);

if (!prevBlock) {
// Orphaned — disableOrphans handles the disabled state; clear ours.
block.setDisabledReason(false, INVALID_IF_CLAUSE_REASON);
continue;
}

const validPrev =
prevBlock.type === "if_clause" &&
prevBlock.getFieldValue("MODE") !== MODE.ELSE;

block.setDisabledReason(!validPrev, INVALID_IF_CLAUSE_REASON);
}
} finally {
Blockly.Events.enable();
}
}

// Re-validate after any structural change so that if_clause blocks that
// land in an invalid position are disabled immediately, and those that
// become valid again are re-enabled.
workspace.addChangeListener(function (event) {
if (
!event.isUiEvent &&
(event.type === Blockly.Events.BLOCK_MOVE ||
event.type === Blockly.Events.BLOCK_CREATE ||
event.type === Blockly.Events.BLOCK_DELETE ||
(event.type === Blockly.Events.BLOCK_CHANGE &&
event.element === "field" &&
event.name === "MODE"))
) {
validateIfClausePositions();
}
});

// Run once on initialisation to catch any blocks already in invalid positions.
validateIfClausePositions();
}

export function initializeWorkspace() {
Expand Down Expand Up @@ -531,6 +600,37 @@ export function initializeWorkspace() {
originalScrollBoundsIntoView.call(this, bounds);
};

// Patch BlockDelete.run so that redo (forward=true) heals the stack just
// like the original dispose call did. Without this, BlockDelete.run(true)
// calls dispose(false) which skips healing and can leave the block below the
// deleted block orphaned when the BLOCK_MOVE heal event is later replayed
// through canConnect and our custom checker rejects the reconnection.
const BlockDeleteClass = Blockly.Events.BlockDelete;
if (BlockDeleteClass && !BlockDeleteClass._healRedoPatched) {
BlockDeleteClass._healRedoPatched = true;
const originalBlockDeleteRun = BlockDeleteClass.prototype.run;
BlockDeleteClass.prototype.run = function (forward) {
if (forward) {
// Redo: delete the block(s) WITH stack healing so that the block
// below the deleted block is reconnected rather than orphaned.
const workspace = this.getEventWorkspace_();
if (this.ids) {
for (let i = 0; i < this.ids.length; i++) {
const block = workspace.getBlockById(this.ids[i]);
if (block) {
block.dispose(true); // heal=true: reconnect next block to parent
} else if (this.ids[i] === this.blockId) {
console.warn("Can't delete non-existent block: " + this.ids[i]);
}
}
}
} else {
// Undo: restore block from saved JSON (standard Blockly behaviour).
originalBlockDeleteRun.call(this, forward);
}
};
}

// Initialize workspace search
const workspaceSearch = new WorkspaceSearch(workspace);
workspaceSearch.init();
Expand Down
Loading