Skip to content

Fix block handler registry cleanup during workspace load #571

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

Closed
tracygardner wants to merge 1 commit into from
Closed

Fix block handler registry cleanup during workspace load #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion main/blockhandling.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,12 +527,18 @@ export function initializeBlockHandling() {
}

// Purge deleted blocks from the registry, then dispatch to handlers.
// Guard: only remove if no live block with that ID exists. When
// workspaces.load() queues DELETE events during its internal clear()
// and flushes them after creating new blocks with the same IDs, the
// new handlers must not be evicted.
if (
event.type === Blockly.Events.BLOCK_DELETE &&
Array.isArray(event.ids)
) {
for (const id of event.ids) {
blockHandlerRegistry.delete(id);
if (!workspace.getBlockById(id)) {
blockHandlerRegistry.delete(id);
}
}
}
Comment on lines 529 to 543

@coderabbitai coderabbitai Bot Apr 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm Blockly's BLOCK_DELETE fires after the block is removed from the workspace,
# so workspace.getBlockById(id) returns null at the time this listener runs.
rg -nP --type=js -C3 '\bBLOCK_DELETE\b|fireDeleteEvents|disposeInternal' -g '!**/node_modules/**'

Repository: flipcomputing/flock

Length of output: 50376

🌐 Web query:

In Blockly, when a BLOCK_DELETE event is dispatched to change listeners, has the block already been removed from workspace.getBlockById?

💡 Result:

No, when a BLOCK_DELETE event is dispatched to change listeners, the block has not yet been removed from the workspace and can still be accessed via workspace.getBlockById(event.blockId).

Citations:

Guard correctly prevents handler eviction for reused IDs.

The guard logic is sound and the inline comment is genuinely noteworthy. However, note that workspace.getBlockById(id) returns the block while BLOCK_DELETE is being dispatched—the block hasn't yet been removed from the workspace at that point. This means the guard condition !workspace.getBlockById(id) will be false during normal user-initiated deletes, and handlers won't be purged in those cases. Handlers for deleted blocks will only be cleared if a subsequent event fires after the block is actually removed from the workspace, or if cleanup happens elsewhere.

This appears to be an intentional trade-off: the guard protects against the race condition described (where new blocks are created with recycled IDs during workspaces.load()), at the cost of potentially accumulating stale handlers. Confirm that handlers are cleaned up elsewhere or that their presence is harmless (e.g., handlers already check workspace.getBlockById() internally).

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@main/blockhandling.js` around lines 529 - 543, The current purge runs during
BLOCK_DELETE handling while workspace.getBlockById(id) still returns the block,
so handlers for actually-deleted blocks aren't removed; to fix, defer the
deletion check to after the workspace mutation (e.g., wrap the loop in a
microtask or setTimeout 0) and then for each id verify
workspace.getBlockById(id) is falsy before calling
blockHandlerRegistry.delete(id); keep references to the same symbols: event.type
=== Blockly.Events.BLOCK_DELETE, event.ids, workspace.getBlockById, and
blockHandlerRegistry.delete to locate and update the code.


Expand Down
10 changes: 1 addition & 9 deletions main/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { workspace } from "./blocklyinit.js";
import { translate } from "./translation.js";
import { getMetadata } from "meta-png";
import { AUTOSAVE_KEY } from "../config.js";
import { blockHandlerRegistry } from "../blocks/blocks.js";

// Function to save the current workspace state
export function saveWorkspace(workspace) {
Expand Down Expand Up @@ -319,14 +318,7 @@ export function loadWorkspaceAndExecute(json, workspace, executeCallback) {
// Validate JSON before loading into workspace
const validatedJson = validateBlocklyJson(json);

// Clear workspace and handlers
const eventsWereEnabled = Blockly.Events.isEnabled();
if (eventsWereEnabled) Blockly.Events.disable();
workspace.clear();
blockHandlerRegistry.clear();
if (eventsWereEnabled) Blockly.Events.enable();

// Load the validated JSON
// Load the validated JSON
Blockly.serialization.workspaces.load(validatedJson, workspace);

workspace.scroll(0, 0);
Expand Down
Loading