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
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@
"@sentry/react": "^10.5.0",
"@sourceacademy/autocomplete": "github:source-academy/autocomplete#e669d9ed98753350a3c8433a92985227eb789663",
"@sourceacademy/c-slang": "^1.0.21",
"@sourceacademy/conductor": "https://github.com/source-academy/conductor.git#0.4.0",
"@sourceacademy/conductor": "portal:../conductor",
"@sourceacademy/language-directory": "https://github.com/source-academy/language-directory.git#0.0.6",
"@sourceacademy/plugin-directory": "https://github.com/source-academy/plugin-directory.git#0.0.2",
"@sourceacademy/plugin-directory": "portal:../plugin-directory",
Comment on lines +49 to +51

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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Verify portal reference targets exist and are valid packages

echo "=== Checking portal reference targets ==="

if [ -d "../conductor" ]; then
  echo "✓ ../conductor directory exists"
  if [ -f "../conductor/package.json" ]; then
    echo "✓ ../conductor/package.json exists"
    cat ../conductor/package.json | jq -r '.name, .version'
  else
    echo "✗ ../conductor/package.json not found"
  fi
else
  echo "✗ ../conductor directory not found"
fi

if [ -d "../plugin-directory" ]; then
  echo "✓ ../plugin-directory directory exists"
  if [ -f "../plugin-directory/package.json" ]; then
    echo "✓ ../plugin-directory/package.json exists"
    cat ../plugin-directory/package.json | jq -r '.name, .version'
  else
    echo "✗ ../plugin-directory/package.json not found"
  fi
else
  echo "✗ ../plugin-directory directory not found"
fi

echo ""
echo "=== Checking if yarn pack works with current dependencies ==="
yarn pack --dry-run

Repository: source-academy/frontend

Length of output: 738


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Repo root =="
pwd
echo "== Parent dir listing =="
ls -la ..

echo "== Search for portal: in package.json and other files =="
rg -n "portal:|portal:" package.json **/package.json .yarnrc* .yarnrc.yml .npmrc 2>/dev/null || true

echo "== Locate conductor/plugin-directory in repo =="
fd -t d -a "conductor|plugin-directory" . 2>/dev/null | head -n 50 || true

echo "== Show relevant package.json section =="
if [ -f package.json ]; then
  python3 - <<'PY'
import json
from pathlib import Path
p=Path("package.json")
d=json.loads(p.read_text())
for k in ["dependencies","devDependencies","peerDependencies","optionalDependencies"]:
    if k in d:
        deps=d[k]
        for name in ["`@sourceacademy/conductor`","`@sourceacademy/plugin-directory`","`@sourceacademy/language-directory`","`@sourceacademy/language-directory`"]:
            if name in deps:
                print(k, name, "=>", deps[name])
PY
fi

echo "== Yarn config files =="
ls -la .yarnrc* .yarn 2>/dev/null || true

echo "== Any CI/workflow referencing sibling checkout paths =="
fd -t f -a ".github/workflows|.gitlab-ci|circleci|build" . 2>/dev/null || true
rg -n "conductor|plugin-directory|portal:|yarn pack|yarn install|workspaces" .github .gitlab-ci.yml .circleci 2>/dev/null || true

Repository: source-academy/frontend

Length of output: 2018


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== pwd =="
pwd
echo "== realpath package.json =="
realpath package.json
echo "== package.json dir listing (first 30 entries) =="
ls -la "$(dirname "$(realpath package.json)")" | head -n 30

echo "== Find plugin-directory directories in repo =="
fd -t d -a "plugin-directory" . 2>/dev/null | head -n 50 || true

echo "== Show existing conductor/plugin candidates near package.json =="
ls -la "$(dirname "$(realpath package.json)")/.." | head -n 50 || true

echo "== Show .yarnrc.yml (or relevant parts) =="
if [ -f .yarnrc.yml ]; then
  sed -n '1,220p' .yarnrc.yml
fi

Repository: source-academy/frontend

Length of output: 191


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== pwd =="
pwd

echo "== Abspath package.json =="
python3 - <<'PY'
from pathlib import Path
p=Path("package.json").resolve()
print(p)
PY

echo "== package.json dir listing (first 40 entries) =="
python3 - <<'PY'
from pathlib import Path
import os, subprocess
p=Path("package.json").resolve()
d=p.parent
print(d)
subprocess.run(["ls","-la",str(d)],check=False)
PY | head -n 60

echo "== Find plugin-directory directories in repo =="
fd -t d -a "plugin-directory" . 2>/dev/null | head -n 50 || true

echo "== Find conductor directories in repo =="
fd -t d -a "conductor" . 2>/dev/null | head -n 50 || true

echo "== Show .yarnrc.yml relevant parts =="
if [ -f .yarnrc.yml ]; then
  sed -n '1,220p' .yarnrc.yml
fi

echo "== Show any portal: usage with relative targets =="
rg -n "portal:[.]{2}/" package.json **/package.json 2>/dev/null || true

Repository: source-academy/frontend

Length of output: 409


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== pwd =="
pwd

echo "== Abspath package.json =="
python3 - <<'PY'
from pathlib import Path
print(Path("package.json").resolve())
PY

echo "== package.json dir listing (first 40 entries) =="
python3 - <<'PY'
from pathlib import Path
p=Path("package.json").resolve()
d=p.parent
print(str(d))
PY
dir="$(python3 - <<'PY'
from pathlib import Path
print(str(Path("package.json").resolve().parent))
PY
)"
ls -la "$dir" | head -n 40

echo "== Find plugin-directory directories in repo =="
fd -t d -a "plugin-directory" . 2>/dev/null | head -n 50 || true

echo "== Find conductor directories in repo =="
fd -t d -a "conductor" . 2>/dev/null | head -n 50 || true

echo "== Show .yarnrc.yml relevant parts (up to first 220 lines) =="
if [ -f .yarnrc.yml ]; then
  sed -n '1,220p' .yarnrc.yml
fi

echo "== Search for relative portal: targets (portal:./ or portal:../) =="
rg -n "portal:[.]{1,2}/" package.json **/package.json 2>/dev/null || true

Repository: source-academy/frontend

Length of output: 2862


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect CI workflow install steps =="
sed -n '1,200p' .github/workflows/ci.yml
echo "----"
sed -n '1,220p' .github/workflows/build-development.yml

echo "== Search for plugin-directory/conductor in repo (filenames + strings) =="
rg -n "plugin-directory|`@sourceacademy/plugin-directory`|portal:../plugin-directory|portal:../conductor|`@sourceacademy/conductor`|portal:../" . || true

echo "== Search for other absolute portal: paths =="
rg -n "portal:/" package.json **/package.json 2>/dev/null || true

echo "== Locate any directories that might be the portal targets =="
fd -t d -a "conductor" . 2>/dev/null | head -n 50 || true
fd -t d -a "plugin-directory" . 2>/dev/null | head -n 50 || true

echo "== Inspect package.json around portal deps =="
node - <<'NODE'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
const deps = {...pkg.dependencies};
for (const k of ['`@sourceacademy/conductor`','`@sourceacademy/plugin-directory`','`@sourceacademy/language-directory`']) {
  console.log(k, '=>', deps[k]);
}
NODE

Repository: source-academy/frontend

Length of output: 8441


Fix CI to satisfy portal: dependency targets (or update the portal paths).

  • package.json points to @sourceacademy/conductor (portal:../conductor) and @sourceacademy/plugin-directory (portal:../plugin-directory), which require conductor/ and plugin-directory/ to exist as sibling directories of the frontend checkout.
  • CI (.github/workflows/ci.yml and build-development.yml) only checks out this single repo and runs yarn install --immutable, without fetching/creating those sibling directories—so the portal: targets will be missing in CI.
  • In the current checkout, ../conductor and ../plugin-directory do not exist, so resolution can’t work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package.json` around lines 49 - 51, package.json currently uses portal:
targets for "`@sourceacademy/conductor`" and "`@sourceacademy/plugin-directory`"
which fail in CI because ../conductor and ../plugin-directory are not present;
either replace those portal: entries with published version specs (e.g. the git
URL or semver) for "`@sourceacademy/conductor`" and
"`@sourceacademy/plugin-directory`" or modify the CI workflows to check out the
sibling repos before yarn install (use actions/checkout for this repo and
additional checkouts into ../conductor and ../plugin-directory or a workspace
layout so portal: resolution succeeds); update .github/workflows/ci.yml and
build-development.yml to perform the extra checkouts (or switch package.json
entries) and ensure yarn install --immutable runs after the sibling directories
are present.

Source: Pipeline failures

"@sourceacademy/sharedb-ace": "2.1.1",
"@sourceacademy/sling-client": "^0.1.0",
"@szhsin/react-menu": "^4.0.0",
Expand Down Expand Up @@ -196,6 +196,7 @@
},
"resolutions": {
"@types/estree": "1.0.9",
"vite": "^8.0.0"
"vite": "^8.0.0",
"js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang"

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.

high

The absolute local path portal:/Users/shreyjain/Downloads/source-academy/js-slang is hardcoded here. This will break the build for other developers and in CI/CD environments. Please use a relative path instead.

Suggested change
"js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang"
"js-slang": "portal:../js-slang"

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 | 🔴 Critical | ⚡ Quick win

Remove hardcoded absolute path to js-slang.

The js-slang resolution uses an absolute path /Users/shreyjain/Downloads/source-academy/js-slang that is specific to one developer's machine. This will break for all other developers and in CI/CD environments.

🔧 Recommended fixes

Option 1 (preferred if js-slang is in a sibling directory): Use a relative portal path:

-    "js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang"
+    "js-slang": "portal:../js-slang"

Option 2 (if this override was only for local testing): Remove the resolution entirely and use the version from dependencies:

  "resolutions": {
    "`@types/estree`": "1.0.9",
    "vite": "^8.0.0",
-    "js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang"
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang"
"js-slang": "portal:../js-slang"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package.json` at line 200, The package.json contains a hardcoded absolute
portal path for the dependency "js-slang" ("js-slang":
"portal:/Users/shreyjain/Downloads/source-academy/js-slang") which will break
for other devs/CI; update the "js-slang" resolution to either a relative portal
path (e.g. use "portal:../path/to/js-slang" if js-slang is a sibling/local
checkout) or remove the portal override entirely so the resolver uses the
published version from dependencies/registry; locate the "js-slang" key in
package.json and replace or delete the portal:/Users/... value accordingly.

}
}
75 changes: 75 additions & 0 deletions public/evaluators/js-slang/SourceStepperEvaluator1.js

Large diffs are not rendered by default.

75 changes: 75 additions & 0 deletions public/evaluators/js-slang/SourceStepperEvaluator2.js

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator1.cjs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator1.js

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator2.cjs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator2.js

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do the evaluators need to be added to the frontend?

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.

@Shrey5132 I was confused bout this as well. They can be built from js-slang just like how py-slang does it.

py-slang CI:

  1. Builds all evaluators (yarn run build --all) on every push to main
  2. Uploads the dist/ folder as a GitHub Pages artifact
  3. Deploys it to GitHub Pages

So the evaluator .js files live at a GitHub Pages URL like https://source-academy.github.io/py-slang/PyCseEvaluator1.js. The frontend's language-directory (or local-directory.json) references those URLs — it doesn't need the files locally at all.

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator3.cjs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator3.js

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator4.cjs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PyCseEvaluator4.js

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PySvmlEvaluator.cjs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PySvmlEvaluator.js

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PySvmlSinterEvaluator.cjs

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions public/evaluators/py-slang/PySvmlSinterEvaluator.js

Large diffs are not rendered by default.

11 changes: 11 additions & 0 deletions public/evaluators/py-slang/PyWasmEvaluator.cjs

Large diffs are not rendered by default.

11 changes: 11 additions & 0 deletions public/evaluators/py-slang/PyWasmEvaluator.js

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions public/evaluators/py-slang/PythonStepperEvaluator1.cjs

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions public/evaluators/py-slang/PythonStepperEvaluator1.js

Large diffs are not rendered by default.

14 changes: 14 additions & 0 deletions public/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,20 @@
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no" />
<meta name="theme-color" content="#000000" />
<!--
Import map so dynamically-imported Conductor web plugin bundles resolve these bare
specifiers to the host frontend's singleton instances (see src/bootstrap/conductorSharedDeps.ts
and public/shims/*). Keeps a single React tree and identical Blueprint styling.
-->
<script type="importmap">
{
"imports": {
"react": "<%= assetPrefix %>/shims/react.mjs",
"react/jsx-runtime": "<%= assetPrefix %>/shims/react-jsx-runtime.mjs",
"@blueprintjs/core": "<%= assetPrefix %>/shims/blueprintjs-core.mjs"
}
}
</script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/gsap/2.0.0/TweenMax.min.js"
integrity="sha256-yYE5i030zFWS1QObMdcpEqiGU10EwGSEAQaYGnQBUs0="
crossorigin="anonymous"></script>
Expand Down
38 changes: 38 additions & 0 deletions public/languages/directory.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
[
{
"id": "source-1",
"name": "Source 1 (Stepper)",
"evaluators": [
{
"id": "default",
"name": "Default",
"path": "/evaluators/js-slang/SourceStepperEvaluator1.js",
"capabilities": []
}
]
},
{
"id": "source-2",
"name": "Source 2 (Stepper)",
"evaluators": [
{
"id": "default",
"name": "Default",
"path": "/evaluators/js-slang/SourceStepperEvaluator2.js",
"capabilities": []
}
]
},
{
"id": "python-1",
"name": "Python 1 (Stepper)",
"evaluators": [
{
"id": "default",
"name": "Default",
"path": "/evaluators/py-slang/PythonStepperEvaluator1.js",
"capabilities": []
}
]
}
]
10 changes: 10 additions & 0 deletions public/plugins/directory.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[
{
"id": "stepper",
"name": "Stepper",
"description": "Visualises the step-by-step substitution evaluation of a program.",
"resolutions": {
"web": "/plugins/stepper/index.mjs"
}
}
]
6 changes: 6 additions & 0 deletions public/plugins/stepper/index.mjs

Large diffs are not rendered by default.

17 changes: 17 additions & 0 deletions public/shims/blueprintjs-core.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Import-map shim: resolves a plugin bundle's `import ... from "@blueprintjs/core"` to the host
// frontend's Blueprint build (exposed on globalThis by src/bootstrap/conductorSharedDeps.ts), so the
// plugin's Blueprint components share the host's CSS and React instance.
//
// Re-exports the surface used by the bundled plugins; extend this list if a plugin needs more.
const Blueprint = globalThis.__SA_BLUEPRINT__;
export const {
Button,
ButtonGroup,
Card,
Classes,
Divider,
Icon,
Popover,
Pre,
Slider,
} = Blueprint;
4 changes: 4 additions & 0 deletions public/shims/react-jsx-runtime.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Import-map shim: resolves a plugin bundle's `import ... from "react/jsx-runtime"` to the host
// frontend's React jsx-runtime (exposed on globalThis by src/bootstrap/conductorSharedDeps.ts).
const jsxRuntime = globalThis.__SA_REACT_JSX__;
export const { jsx, jsxs, Fragment } = jsxRuntime;
41 changes: 41 additions & 0 deletions public/shims/react.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Import-map shim: resolves a plugin bundle's `import ... from "react"` to the host frontend's
// single React instance (exposed on globalThis by src/bootstrap/conductorSharedDeps.ts).
const React = globalThis.__SA_REACT__;

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 | 🟠 Major | ⚡ Quick win

All three shim modules lack defensive checks for missing globals. The shim files public/shims/react.mjs, public/shims/react-jsx-runtime.mjs, and public/shims/blueprintjs-core.mjs all read from globalThis properties (__SA_REACT__, __SA_REACT_JSX__, __SA_BLUEPRINT__) and immediately destructure them. If src/bootstrap/conductorSharedDeps.ts fails to execute or loads in the wrong order, these globals will be undefined and destructuring will throw cryptic TypeError exceptions. Add runtime guards in each shim that throw clear error messages identifying the missing bootstrap step.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@public/shims/react.mjs` at line 3, Each shim must guard against missing
bootstrap globals and throw a clear error when they’re undefined: update
public/shims/react.mjs to check globalThis.__SA_REACT__ before assigning React
(and throw an Error mentioning the missing conductorSharedDeps/bootstrap and
"__SA_REACT__" if absent), do the same in public/shims/react-jsx-runtime.mjs for
globalThis.__SA_REACT_JSX__ (guard before using React JSX runtime exports), and
in public/shims/blueprintjs-core.mjs for globalThis.__SA_BLUEPRINT__ (guard
before destructuring blueprint exports); ensure the thrown message names the
missing global (e.g., "__SA_REACT__", "__SA_REACT_JSX__", "__SA_BLUEPRINT__")
and instructs that conductorSharedDeps bootstrap must run first.

export default React.default ?? React;
export const {
Children,
Component,
Fragment,
Profiler,
PureComponent,
StrictMode,
Suspense,
cloneElement,
createContext,
createElement,
createRef,
forwardRef,
isValidElement,
lazy,
memo,
startTransition,
use,
useActionState,
useCallback,
useContext,
useDebugValue,
useDeferredValue,
useEffect,
useId,
useImperativeHandle,
useInsertionEffect,
useLayoutEffect,
useMemo,
useOptimistic,
useReducer,
useRef,
useState,
useSyncExternalStore,
useTransition,
version,
} = React;
20 changes: 20 additions & 0 deletions src/bootstrap/conductorSharedDeps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Exposes the frontend's singleton library instances so that dynamically-imported Conductor web
* plugin bundles can share them, rather than bundling (and duplicating) their own copies.
*
* Plugin bundles import `react`, `react/jsx-runtime` and `@blueprintjs/core` as bare specifiers.
* The import map in `public/index.html` maps those specifiers to the shim modules in `public/shims`,
* which re-export the globals set here. The result: the plugin renders inside the host's single
* React tree and uses the host's exact Blueprint build (so styling is identical).
*
* This module must be imported before any plugin is loaded; it is imported first from `index.tsx`.
*/
import * as Blueprint from '@blueprintjs/core';
// eslint-disable-next-line no-restricted-imports
import * as React from 'react';
import * as ReactJsxRuntime from 'react/jsx-runtime';

const globals = globalThis as Record<string, unknown>;
globals.__SA_REACT__ = React;
globals.__SA_REACT_JSX__ = ReactJsxRuntime;
globals.__SA_BLUEPRINT__ = Blueprint;
91 changes: 63 additions & 28 deletions src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -448,9 +448,14 @@ function* handleResults(
workspaceLocation: WorkspaceLocation,
): SagaIterator {
const resultChan = eventChannel(emitter => {
hostPlugin.receiveResult = emitter;
// redux-saga's eventChannel throws on `undefined`; an evaluator may legitimately produce an
// undefined result, so drop it rather than crash the channel.
const receive = (result: any) => {
if (result !== undefined) emitter(result);
};
hostPlugin.receiveResult = receive;
return () => {
if (hostPlugin.receiveResult === emitter) delete hostPlugin.receiveResult;
if (hostPlugin.receiveResult === receive) delete hostPlugin.receiveResult;
};
});
try {
Expand All @@ -468,6 +473,7 @@ function* handleResults(
function* handleErrors(
hostPlugin: BrowserHostPlugin,
workspaceLocation: WorkspaceLocation,
onTerminate: () => void,
): SagaIterator {
const errorChan = eventChannel(emitter => {
hostPlugin.receiveError = emitter;
Expand All @@ -479,6 +485,11 @@ function* handleErrors(
while (true) {
const error = yield take(errorChan);
yield put(actions.appendInterpreterError([toConductorSourceError(error)], workspaceLocation));
// Signal the REPL loop that evaluation has ended due to an error.
// We call onTerminate() here as a safety net: the runner should also
// send a terminal status (STOPPED/ERROR) which handleStatuses will catch,
// but if it doesn't (e.g. older evaluator build), this ensures we unblock.
onTerminate();
Comment on lines +488 to +492

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 | 🟠 Major | 🏗️ Heavy lift

Don’t terminate the whole conductor session on any error event.

public/evaluators/py-slang/PythonStepperEvaluator1.js:1’s qe.runChunk() catches user-code errors, sends them through sendError(...), and keeps the evaluator alive for subsequent chunks. Calling onTerminate() here means a single REPL error tears down the session, so later evalRepl inputs stop working until the editor is re-run.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts` around lines 488 - 492,
The helper currently calls onTerminate() whenever an evaluation error occurs
which forcefully ends the entire conductor session; instead, stop calling
onTerminate() on ordinary user-code errors so the evaluator stays alive for
subsequent REPL chunks—only allow termination when you detect a true
evaluator-level fatal condition. Locate the error-handling block where
onTerminate() is invoked (referencing onTerminate() in this file and the REPL
caller evalRepl) and remove or gate that call so that errors reported via
sendError()/qe.runChunk() are propagated back to handleStatuses without tearing
down the session; ensure any fatal/evaluator-crash paths still call
onTerminate() but normal user exceptions do not.

}
} finally {
if (yield cancelled()) {
Expand All @@ -490,6 +501,7 @@ function* handleErrors(
function* handleStatuses(
hostPlugin: BrowserHostPlugin,
workspaceLocation: WorkspaceLocation,
onTerminate: () => void,
): SagaIterator {
const statusChan = eventChannel<{ status: RunnerStatus; isActive: boolean }>(emitter => {
const onStatusUpdate = (status: RunnerStatus, isActive: boolean) =>
Expand All @@ -510,7 +522,8 @@ function* handleStatuses(
isActive && (status === RunnerStatus.STOPPED || status === RunnerStatus.ERROR);

if (isTerminalStatus) {
yield put(actions.beginInterruptExecution(workspaceLocation));
// Unblock the REPL loop via the shared termination signal.
onTerminate();
}
}
} finally {
Expand Down Expand Up @@ -551,35 +564,57 @@ export function* evalCodeConductorSaga(
{ files, consume: true },
);

// A one-shot Promise that resolves when the evaluator signals it has finished
// (either via a terminal status or via an error with no follow-up status).
// Using a Promise + eventChannel means the signal is completely decoupled from
// Redux action ordering, avoiding the race condition where beginInterruptExecution
// dispatched by evalEditorSaga is buffered and consumed by this loop before the
// evaluator even has a chance to report an error.
let resolveTerminated!: () => void;
const terminatedChan = eventChannel<true>(emitter => {
resolveTerminated = () => emitter(true);
return () => {};
});


// Begin evaluation
const stdoutTask = yield fork(handleStdout, hostPlugin, workspaceLocation);
const resultTask = yield fork(handleResults, hostPlugin, workspaceLocation);
const errorTask = yield fork(handleErrors, hostPlugin, workspaceLocation);
const statusTask = yield fork(handleStatuses, hostPlugin, workspaceLocation);
yield call([hostPlugin, 'startEvaluator'], entrypointFilePath);

// This exit logic of this while loop might be causing an unintended infinite loop in the REPL
while (true) {
const { stop } = yield race({
repl: take(actions.evalRepl.type),
stop: take(actions.beginInterruptExecution.type),
});
if (stop) break;
const code: string = yield select(
(state: OverallState) => state.workspaces[workspaceLocation].replValue,
);
yield put(actions.sendReplInputToOutput(code, workspaceLocation));
yield put(actions.clearReplInput(workspaceLocation));
yield call([hostPlugin, 'sendChunk'], code);
const errorTask = yield fork(handleErrors, hostPlugin, workspaceLocation, resolveTerminated);
const statusTask = yield fork(handleStatuses, hostPlugin, workspaceLocation, resolveTerminated);

try {
yield call([hostPlugin, 'startEvaluator'], entrypointFilePath);

// The REPL loop processes REPL inputs until the evaluator signals termination.
// Using a dedicated termination channel (not beginInterruptExecution) avoids the
// race condition where a pre-dispatched beginInterruptExecution from evalEditorSaga
// is buffered and consumed by this loop prematurely.
while (true) {
const { stop } = yield race({
repl: take(actions.evalRepl.type),
stop: take(terminatedChan),
});
if (stop) break;
const code: string = yield select(
(state: OverallState) => state.workspaces[workspaceLocation].replValue,
);
yield put(actions.sendReplInputToOutput(code, workspaceLocation));
yield put(actions.clearReplInput(workspaceLocation));
yield call([hostPlugin, 'sendChunk'], code);
}
} finally {
// Always clean up — runs on normal exit, error, or saga cancellation (takeLatest).
terminatedChan.close();
yield cancel(statusTask);
yield cancel(stdoutTask);
yield cancel(resultTask);
yield cancel(errorTask);
yield call([conduit, 'terminate']);

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.

critical

conduit.terminate() throws an error if the conduit is already terminated (due to the internal l() check). If this throws inside the finally block, it will interrupt the saga's cleanup phase, preventing subsequent actions like endInterruptExecution and setIsRunning(false) from being dispatched. This leaves the workspace in a permanently 'running' state. Wrapping it in a try-catch block ensures safe cleanup.

    try {
      yield call([conduit, 'terminate']);
    } catch (e) {
      console.warn('[conductor] failed to terminate conduit', e);
    }

yield put(actions.endInterruptExecution(workspaceLocation));
yield put(actions.setIsRunning(false, workspaceLocation));
console.log('[conductor] saga terminated');
}
yield cancel(statusTask);
yield call([conduit, 'terminate']);
yield cancel(stdoutTask);
yield cancel(resultTask);
yield cancel(errorTask);
//yield put(actions.debuggerReset(workspaceLocation));
yield put(actions.endInterruptExecution(workspaceLocation));
console.log('killed');
}

// Special module errors
Expand Down
6 changes: 4 additions & 2 deletions src/commons/sagas/WorkspaceSaga/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,10 @@ const WorkspaceSaga = combineSagaHandlers({
}
});
},
[WorkspaceActions.evalEditor.type]: ({ payload: { workspaceLocation } }) =>
evalEditorSaga(workspaceLocation),
[WorkspaceActions.evalEditor.type]: {
takeLatest: ({ payload: { workspaceLocation } }) => evalEditorSaga(workspaceLocation),
},
Comment on lines +139 to +141

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 | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== combineSagaHandlers / takeLatest implementation =="
rg -n -C4 '\bcombineSagaHandlers\b|\btakeLatest\b' src/commons/redux

echo
echo "== evalEditor / evalRepl action creators =="
rg -n -C2 '\bevalEditor:\s*\(|\bevalRepl:\s*\(' src/commons/workspace/WorkspaceActions.ts

echo
echo "== WorkspaceLocation definition =="
rg -n -C2 '\b(type|export type)\s+WorkspaceLocation\b' src/commons/workspace/WorkspaceTypes.ts

echo
echo "== evalEditor / evalRepl dispatch sites =="
rg -n -C2 '\b(WorkspaceActions\.)?(evalEditor|evalRepl)\s*\(' --type=ts --type=tsx src

Repository: source-academy/frontend

Length of output: 5532


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== combineSagaHandlers / takeLatest implementation =="
rg -n -C4 '\bcombineSagaHandlers\b|\btakeLatest\b' src/commons/redux

echo
echo "== WorkspaceActions action creators (workspaceLocation payload) =="
rg -n -C2 'evalEditor:\s*\(|evalRepl:\s*\(' src/commons/workspace/WorkspaceActions.ts

echo
echo "== WorkspaceLocation type definition =="
rg -n -C2 '\btype\s+WorkspaceLocation\b|\bexport\s+type\s+WorkspaceLocation\b' src/commons/workspace/WorkspaceTypes.ts

echo
echo "== Inspect WorkspaceSaga index.ts around lines 139-141 =="
nl -ba src/commons/sagas/WorkspaceSaga/index.ts | sed -n '120,170p'

echo
echo "== Inspect evalCode.ts around lines 594-604 =="
nl -ba src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts | sed -n '540,660p'

echo
echo "== Find dispatch sites for evalEditor/evalRepl =="
rg -n --hidden --glob='*.ts' --glob='*.tsx' '(WorkspaceActions\.)?(evalEditor|evalRepl)\s*\(' src | head -n 200

Repository: source-academy/frontend

Length of output: 5591


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== WorkspaceSaga/index.ts around requested lines =="
echo "--- src/commons/sagas/WorkspaceSaga/index.ts (130-155) ---"
sed -n '130,155p' src/commons/sagas/WorkspaceSaga/index.ts

echo
echo "== WorkspaceSaga/helpers/evalCode.ts around requested lines =="
echo "--- src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts (570-640) ---"
sed -n '570,640p' src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts

echo
echo "== Find definition/usages of take(actions.evalRepl.type / evalEditor Saga mapping) =="
rg -n --glob='*.ts' --glob='*.tsx' \
  "(take\\(actions\\.evalRepl\\.type\\)|actions\\.evalRepl\\.type|evalRepl\\.type)" \
  src/commons/sagas/WorkspaceSaga

echo
echo "== Find evalEditor/evalRepl dispatch sites =="
rg -n --glob='*.ts' --glob='*.tsx' \
  '\b(WorkspaceActions\.)?(evalEditor|evalRepl)\s*\(' src | head -n 200

Repository: source-academy/frontend

Length of output: 6064


Fix saga concurrency: key evalEditor/evalRepl off payload.workspaceLocation

src/commons/sagas/WorkspaceSaga/index.ts registers WorkspaceActions.evalEditor.type with takeLatest, and combineSagaHandlers wires takeLatest(actionName, ...) using only the action type (no payload-based filtering). Since WorkspaceActions.evalEditor(workspaceLocation) includes workspaceLocation (src/commons/workspace/WorkspaceActions.ts:90-91), this still cancels a previous evalEditor saga across all workspaces.

In src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts, the REPL loop does repl: take(actions.evalRepl.type) without checking workspaceLocation, then continues using the enclosing workspaceLocation for select/put/clear/sendChunk. That allows REPL inputs from one workspace to be consumed/processed by another workspace’s conductor session (wrong routing + clearing of the wrong workspace state).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/commons/sagas/WorkspaceSaga/index.ts` around lines 139 - 141, The sagas
currently use global takeLatest by action type which causes evalEditor/evalRepl
concurrency and cross-workspace routing problems; update the handler
registration and the REPL loop so handlers are keyed by workspaceLocation:
change the combineSagaHandlers/takeLatest wiring for
WorkspaceActions.evalEditor.type to scope by payload.workspaceLocation (so
evalEditorSaga(workspaceLocation) instances don't cancel across workspaces) and
in src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts replace the unconditional
repl: take(actions.evalRepl.type) with a filtered take (e.g., take(action =>
action.type === actions.evalRepl.type && action.payload?.workspaceLocation ===
workspaceLocation)) so select/put/clear/sendChunk operate only on the matching
workspaceLocation and REPL inputs are routed to the correct conductor session.


[WorkspaceActions.promptAutocomplete.type]: function* (action) {
const workspaceLocation = action.payload.workspaceLocation;
const {
Expand Down
Loading
Loading