Port sp_javascriptV2 to PenguinMod's ACE editor approach; fix field default-value double-escaping#55
Merged
supervoidcoder merged 2 commits intodevelopfrom Mar 6, 2026
Conversation
…double-escaping Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix issues in JavaScript extension for sandboxing
Port sp_javascriptV2 to PenguinMod's ACE editor approach; fix field default-value double-escaping
Mar 6, 2026
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbitRelease Notes
WalkthroughThis PR modifies XML escaping semantics for block argument default values in the runtime engine and significantly overhauls the JavaScript V2 extension's code editor integration with ACE-based dynamic loading, autocompletion schema management, and sandbox state persistence. Changes
Sequence DiagramsequenceDiagram
participant User as User/Blockly
participant Ext as SPjavascriptV2
participant ACE as ACE Editor Loader
participant Schema as Schema Manager
participant Compiler as Code Compiler
participant Sandbox as SandboxRunner
User->>Ext: Initialize extension with block
Ext->>ACE: Load ACE packages (async)
ACE-->>Ext: ACE loaded & ready
Ext->>Ext: Create per-block editor instance
Ext->>Schema: Build autocompletion from globalFuncs
Schema-->>Ext: Schema updated
Ext->>Ext: Initialize editor with defaultValue (SECRET_BLOCK_KEY)
User->>Ext: Enter/modify code in editor
Ext->>Ext: Update field value via ACE session
User->>Ext: Define new global function
Ext->>Ext: Validate name (_isLegalFuncName)
Ext->>Ext: Store in globalFuncs
Ext->>Schema: Refresh schema with updated globalFuncs
Schema-->>Ext: Autocompletion list updated
User->>Ext: Execute code block
Ext->>Compiler: Compile code with injected globals (_compileCode)
Compiler->>Compiler: Parse arguments (_parseArguments)
Compiler->>Sandbox: Execute (sandboxed or unsandboxed)
Sandbox-->>Ext: Result
Ext-->>User: Return output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/extensions/sp_javascriptV2/index.js (2)
242-260:⚠️ Potential issue | 🟠 MajorRestore pointer events after resize ends.
Line 242 sets
input.style.pointerEvents = "none", butonMouseUp()never turns it back on. After the first resize, the editor can stay unclickable until some unrelated class mutation flips the style again.💡 Minimal fix
function onMouseUp() { isResizing = false; + input.style.pointerEvents = "all"; ScratchBlocks.mainWorkspace.allowDragging = true; parent.setMovable(true); document.removeEventListener("mousemove", onMouseMove); document.removeEventListener("mouseup", onMouseUp); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extensions/sp_javascriptV2/index.js` around lines 242 - 260, The resize handler disables pointer events on the editor input (input.style.pointerEvents = "none") but onMouseUp never restores it, leaving the editor unclickable after a resize; update the onMouseUp() function (the same scope that removes the mousemove/mouseup listeners and toggles isResizing/ScratchBlocks.mainWorkspace/parent.setMovable) to reset the input pointer events (e.g., set input.style.pointerEvents back to "" or "auto"), guarding that input is defined before touching its style, and keep this restoration alongside the other teardown steps so the editor becomes interactive again after resizing.
494-505:⚠️ Potential issue | 🟠 MajorMake deserialization assign the target state instead of toggling it.
deserialize()callstoggleSandbox(), so a second deserialize—or loading into an instance that is already unsandboxed—flips the flag the wrong way. Restore the desired state explicitly and refresh blocks once the permission check passes.💡 Safer restore logic
deserialize(data) { - if (data.isUnsandboxed) this.toggleSandbox(); + const shouldBeUnsandboxed = Boolean(data && data.isUnsandboxed); + if (shouldBeUnsandboxed === this.isEditorUnsandboxed) return; + + if (!shouldBeUnsandboxed) { + this.isEditorUnsandboxed = false; + this.runtime.extensionManager.refreshBlocks("SPjavascriptV2"); + return; + } + + this.runtime.vm.securityManager.canUnsandbox("JavaScript").then(isAllowed => { + if (!isAllowed) return; + this.isEditorUnsandboxed = true; + this.runtime.extensionManager.refreshBlocks("SPjavascriptV2"); + }); }Also applies to: 747-749
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extensions/sp_javascriptV2/index.js` around lines 494 - 505, deserialize() should not call toggleSandbox() because that flips isEditorUnsandboxed; instead set the desired state explicitly: call runtime.vm.securityManager.canUnsandbox("JavaScript").then(isAllowed => { if (!isAllowed) return; this.isEditorUnsandboxed = <desiredBoolean>; this.runtime.extensionManager.refreshBlocks("SPjavascriptV2"); }); Replace any deserialize usages that call toggleSandbox() (and the similar code area around the other occurrence) so they assign this.isEditorUnsandboxed directly and refresh blocks only after the permission check passes, referencing toggleSandbox(), deserialize(), isEditorUnsandboxed, runtime.vm.securityManager.canUnsandbox, and runtime.extensionManager.refreshBlocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/extensions/sp_javascriptV2/index.js`:
- Around line 138-145: The DOM cleanup is missing: open(), modify(), and close()
create new recyclableDiv nodes and listeners without removing previous
instances; update these functions (open, modify, close) to first find and remove
any existing recyclableDiv appended to the parent (or keep a single
module-scoped recyclableDiv and reuse it), detach any event listeners attached
to unloadedEditor/svgGroup_ (undo svgGroup_.setAttribute closures or remove
handlers) and then append or reinitialize the element; reference the
recyclableDiv, unloadedEditor, parent, open(), modify(), close(), and svgGroup_
symbols when making the change so no stale nodes or listeners remain across
mount cycles.
- Around line 16-29: The ACE loading logic (ACE_URL, ACE_PACKAGES,
loadedPackages, importAcePackages and waitForAce()) currently only listens for
onload and can hang indefinitely if assets fail; update importAcePackages to
attach onerror handlers to each created <script> that increments a failure
counter and rejects/marks that package as failed, add a global timeout in
waitForAce() (e.g., Promise.race with a timer) that resolves/rejects after a
configurable delay, and modify the caller that initializes the editor to detect
load failure and instantiate a plain-text fallback editor (simple <textarea> or
contentEditable element) when ACE fails to load or the timeout fires so the UI
never stays blocked.
---
Outside diff comments:
In `@src/extensions/sp_javascriptV2/index.js`:
- Around line 242-260: The resize handler disables pointer events on the editor
input (input.style.pointerEvents = "none") but onMouseUp never restores it,
leaving the editor unclickable after a resize; update the onMouseUp() function
(the same scope that removes the mousemove/mouseup listeners and toggles
isResizing/ScratchBlocks.mainWorkspace/parent.setMovable) to reset the input
pointer events (e.g., set input.style.pointerEvents back to "" or "auto"),
guarding that input is defined before touching its style, and keep this
restoration alongside the other teardown steps so the editor becomes interactive
again after resizing.
- Around line 494-505: deserialize() should not call toggleSandbox() because
that flips isEditorUnsandboxed; instead set the desired state explicitly: call
runtime.vm.securityManager.canUnsandbox("JavaScript").then(isAllowed => { if
(!isAllowed) return; this.isEditorUnsandboxed = <desiredBoolean>;
this.runtime.extensionManager.refreshBlocks("SPjavascriptV2"); }); Replace any
deserialize usages that call toggleSandbox() (and the similar code area around
the other occurrence) so they assign this.isEditorUnsandboxed directly and
refresh blocks only after the permission check passes, referencing
toggleSandbox(), deserialize(), isEditorUnsandboxed,
runtime.vm.securityManager.canUnsandbox, and
runtime.extensionManager.refreshBlocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9f1650e-f1ee-4658-a858-9ae2f3c68c57
📒 Files selected for processing (2)
src/engine/runtime.jssrc/extensions/sp_javascriptV2/index.js
Comment on lines
+16
to
+29
| const ACE_URL = "https://cdn.jsdelivr.net/npm/ace-builds@1.32.3/src-min-noconflict/"; | ||
| const ACE_PACKAGES = [ | ||
| "ace.js", "ext-language_tools.js", | ||
| "mode-javascript.js", "theme-monokai.js" | ||
| ]; | ||
|
|
||
| let loadedPackages = 0; | ||
| const importAcePackages = () => { | ||
| for (const packageName of ACE_PACKAGES) { | ||
| const script = document.createElement("script"); | ||
| script.src = ACE_URL + packageName; | ||
| script.async = false; | ||
| script.onload = () => loadedPackages++; | ||
| document.body.appendChild(script); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/extensions/sp_javascriptV2/index.jsRepository: OmniBlocks/scratch-vm
Length of output: 1876
🏁 Script executed:
cat -n src/extensions/sp_javascriptV2/index.js | sed -n '16,42p'Repository: OmniBlocks/scratch-vm
Length of output: 1089
🏁 Script executed:
cat -n src/extensions/sp_javascriptV2/index.js | sed -n '160,175p'Repository: OmniBlocks/scratch-vm
Length of output: 659
🏁 Script executed:
# Find the waitForAce function definition
rg -n "waitForAce" src/extensions/sp_javascriptV2/index.js -A 10 -B 2Repository: OmniBlocks/scratch-vm
Length of output: 784
Add error and timeout handling to prevent permanent editor hang when ACE assets fail to load.
waitForAce() only resolves when all onload callbacks fire. If external ACE assets are blocked by CSP, DNS failure, offline mode, or CDN unavailability, the Promise never resolves and the editor at line 166 remains permanently blocked. Add onerror handlers to the script elements, implement a timeout mechanism, and provide a plain-text fallback editor when ACE fails to load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/extensions/sp_javascriptV2/index.js` around lines 16 - 29, The ACE
loading logic (ACE_URL, ACE_PACKAGES, loadedPackages, importAcePackages and
waitForAce()) currently only listens for onload and can hang indefinitely if
assets fail; update importAcePackages to attach onerror handlers to each created
<script> that increments a failure counter and rejects/marks that package as
failed, add a global timeout in waitForAce() (e.g., Promise.race with a timer)
that resolves/rejects after a configurable delay, and modify the caller that
initializes the editor to detect load failure and instantiate a plain-text
fallback editor (simple <textarea> or contentEditable element) when ACE fails to
load or the timeout fires so the UI never stays blocked.
Comment on lines
+138
to
145
| // element reused by the custom input api | ||
| const recyclableDiv = document.createElement("div"); | ||
| recyclableDiv.setAttribute("style", `display: flex; justify-content: center; padding-top: 10px; width: 250px; height: 200px;`); | ||
| recyclableDiv.setAttribute("style", `display: flex; justify-content: center; padding-top: 10px; width: 250px; height: 100px;`); | ||
|
|
||
| const unloadedEditor = document.createElement("div"); | ||
| unloadedEditor.setAttribute("style", "background: #272822; border-radius: 10px; border: none; width: 100%; height: calc(100% - 20px);"); | ||
| recyclableDiv.appendChild(unloadedEditor); | ||
|
|
There was a problem hiding this comment.
Teardown cleanup is missing between mount cycles — elements and listeners accumulate in the parent container.
The open(), modify(), and close() functions all create fresh recyclableDiv elements and append them to the same parent container without clearing previous instances. This causes DOM nodes, event listeners (line 160, 220), and closures from the wrapped svgGroup_.setAttribute to persist across invocations. After multiple open/close cycles, multiple listeners will fire simultaneously and operate on stale block references.
Each invocation should remove the old recyclableDiv before appending the new one, or use a single reusable instance with proper state reset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/extensions/sp_javascriptV2/index.js` around lines 138 - 145, The DOM
cleanup is missing: open(), modify(), and close() create new recyclableDiv nodes
and listeners without removing previous instances; update these functions (open,
modify, close) to first find and remove any existing recyclableDiv appended to
the parent (or keep a single module-scoped recyclableDiv and reuse it), detach
any event listeners attached to unloadedEditor/svgGroup_ (undo
svgGroup_.setAttribute closures or remove handlers) and then append or
reinitialize the element; reference the recyclableDiv, unloadedEditor, parent,
open(), modify(), close(), and svgGroup_ symbols when making the change so no
stale nodes or listeners remain across mount cycles.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The JS extension's code editor block crashed with
Cannot read properties of null (reading 'isInFlyout')and never rendered the ACE editor. A second bug caused the ARGS input to display{ "FOO": "bar" }literally instead of{ "FOO": "bar" }.src/extensions/sp_javascriptV2/index.js— ported to PenguinMod's implementationRoot cause of null crash:
jsCommandBinded(andjsReporter/jsBooleanBinded) embedded the custom editor field directly on the outer block viatype: ArgumentType.CUSTOM. In the flyout those blocks have no parent, sosrcBlock.parentBlock_was alwaysnull.Fix: Switch to
fillIn: "codeInput"(matching PenguinMod), so the editor field lives inside acodeInputshadow whoseparentBlock_is always the outer block:Root cause of blank editor box: The old code injected ACE inside a sandboxed
<iframe>communicating overpostMessage. The iframe never rendered correctly in this context.Fix: Replaced with PenguinMod's approach — ACE scripts are injected once into the page and the editor is mounted directly into the block's foreignObject
<div>. Also brings in:updateEditorSchema— populates ACE autocomplete with global function names_compileCode/_parseArguments— cleaner argument injection (array vs. object dispatch)serialize/deserialize— persists sandbox toggle state across savessrc/engine/runtime.js— fix double-escaped field default valuesdefaultValuewas passed throughxmlEscape.escapeAttributetwice — once at computation, once at XML interpolation — causing"→"→&quot;in the emitted XML. Blockly then displayed the raw entity text.Changed computation to plain
xmlEscape(correct for element content, noJSON.stringifyside-effects) and removed the redundant second escape at the insertion point:Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
cdn.assets.scratch.mit.edu/opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node ./test/integration/complex.js(dns block)/opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node ./test/integration/import_nested_sb2.js(dns block)/opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node ./test/integration/import_sb2.js(dns block)turbowarp.org/opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node ./test/unit/tw_extension_manager.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.