Skip to content

Handle non-standard p5/py5 exceptions - #1561

Open
cocomarine wants to merge 16 commits into
mainfrom
1652-handle-non-standard-p5-exceptions
Open

Handle non-standard p5/py5 exceptions#1561
cocomarine wants to merge 16 commits into
mainfrom
1652-handle-non-standard-p5-exceptions

Conversation

@cocomarine

@cocomarine cocomarine commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Closes https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1652

Possible problem

  • Skulpt exceptions raised in p5/py5 sketches were escaping to window.onerror, reported by Sentry as the opaque "Non-Error exception captured with keys: $d, args, traceback".
  • The captured object is a Skulpt Python exception with an empty traceback (constructed in JS, e.g. p5's _friendlyError), thrown from an async p5 callback , so it never reached our error handling.

Changes

  • Route preload/setup/draw (whole body, incl. the per-frame preamble) through Sk.uncaughtException, and stop the sketch (stopOnError) so a pre-canvas failure can't leave the draw loop running.
  • Harden handleError against non-standard exceptions (missing tp$str, empty traceback, no constructor).
  • Aslo fixed flaky tests in spec-wc-resize.cy.js by updating the helper file:
    • Hardened dragHandle to re-query the handle for trigger("mousedown") and made loadPythonStarterProject wait on #project-identifier so it resolves against the replacement rather than the one about to be destroyed.

Note

This is defensive hardening for the async-escape paths; the exact production trigger for EDITOR-UI-ABW was a private learner sketch and could not be reproduced. Behaviour is unchanged.

Generated with the help of Claude

Further investigation

  • There are other similar errors (current error in the top row).
  • Need to investigate if they share the same route cause or are different problems.
Screenshot 2026-08-05 at 11 34 56

@cocomarine
cocomarine temporarily deployed to previews/1561/merge July 29, 2026 11:46 — with GitHub Actions Inactive
@cocomarine cocomarine changed the title Cater for non-standard exception and make handleError defensive Make handleError defensive Jul 29, 2026
@cocomarine
cocomarine temporarily deployed to previews/1561/merge July 29, 2026 13:40 — with GitHub Actions Inactive
@cocomarine
cocomarine marked this pull request as ready for review July 29, 2026 14:26
@cocomarine
cocomarine temporarily deployed to previews/1561/merge July 31, 2026 10:05 — with GitHub Actions Inactive
Comment thread src/components/Editor/Runners/PythonRunner/SkulptRunner/SkulptRunner.jsx Outdated
@cocomarine
cocomarine temporarily deployed to previews/1561/merge July 31, 2026 12:58 — with GitHub Actions Inactive
@cocomarine
cocomarine temporarily deployed to previews/1561/merge July 31, 2026 13:00 — with GitHub Actions Inactive
@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 4, 2026 07:41 — with GitHub Actions Inactive
@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 5, 2026 09:23 — with GitHub Actions Inactive
@zetter-rpf

Copy link
Copy Markdown
Contributor

Do you know of any examples of p5 code that can cause this kind of error? I'm trying to understand if the error we show to the user would be clear in this cases (and that context would be useful to have in the PR)

@cocomarine

Copy link
Copy Markdown
Contributor Author

Do you know of any examples of p5 code that can cause this kind of error? I'm trying to understand if the error we show to the user would be clear in this cases (and that context would be useful to have in the PR)

Thanks, Chris. I will add some more context to the PR note. I think it needs more investigating (perhaps along with other similar errors).

@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 5, 2026 11:14 — with GitHub Actions Inactive
@cocomarine cocomarine changed the title Make handleError defensive Handle non-standard p5/py5 exceptions Aug 5, 2026
@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 5, 2026 13:46 — with GitHub Actions Inactive

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1683ad6. Configure here.

Comment thread public/shims/processing/p5/p5-shim.js
@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 5, 2026 14:21 — with GitHub Actions Inactive
@cocomarine

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

…ard-p5-exceptions

Co-authored-by: cocomarine <12242598+cocomarine@users.noreply.github.com>

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved — merged origin/main into the branch cleanly (no conflicting changes). The branch is now up to date.

Copilot AI temporarily deployed to previews/1561/merge August 5, 2026 15:56 Inactive
@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 6, 2026 07:41 — with GitHub Actions Inactive
…aspberryPiFoundation/editor-ui into 1652-handle-non-standard-p5-exceptions
@cocomarine
cocomarine temporarily deployed to previews/1561/merge August 6, 2026 10:46 — with GitHub Actions Inactive

@DNR500 DNR500 left a comment

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.

Looks good to me :-)

Tried the preload/draw examples locally - was also nice to see that the Visual Output loading indicator actually stops too as part of this.

Image

);

getErrorMessage().should("contain.text", "ValueError: boom in preload");
getSkulptRunner().should("not.contain", "draw ran");

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.

Nice that this checks draw never runs - that’s the important bit for the stuck loading indicator


for (const cb of Object.keys(callBacks)) {
if (Sk.globals[cb]) {
sketch[callBacks[cb]] = new Function(

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.

Small note - not a blocker.

If preload, setup, or draw fails, we show the error and stop the sketch. If something like mouse_pressed fails, we still show the error but the sketch keeps running. Fine for this PR - might be worth matching later but I don't think it necessarily needs to be done on this PR

Comment thread cypress/helpers/editor.js
// 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 });

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.

Nice fix - dropping cy.wrap($handle) and refinding the handle before mousedown is a good call.

The flake comes from a race - while we’re mid-drag (or even just waiting to start it), the app can rerender and replace the resize handle with a new DOM node. Cypress then keeps poking the old one until it times out. Requerying means we always hit the current handle, which should clear most of these failures. Waiting for #project-identifier before trusting the Run button is a good companion change for the same class of “element got swapped out underneath us” problem.

If this still flakes occasionally, we could harden a bit further without changing the app - e.g. wait until the handle’s position/size stops shifting before we drag, especially in the stacked layout test where things settle more slowly near the layout breakpoints. Happy to leave that until we see whether this is enough on its own

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants