From 473e448a3730e505c87df6aa6cf1ad8d2e5d65b3 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Wed, 29 Jul 2026 12:45:08 +0100 Subject: [PATCH 1/6] cater for non-standard exception and make handleError defensive --- .../SkulptRunner/SkulptRunner.jsx | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx index 647232b7c..f4ff4da45 100644 --- a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx +++ b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx @@ -412,19 +412,29 @@ const SkulptRunner = ({ message: errorMessage, }; } else { - const errorDescription = (err.tp$str && err.tp$str().v) + // err is normally a Skulpt exception (tp$str/traceback), but native + // code called from Python (e.g. the p5/py5 shims) can throw plain JS + // errors instead, which don't have that shape - fall back gracefully + // rather than letting this handler itself throw. + const rawDescription = + typeof err.tp$str === "function" ? err.tp$str().v : undefined; + const errorDescription = ( + rawDescription || + err.message || + t("editor.errors.generalError") + ) .replace(/\[(.*?)\]/, "") .replace(/\.$/, ""); - const errorType = err.tp$name || err.constructor.name; - let lineNumber = err.traceback[0].lineno; - let fileName = err.traceback[0].filename; + const errorType = err.tp$name || err.constructor?.name; + let lineNumber = err.traceback?.[0]?.lineno; + let fileName = err.traceback?.[0]?.filename; // If this is an error in the sense_hat.py shim, use the next entry in // the traceback as this will be the line in the shim which we don't want // to show to users, so that the error message will instead point to the // line in the user's code which caused the error. if ( - err.traceback.length > 1 && + err.traceback?.length > 1 && fileName === "./sense_hat.py" && ["RuntimeError", "ValueError"].includes(errorType) ) { @@ -432,7 +442,7 @@ const SkulptRunner = ({ fileName = err.traceback[1].filename; } - fileName = fileName.replace(/^\.\//, ""); + fileName = fileName ? fileName.replace(/^\.\//, "") : fileName; if (errorType === "ImportError" && window.crossOriginIsolated) { const articleLink = `https://help.editor.raspberrypi.org/hc/en-us/articles/30841379339924-What-Python-libraries-are-available-in-the-Code-Editor`; From 9de752ffd88396ba63a4034e2cb8613d957df54f Mon Sep 17 00:00:00 2001 From: cocomarine Date: Wed, 29 Jul 2026 14:39:44 +0100 Subject: [PATCH 2/6] add test --- .../SkulptRunner/SkulptRunner.test.js | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js index cd60876eb..656ae83e8 100644 --- a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js +++ b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js @@ -396,6 +396,77 @@ describe("When an error originates in the sense_hat shim", () => { }); }); +describe("When native code called from Python throws a plain JS error (e.g. the p5/py5 shims), rather than a Skulpt exception", () => { + let store; + let asyncToPromiseSpy; + + beforeEach(() => { + asyncToPromiseSpy = jest + .spyOn(Sk.misceval, "asyncToPromise") + .mockReturnValue( + Promise.reject( + new TypeError("Cannot read properties of undefined (reading 'foo')"), + ), + ); + + const middlewares = []; + const mockStore = configureMockStore(middlewares); + const initialState = { + editor: { + project: { + components: [ + { + name: "main", + extension: "py", + content: "import py5", + }, + ], + image_list: [], + }, + codeRunTriggered: true, + }, + auth: { + user, + }, + }; + store = mockStore(initialState); + render( + + + , + ); + }); + + afterEach(() => { + asyncToPromiseSpy.mockRestore(); + }); + + test("falls back to the native error message instead of throwing", () => { + expect(store.getActions()).toEqual( + expect.arrayContaining([ + setError( + "TypeError: Cannot read properties of undefined (reading 'foo') on line undefined of undefined", + ), + ]), + ); + }); + + test("sets errorDetails using the native error type and message", () => { + expect(store.getActions()).toEqual( + expect.arrayContaining([ + setErrorDetails({ + type: "TypeError", + line: undefined, + file: undefined, + description: "Cannot read properties of undefined (reading 'foo')", + message: + "TypeError: Cannot read properties of undefined (reading 'foo') on line undefined of undefined", + }), + ]), + ); + }); +}); + describe("When an error has occurred", () => { let mockStore; let store; From 8c9426e73f857f22c90c7a1dab25c7ef8cb2ba5e Mon Sep 17 00:00:00 2001 From: cocomarine Date: Fri, 31 Jul 2026 13:58:28 +0100 Subject: [PATCH 3/6] Fix bugs detected by Cursor and update test --- .../Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx | 10 ++++------ .../PythonRunner/SkulptRunner/SkulptRunner.test.js | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx index f4ff4da45..e300ebe27 100644 --- a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx +++ b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx @@ -418,11 +418,7 @@ const SkulptRunner = ({ // rather than letting this handler itself throw. const rawDescription = typeof err.tp$str === "function" ? err.tp$str().v : undefined; - const errorDescription = ( - rawDescription || - err.message || - t("editor.errors.generalError") - ) + const errorDescription = (rawDescription || err.message || "") .replace(/\[(.*?)\]/, "") .replace(/\.$/, ""); const errorType = err.tp$name || err.constructor?.name; @@ -457,7 +453,9 @@ const SkulptRunner = ({ const { createError } = ApiCallHandler({ reactAppApiEndpoint }); - errorMessage = `${errorType}: ${errorDescription} on line ${lineNumber} of ${fileName}${ + const location = + lineNumber && fileName ? ` on line ${lineNumber} of ${fileName}` : ""; + errorMessage = `${errorType}: ${errorDescription}${location}${ explanation ? `. ${explanation}` : "" }`; createError(projectIdentifier, userId, { diff --git a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js index 656ae83e8..b2e2e659e 100644 --- a/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js +++ b/src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.test.js @@ -445,7 +445,7 @@ describe("When native code called from Python throws a plain JS error (e.g. the expect(store.getActions()).toEqual( expect.arrayContaining([ setError( - "TypeError: Cannot read properties of undefined (reading 'foo') on line undefined of undefined", + "TypeError: Cannot read properties of undefined (reading 'foo')", ), ]), ); @@ -460,7 +460,7 @@ describe("When native code called from Python throws a plain JS error (e.g. the file: undefined, description: "Cannot read properties of undefined (reading 'foo')", message: - "TypeError: Cannot read properties of undefined (reading 'foo') on line undefined of undefined", + "TypeError: Cannot read properties of undefined (reading 'foo')", }), ]), ); From 3c547a12a7fc2512135e8f03c70b44f42d67707a Mon Sep 17 00:00:00 2001 From: cocomarine Date: Wed, 5 Aug 2026 14:46:10 +0100 Subject: [PATCH 4/6] wrap p5/py5 shim functions in try catch block and route to Sk.uncaughtException --- cypress/e2e/spec-wc-skulpt.cy.js | 23 ++++++++++ public/shims/processing/p5/p5-shim.js | 60 +++++++++++++++---------- public/shims/processing/py5/py5-shim.js | 44 ++++++++++-------- 3 files changed, 85 insertions(+), 42 deletions(-) diff --git a/cypress/e2e/spec-wc-skulpt.cy.js b/cypress/e2e/spec-wc-skulpt.cy.js index d2fe74f18..18e5a3c05 100644 --- a/cypress/e2e/spec-wc-skulpt.cy.js +++ b/cypress/e2e/spec-wc-skulpt.cy.js @@ -60,6 +60,29 @@ describe("Running the code with skulpt", () => { getP5Canvas().should("exist"); }); + it("shows an error message when a p5 sketch raises in setup after an async preload", () => { + // preload() defers setup() to an async callback; without the shim wrapping + // its error escapes to window.onerror. No cy.on("uncaught:exception") + // allowance, so Cypress would fail the test if anything escaped. + const onePixelGif = + "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="; + runCode( + `from p5 import *\n\ndef preload():\n\tload_image("${onePixelGif}")\ndef setup():\n\tsize(400, 400)\n\traise ValueError('boom in setup')\ndef draw():\n\tpass\nrun(frame_rate=2)`, + ); + + getErrorMessage().should("contain.text", "ValueError: boom in setup"); + }); + + it("shows an error message when a p5 sketch raises in draw", () => { + // draw() runs every frame via requestAnimationFrame; the shim routes its + // errors to the editor rather than window.onerror. + runCode( + "from p5 import *\n\ndef setup():\n\tsize(400, 400)\ndef draw():\n\traise ValueError('boom in draw')\nrun(frame_rate=2)", + ); + + getErrorMessage().should("contain.text", "ValueError: boom in draw"); + }); + it("runs a simple py5 program", () => { runCode( "import py5\ndef setup():\n\tpy5.size(400, 400)\ndef draw():\n\tpy5.background(255)\npy5.run_sketch()", diff --git a/public/shims/processing/p5/p5-shim.js b/public/shims/processing/p5/p5-shim.js index 510c1f9b6..510b0c3f4 100644 --- a/public/shims/processing/p5/p5-shim.js +++ b/public/shims/processing/p5/p5-shim.js @@ -1512,45 +1512,57 @@ const $builtinmodule = function (name) { sketch.preload = function () { if (Sk.globals["preload"] && !isPy5Version) { - Sk.misceval.callsimArray(Sk.globals["preload"]); + // p5 calls preload/setup/draw asynchronously, so route exceptions + // through Sk.uncaughtException rather than let them hit window.onerror. + try { + Sk.misceval.callsimArray(Sk.globals["preload"]); + } catch (e) { + Sk.uncaughtException(e); + } } }; sketch.setup = function () { - if (Sk.globals["settings"] && isPy5Version) { - Sk.misceval.callsimArray(Sk.globals["settings"]); - } - if (Sk.globals["setup"]) { - Sk.misceval.callsimArray(Sk.globals["setup"]); - - for (const cb of Object.keys(callBacks)) { - if (Sk.globals[cb]) { - sketch[callBacks[cb]] = new Function( - "try {Sk.misceval.callsimArray(Sk.globals['" + - cb + - "']);} catch(e) {Sk.uncaughtException(e);}", - ); + try { + if (Sk.globals["settings"] && isPy5Version) { + Sk.misceval.callsimArray(Sk.globals["settings"]); + } + if (Sk.globals["setup"]) { + Sk.misceval.callsimArray(Sk.globals["setup"]); + + for (const cb of Object.keys(callBacks)) { + if (Sk.globals[cb]) { + sketch[callBacks[cb]] = new Function( + "try {Sk.misceval.callsimArray(Sk.globals['" + + cb + + "']);} catch(e) {Sk.uncaughtException(e);}", + ); + } } } + } catch (e) { + Sk.uncaughtException(e); } }; mod.pInst.frameRate(frame_rate.v); sketch.draw = function () { - mod.pInst.scale(scaleFactor, scaleFactor); - if (mod.__name__ === Sk.builtin.str("py5")) { - mod.frame_count = new Sk.builtin.int_(sketch.frameCount); - } else { - Sk.builtins.frame_count = new Sk.builtin.int_(sketch.frameCount); - } + // Wrap the whole body (not just the user draw() call) - draw runs every + // frame and any exception would otherwise escape to window.onerror. + try { + mod.pInst.scale(scaleFactor, scaleFactor); + if (mod.__name__ === Sk.builtin.str("py5")) { + mod.frame_count = new Sk.builtin.int_(sketch.frameCount); + } else { + Sk.builtins.frame_count = new Sk.builtin.int_(sketch.frameCount); + } - if (Sk.globals["draw"]) { - try { + if (Sk.globals["draw"]) { Sk.misceval.callsimArray(Sk.globals["draw"]); - } catch (e) { - Sk.uncaughtException(e); } + } catch (e) { + Sk.uncaughtException(e); } }; diff --git a/public/shims/processing/py5/py5-shim.js b/public/shims/processing/py5/py5-shim.js index 640eaa2d2..95575dfbb 100644 --- a/public/shims/processing/py5/py5-shim.js +++ b/public/shims/processing/py5/py5-shim.js @@ -1474,32 +1474,40 @@ const $builtinmodule = function (name) { mod.pInst = sketch; sketch.setup = function () { - if (Sk.globals["settings"]) { - Sk.misceval.callsimArray(Sk.globals["settings"]); - } - if (Sk.globals["setup"]) { - Sk.misceval.callsimArray(Sk.globals["setup"]); - - for (const cb of Object.keys(callBacks)) { - if (Sk.globals[cb]) { - sketch[callBacks[cb]] = new Function( - "try {Sk.misceval.callsimArray(Sk.globals['" + - cb + - "']);} catch(e) {Sk.uncaughtException(e);}", - ); + // p5 calls settings/setup/draw asynchronously, so route exceptions + // through Sk.uncaughtException rather than let them hit window.onerror. + try { + if (Sk.globals["settings"]) { + Sk.misceval.callsimArray(Sk.globals["settings"]); + } + if (Sk.globals["setup"]) { + Sk.misceval.callsimArray(Sk.globals["setup"]); + + for (const cb of Object.keys(callBacks)) { + if (Sk.globals[cb]) { + sketch[callBacks[cb]] = new Function( + "try {Sk.misceval.callsimArray(Sk.globals['" + + cb + + "']);} catch(e) {Sk.uncaughtException(e);}", + ); + } } } + } catch (e) { + Sk.uncaughtException(e); } }; sketch.draw = function () { - mod.frame_count = new Sk.builtin.int_(sketch.frameCount); - if (Sk.globals["draw"]) { - try { + // Wrap the whole body (not just the user draw() call) - draw runs every + // frame and any exception would otherwise escape to window.onerror. + try { + mod.frame_count = new Sk.builtin.int_(sketch.frameCount); + if (Sk.globals["draw"]) { Sk.misceval.callsimArray(Sk.globals["draw"]); - } catch (e) { - Sk.uncaughtException(e); } + } catch (e) { + Sk.uncaughtException(e); } }; From eadcf4c6dc005974ef404337695819f39702b0e0 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Wed, 5 Aug 2026 15:21:21 +0100 Subject: [PATCH 5/6] route errors through stopnOnError so that the functions stop --- cypress/e2e/spec-wc-skulpt.cy.js | 16 +++++++-------- public/shims/processing/p5/p5-shim.js | 27 ++++++++++++++++++++----- public/shims/processing/py5/py5-shim.js | 22 ++++++++++++++++---- 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/cypress/e2e/spec-wc-skulpt.cy.js b/cypress/e2e/spec-wc-skulpt.cy.js index 18e5a3c05..d3114475a 100644 --- a/cypress/e2e/spec-wc-skulpt.cy.js +++ b/cypress/e2e/spec-wc-skulpt.cy.js @@ -60,17 +60,17 @@ describe("Running the code with skulpt", () => { getP5Canvas().should("exist"); }); - it("shows an error message when a p5 sketch raises in setup after an async preload", () => { - // preload() defers setup() to an async callback; without the shim wrapping - // its error escapes to window.onerror. No cy.on("uncaught:exception") - // allowance, so Cypress would fail the test if anything escaped. - const onePixelGif = - "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="; + it("shows an error and stops the sketch when a p5 preload raises", () => { + // preload/setup run before any canvas exists, so VisualOutputPane can't + // stop the sketch. The shim must stop it, otherwise p5 treats the callback + // as successful and carries on into setup/draw - here draw() would print, + // which is asserted never to happen. runCode( - `from p5 import *\n\ndef preload():\n\tload_image("${onePixelGif}")\ndef setup():\n\tsize(400, 400)\n\traise ValueError('boom in setup')\ndef draw():\n\tpass\nrun(frame_rate=2)`, + "from p5 import *\n\ndef preload():\n\traise ValueError('boom in preload')\ndef setup():\n\tsize(400, 400)\ndef draw():\n\tprint('draw ran')\nrun(frame_rate=2)", ); - getErrorMessage().should("contain.text", "ValueError: boom in setup"); + getErrorMessage().should("contain.text", "ValueError: boom in preload"); + getSkulptRunner().should("not.contain", "draw ran"); }); it("shows an error message when a p5 sketch raises in draw", () => { diff --git a/public/shims/processing/p5/p5-shim.js b/public/shims/processing/p5/p5-shim.js index 510b0c3f4..bd1dba77d 100644 --- a/public/shims/processing/p5/p5-shim.js +++ b/public/shims/processing/p5/p5-shim.js @@ -1510,19 +1510,33 @@ const $builtinmodule = function (name) { Sk.builtin.str("py5_imported"), ].includes(mod.__name__); + // p5 runs preload/setup/draw asynchronously, so an uncaught exception + // there escapes to window.onerror. Catching it isn't enough on its own: + // p5 would treat the callback as successful and carry on into the draw + // loop, and VisualOutputPane only stops the sketch once a canvas exists - + // so a failure before then would keep looping. Surface the error and stop + // the loop here, and skip the remaining callbacks. + let sketchStopped = false; + const stopOnError = (e) => { + sketchStopped = true; + mod.pInst?.noLoop(); + Sk.uncaughtException(e); + }; + sketch.preload = function () { if (Sk.globals["preload"] && !isPy5Version) { - // p5 calls preload/setup/draw asynchronously, so route exceptions - // through Sk.uncaughtException rather than let them hit window.onerror. try { Sk.misceval.callsimArray(Sk.globals["preload"]); } catch (e) { - Sk.uncaughtException(e); + stopOnError(e); } } }; sketch.setup = function () { + if (sketchStopped) { + return; + } try { if (Sk.globals["settings"] && isPy5Version) { Sk.misceval.callsimArray(Sk.globals["settings"]); @@ -1541,13 +1555,16 @@ const $builtinmodule = function (name) { } } } catch (e) { - Sk.uncaughtException(e); + stopOnError(e); } }; mod.pInst.frameRate(frame_rate.v); sketch.draw = function () { + if (sketchStopped) { + return; + } // Wrap the whole body (not just the user draw() call) - draw runs every // frame and any exception would otherwise escape to window.onerror. try { @@ -1562,7 +1579,7 @@ const $builtinmodule = function (name) { Sk.misceval.callsimArray(Sk.globals["draw"]); } } catch (e) { - Sk.uncaughtException(e); + stopOnError(e); } }; diff --git a/public/shims/processing/py5/py5-shim.js b/public/shims/processing/py5/py5-shim.js index 95575dfbb..73ba65925 100644 --- a/public/shims/processing/py5/py5-shim.js +++ b/public/shims/processing/py5/py5-shim.js @@ -1473,9 +1473,20 @@ const $builtinmodule = function (name) { mod.pInst = sketch; + // p5 runs settings/setup/draw asynchronously, so an uncaught exception + // there escapes to window.onerror. Catching it isn't enough on its own: + // p5 would treat the callback as successful and carry on into the draw + // loop, and VisualOutputPane only stops the sketch once a canvas exists - + // so a failure before then would keep looping. Surface the error and stop + // the loop here, and skip the remaining callbacks. + let sketchStopped = false; + const stopOnError = (e) => { + sketchStopped = true; + mod.pInst?.noLoop(); + Sk.uncaughtException(e); + }; + sketch.setup = function () { - // p5 calls settings/setup/draw asynchronously, so route exceptions - // through Sk.uncaughtException rather than let them hit window.onerror. try { if (Sk.globals["settings"]) { Sk.misceval.callsimArray(Sk.globals["settings"]); @@ -1494,11 +1505,14 @@ const $builtinmodule = function (name) { } } } catch (e) { - Sk.uncaughtException(e); + stopOnError(e); } }; sketch.draw = function () { + if (sketchStopped) { + return; + } // Wrap the whole body (not just the user draw() call) - draw runs every // frame and any exception would otherwise escape to window.onerror. try { @@ -1507,7 +1521,7 @@ const $builtinmodule = function (name) { Sk.misceval.callsimArray(Sk.globals["draw"]); } } catch (e) { - Sk.uncaughtException(e); + stopOnError(e); } }; From 3e5254a3c84faca5fcc968519c5eec81d399ca4d Mon Sep 17 00:00:00 2001 From: cocomarine Date: Thu, 6 Aug 2026 11:45:59 +0100 Subject: [PATCH 6/6] fix flaky resize test --- cypress/helpers/editor.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/cypress/helpers/editor.js b/cypress/helpers/editor.js index bc2c570a3..79dcce329 100644 --- a/cypress/helpers/editor.js +++ b/cypress/helpers/editor.js @@ -93,7 +93,11 @@ const dragHandle = (getHandle, { deltaX = 0, deltaY = 0 }) => { const clientX = left + width / 2; const clientY = top + height / 2; - cy.wrap($handle).trigger("mousedown", { button: 0, clientX, clientY }); + // Re-query the handle instead of reusing $handle. If the app re-renders + // while trigger() is waiting for the element to become actionable, a + // wrapped element is detached for good and trigger() burns its full + // timeout; a query chain retries against the current element. + getHandle().trigger("mousedown", { button: 0, clientX, clientY }); getHandle() .parent("div") @@ -125,6 +129,14 @@ export const openFilePanel = () => export const loadPythonStarterProject = () => { cy.findByText("blank-python-starter").click(); + + // web-component.html fetches the project JSON, then throws away the current + // and prepends a brand new one. The component being replaced + // already has a visible Run button, so waiting on that alone can resolve + // against the doomed component and leave the test holding elements that + // detach part-way through. Only the replacement publishes an identifier. + cy.get("#project-identifier").should("have.text", "blank-python-starter"); + getEditorShadow().findByRole("button", { name: /run/i }).should("be.visible"); };