Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { useTranslation } from "react-i18next";
import classNames from "classnames";
import {
setError,
setErrorDetails,
setFriendlyError,
codeRunHandled,
setLoadedRunner,
Expand Down Expand Up @@ -217,9 +218,14 @@ const PyodideRunner = ({

const handleError = (file, line, mistake, type, info, rawTraceback) => {
let errorMessage;
let errorDetails = {};

if (type === "KeyboardInterrupt") {
errorMessage = t("output.errors.interrupted");
errorDetails = {
type: "Interrupted",
message: errorMessage,
};
} else {
const message = [type, info].filter((s) => s).join(": ");
errorMessage = [message, `on line ${line} of ${file}`].join(" ");
Expand All @@ -228,6 +234,14 @@ const PyodideRunner = ({
errorMessage += `:\n${mistake}`;
}

errorDetails = {
type,
line,
file,
description: info || mistake || "",
message: errorMessage,
};

const { createError } = ApiCallHandler({
reactAppApiEndpoint,
});
Expand Down Expand Up @@ -257,6 +271,7 @@ const PyodideRunner = ({
}

dispatch(setError(errorMessage));
dispatch(setErrorDetails(errorDetails));
disableInput();
};

Expand Down Expand Up @@ -303,6 +318,7 @@ const PyodideRunner = ({
const handleRun = async () => {
output.current.innerHTML = "";
dispatch(setError(""));
dispatch(setErrorDetails({}));
dispatch(setFriendlyError(null));
setVisuals([]);
stdinClosed.current = false;
Expand Down
35 changes: 30 additions & 5 deletions src/components/WebComponentProject/WebComponentProject.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect, useState } from "react";
import React, { useEffect, useRef, useState } from "react";
import { useDispatch, useSelector } from "react-redux";
import { useMediaQuery } from "react-responsive";
import { marked } from "marked";
Expand Down Expand Up @@ -52,6 +52,7 @@ const WebComponentProject = ({

const error = useSelector((state) => state.editor.error);
const errorDetails = useSelector((state) => state.editor.errorDetails);
const friendlyError = useSelector((state) => state.editor.friendlyError);
const codeHasBeenRun = useSelector((state) => state.editor.codeHasBeenRun);
const projectInstructions = useSelector(
(state) => state.editor.project.instructions,
Expand All @@ -64,6 +65,7 @@ const WebComponentProject = ({
);
const isMobile = useMediaQuery({ query: MOBILE_MEDIA_QUERY });
const [codeHasRun, setCodeHasRun] = useState(codeHasBeenRun);
const prevCodeRunTriggeredRef = useRef(false);
const dispatch = useDispatch();
const renderer = new marked.Renderer();

Expand Down Expand Up @@ -120,19 +122,39 @@ const WebComponentProject = ({
}, [dispatch, projectInstructions, permitInstructionsOverride]);

useEffect(() => {
if (codeRunTriggered) {
document.dispatchEvent(runStartedEvent({ step: currentStepPosition }));
if (codeRunTriggered && !prevCodeRunTriggeredRef.current) {
document.dispatchEvent(
runStartedEvent({
step: currentStepPosition,
projectIdentifier,
projectType,
}),
);
setCodeHasRun(true);
} else if (codeHasRun) {
}
prevCodeRunTriggeredRef.current = codeRunTriggered;
}, [codeRunTriggered, currentStepPosition, projectIdentifier, projectType]);

useEffect(() => {
if (!codeRunTriggered && codeHasRun) {
const mz_criteria = Sk.sense_hat
? Sk.sense_hat.mz_criteria
: { ...defaultMZCriteria };

const payload = outputOnly
? { errorDetails, step: currentStepPosition }
? {
errorDetails,
step: currentStepPosition,
projectIdentifier,
projectType,
}
: {
isErrorFree: error === "",
step: currentStepPosition,
errorDetails,
friendlyErrorShown: Boolean(friendlyError?.html),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale friendly error tracking flag

Medium Severity

The new friendlyErrorShown field on editor-runCompleted is derived only from friendlyError?.html in Redux. Skulpt clears error and errorDetails at run start but does not clear friendlyError, so a successful run after a failed one can still emit friendlyErrorShown: true while isErrorFree is true.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1174d2a. Configure here.

projectIdentifier,
projectType,
...mz_criteria,
};

Expand All @@ -144,7 +166,10 @@ const WebComponentProject = ({
outputOnly,
error,
errorDetails,
friendlyError,
currentStepPosition,
projectIdentifier,
projectType,
]);

useEffect(() => {
Expand Down
109 changes: 99 additions & 10 deletions src/components/WebComponentProject/WebComponentProject.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,17 @@ const renderWebComponentProject = ({
loading,
codeRunTriggered = false,
codeHasBeenRun = false,
error = "",
errorDetails,
friendlyError,
props = {},
}) => {
const middlewares = [];
const mockStore = configureStore(middlewares);
const initialState = {
editor: {
project: {
identifier: "test-project",
project_type: projectType,
components: [
{ name: "main", extension: "py", content: "print('hello')" },
Expand All @@ -47,6 +51,9 @@ const renderWebComponentProject = ({
focussedFileIndices: [],
codeRunTriggered,
codeHasBeenRun,
error,
errorDetails,
friendlyError,
},
instructions: {
currentStepPosition: 3,
Expand All @@ -65,7 +72,9 @@ const renderWebComponentProject = ({

describe("When state set", () => {
beforeEach(() => {
runStartedHandler.mockClear();
renderWebComponentProject({
projectType: "python",
instructions: "My amazing instructions",
codeRunTriggered: true,
});
Expand All @@ -83,9 +92,15 @@ describe("When state set", () => {
expect(codeChangedHandler.mock.lastCall[0].detail).toHaveProperty("step");
});

test("Triggers runStarted event", () => {
expect(runStartedHandler).toHaveBeenCalled();
expect(runStartedHandler.mock.lastCall[0].detail).toHaveProperty("step");
test("Triggers runStarted event once", () => {
expect(runStartedHandler).toHaveBeenCalledTimes(1);
expect(runStartedHandler.mock.lastCall[0].detail).toEqual(
expect.objectContaining({
step: 3,
projectIdentifier: "test-project",
projectType: "python",
}),
);
});

test("Triggers stepChanged event", () => {
Expand Down Expand Up @@ -232,24 +247,98 @@ describe("When overriding instructions is not permitted", () => {

describe("When code run finishes", () => {
test("Triggers runCompletedEvent", () => {
renderWebComponentProject({ codeHasBeenRun: true });
renderWebComponentProject({
projectType: "python",
codeHasBeenRun: true,
});
expect(runCompletedHandler).toHaveBeenCalled();
expect(runCompletedHandler.mock.lastCall[0].detail).toHaveProperty(
"isErrorFree",
expect(runCompletedHandler.mock.lastCall[0].detail).toEqual(
expect.objectContaining({
isErrorFree: true,
step: 3,
projectIdentifier: "test-project",
projectType: "python",
errorDetails: undefined,
friendlyErrorShown: false,
}),
);
});

test("includes error details and friendly error state when a run failed", () => {
const middlewares = [];
const mockStore = configureStore(middlewares);
const initialState = {
editor: {
project: {
identifier: "test-project",
project_type: "python",
components: [
{ name: "main", extension: "py", content: "print('hello')" },
],
image_list: [],
},
loading: "success",
openFiles: [],
focussedFileIndices: [],
codeRunTriggered: false,
codeHasBeenRun: true,
error: "NameError: name 'kettle' is not defined on line 5 of main.py",
errorDetails: {
type: "NameError",
file: "main.py",
line: 5,
description: "name 'kettle' is not defined",
},
friendlyError: {
html: "<p>Friendly error</p>",
},
},
instructions: {
currentStepPosition: 3,
permitOverride: true,
},
auth: {},
};
store = mockStore(initialState);

render(
<Provider store={store}>
<WebComponentProject />
</Provider>,
);

expect(runCompletedHandler).toHaveBeenCalled();
expect(runCompletedHandler.mock.lastCall[0].detail).toEqual(
expect.objectContaining({
isErrorFree: false,
errorDetails: {
type: "NameError",
file: "main.py",
line: 5,
description: "name 'kettle' is not defined",
},
friendlyErrorShown: true,
projectIdentifier: "test-project",
projectType: "python",
}),
);
expect(runCompletedHandler.mock.lastCall[0].detail).toHaveProperty("step");
});

test("Triggers runCompletedEvent with error details when outputOnly is true", () => {
renderWebComponentProject({
projectType: "python",
codeHasBeenRun: true,
props: { outputOnly: true },
});
expect(runCompletedHandler).toHaveBeenCalled();
expect(runCompletedHandler.mock.lastCall[0].detail).toHaveProperty(
"errorDetails",
expect(runCompletedHandler.mock.lastCall[0].detail).toEqual(
expect.objectContaining({
errorDetails: undefined,
step: 3,
projectIdentifier: "test-project",
projectType: "python",
}),
);
expect(runCompletedHandler.mock.lastCall[0].detail).toHaveProperty("step");
});
});

Expand Down
Loading