feat(stepper): integrate Conductor plugin tabs and dynamic loading of web plugins#3959
feat(stepper): integrate Conductor plugin tabs and dynamic loading of web plugins#3959Shrey5132 wants to merge 1 commit into
Conversation
Summary by CodeRabbitRelease Notes
WalkthroughThis PR integrates Conductor web plugins into the Source Academy frontend, adding a Python step-by-step evaluator and stepper visualization plugin. It establishes shared dependency injection for dynamically loaded plugins, refactors REPL execution termination, and wires plugin-contributed tabs into the Playground UI. ChangesConductor Web Plugin Integration and Python Stepper
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
There was a problem hiding this comment.
Code Review
This pull request integrates the Conductor framework into the frontend, allowing dynamically-loaded web plugins (like the Stepper) to contribute side-content tabs. It introduces a plugin tab registry, dependency shims, and updates the Playground and evaluation sagas to support Conductor. Key feedback includes wrapping conduit.terminate() in a try-catch block to ensure safe cleanup, replacing a hardcoded absolute path in package.json with a relative one, preventing a race condition and a potential TypeError in loadWebPlugin, and correcting an invalid CSS value false for overflow-x in _workspace.scss.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| yield cancel(stdoutTask); | ||
| yield cancel(resultTask); | ||
| yield cancel(errorTask); | ||
| yield call([conduit, 'terminate']); |
There was a problem hiding this comment.
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);
}| "@types/estree": "1.0.9", | ||
| "vite": "^8.0.0" | ||
| "vite": "^8.0.0", | ||
| "js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang" |
There was a problem hiding this comment.
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.
| "js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang" | |
| "js-slang": "portal:../js-slang" |
| const plugin = await hostPlugin.importAndRegisterExternalPlugin(url); | ||
| registerPluginTabIfPresent(plugin); |
There was a problem hiding this comment.
There is a potential race condition here: if the conductor is terminated/cleared while loadWebPlugin is still asynchronously fetching or registering the plugin, registerPluginTabIfPresent will still be called. This can cause a tab from a previous, dead session to unexpectedly appear in the UI. Checking if hostPlugin is still the active one in preparedConductor prevents this.
const plugin = await hostPlugin.importAndRegisterExternalPlugin(url);
if (preparedConductor?.hostPlugin === hostPlugin) {
registerPluginTabIfPresent(plugin);
}| color: $code-color-result; | ||
| text-align: justify; | ||
| overflow-x: auto; | ||
| overflow-x: false; |
| */ | ||
| async function resolveWebPluginUrl(pluginId: string): Promise<string | undefined> { | ||
| for (let attempt = 0; attempt < 50; attempt++) { | ||
| const url = store.getState().pluginDirectory.pluginMap[pluginId]?.resolutions?.[PluginType.WEB]; |
There was a problem hiding this comment.
If pluginMap is undefined (e.g., before the directory has finished loading), accessing pluginMap[pluginId] will throw a TypeError. Using optional chaining ?.[pluginId] ensures safe access.
| const url = store.getState().pluginDirectory.pluginMap[pluginId]?.resolutions?.[PluginType.WEB]; | |
| const url = store.getState().pluginDirectory.pluginMap?.[pluginId]?.resolutions?.[PluginType.WEB]; |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/pages/playground/Playground.tsx (1)
979-983: ⚡ Quick winDeduplicate the stepper tab ID constant across conductor checks.
Line 982 and Line 1052 hardcode
'stepper'. Since this ID is also part of cross-file contracts (plugin directory + tab panel styling), centralizing it avoids silent drift.♻️ Suggested refactor
+const CONDUCTOR_STEPPER_TAB_ID = 'stepper'; ... - (conductorEnabled && (selectedTab as string) === 'stepper'), + (conductorEnabled && (selectedTab as string) === CONDUCTOR_STEPPER_TAB_ID), ... - !(conductorEnabled && (selectedTab as string) === 'stepper'), + !(conductorEnabled && (selectedTab as string) === CONDUCTOR_STEPPER_TAB_ID),Also applies to: 1049-1052
🤖 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/pages/playground/Playground.tsx` around lines 979 - 983, The code hardcodes the stepper tab id string in multiple places; create a single exported constant (e.g., STEPPER_TAB_ID) and use it instead of the literal 'stepper' wherever the conductor tab is checked—replace the comparisons using (selectedTab as string) === 'stepper' and any other occurrence in Playground.tsx (and import from the plugin/contracts module if there is a shared constants file) so the conductorEnabled check and the tab panel styling/contract all reference the same identifier.src/commons/sagas/helpers/conductorEvaluatorCache.ts (1)
73-80: ⚡ Quick winDirect Redux store access breaks saga pattern and testability.
resolveWebPluginUrldirectly callsstore.getState()from an async function. This bypasses Redux Saga's effect system, making the code harder to test and breaking the saga abstraction. Consider refactoring to use sagaselect()effects or passing the state snapshot as a parameter.♻️ Refactor to use saga effects
Option 1: Make it a generator and use
selecteffect:-async function resolveWebPluginUrl(pluginId: string): Promise<string | undefined> { +function* resolveWebPluginUrlSaga(pluginId: string): SagaIterator<string | undefined> { for (let attempt = 0; attempt < 50; attempt++) { - const url = store.getState().pluginDirectory.pluginMap[pluginId]?.resolutions?.[PluginType.WEB]; + const pluginMap: any = yield select((state: OverallState) => state.pluginDirectory.pluginMap); + const url = pluginMap[pluginId]?.resolutions?.[PluginType.WEB]; if (url) return url; - await new Promise(resolve => setTimeout(resolve, 100)); + yield delay(100); } return undefined; }Then update
loadWebPluginto be a saga as well and call it withyield call(...).🤖 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/helpers/conductorEvaluatorCache.ts` around lines 73 - 80, resolveWebPluginUrl currently reads state via store.getState() which breaks the saga effect model; change it to not access store directly by either (A) making resolveWebPluginUrl a generator saga that uses the redux-saga select effect to read pluginDirectory.pluginMap (and update callers such as loadWebPlugin to call it with yield call or yield*), or (B) convert resolveWebPluginUrl into a pure async helper that accepts the pluginMap (or a snapshot of pluginDirectory) as a parameter and have the saga caller pass state to it via yield select; update all callers (e.g., loadWebPlugin) accordingly so no function directly calls store.getState().src/features/conductor/makePluginTabFrom.tsx (1)
20-20: ⚖️ Poor tradeoffFree-form plugin IDs cast to
SideContentTypeenum.Plugin IDs are intentionally free-form strings but are cast to the
SideContentTypeenum. This works at runtime but breaks type safety—ifSideContentTypeis used elsewhere as a discriminated union key, TypeScript won't catch collisions. Consider either:
- Extending
SideContentTypeto include a plugin namespace (e.g.,PLUGIN_prefix), or- Refactoring the side-content system to accept
stringIDs instead of the enum.This is an architectural tradeoff; if the current approach is intentional, add a comment explaining the risk.
🤖 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/features/conductor/makePluginTabFrom.tsx` at line 20, The current cast "id: pluginTab.id as SideContentType" in makePluginTabFrom forces free-form pluginTab.id into the SideContentType enum and breaks type safety; either (A) expand the SideContentType enum to include plugin-specific variants (e.g., add PLUGIN_<name> entries or a PLUGIN_PREFIX strategy and map pluginTab.id to that) or (B) change the tab id typing so makePluginTabFrom produces a string id instead of SideContentType (update the Tab/SideContent consumer types to accept string), or if you intentionally accept the risk add a clear inline comment above the cast explaining why the unsafe cast is necessary and the potential collision risk. Ensure you update references to SideContentType or tab id usages so type signatures remain consistent when choosing A or B.src/features/conductor/pluginTabRegistry.ts (1)
65-68: ⚡ Quick winMisleading comment about reference stability.
The comment claims the returned array has a "stable reference between changes," but line 36 creates a new array on every registration (
pluginTabs = [...pluginTabs, tab]), so the reference changes with each registration. Consider clarifying or removing this phrase.📝 Suggested clarification
-/** The current snapshot of registered plugin tabs (stable reference between changes). */ +/** Returns the current snapshot of registered plugin tabs. */ export function getPluginTabs(): ConductorPluginTab[] { return pluginTabs; }🤖 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/features/conductor/pluginTabRegistry.ts` around lines 65 - 68, The comment on getPluginTabs incorrectly claims a "stable reference between changes" — the registration logic reassigns pluginTabs using the spread pattern (pluginTabs = [...pluginTabs, tab]) so the array reference changes on registration; update the docstring for getPluginTabs (and any similar comments) to remove or reword the "stable reference" phrase (e.g., "Returns the current snapshot of registered plugin tabs") or alternatively, if you need a stable reference, change the registration logic to mutate the existing pluginTabs array instead of reassigning it; reference the getPluginTabs function and the pluginTabs reassignment in the registration code when making the change.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@package.json`:
- 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.
- Around line 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.
In `@public/shims/react.mjs`:
- 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.
In `@src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts`:
- Around line 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.
In `@src/commons/sagas/WorkspaceSaga/index.ts`:
- Around line 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.
In `@src/features/conductor/makePluginTabFrom.tsx`:
- Line 17: The code unsafely casts pluginTab.iconName to Blueprint's IconName in
makePluginTabFrom, which can silently fail; replace the direct cast with a
runtime validation: check pluginTab.iconName against Blueprint's allowed icon
names (or a maintained whitelist) using a helper like isValidIconName and only
assign it as IconName if valid, otherwise set a safe default icon or log a
warning; update the assignment that currently reads "iconName:
pluginTab.iconName as IconName" to perform this validation and fallback before
returning the tab object.
In `@src/features/conductor/pluginTabRegistry.ts`:
- Around line 44-56: registerPluginTabIfPresent currently checks tab.id,
tab.label, and tab.Component but omits validating tab.iconName so a plugin
missing iconName can slip through and later fail; update
registerPluginTabIfPresent to also verify that (plugin as TabContributingPlugin
| null)?.tab?.iconName exists and is a string before calling registerPluginTab,
referencing the TabContributingPlugin/ConductorPluginTab shape and the
tab.iconName property to ensure full runtime validation.
- Line 18: The file references React.ComponentType but React is not imported;
add an import to satisfy the type reference (either add "import React from
'react'" at the top or, preferably for types-only usage, add "import type {
ComponentType } from 'react'" and replace React.ComponentType with
ComponentType) so that the symbol React.ComponentType (or ComponentType) used in
pluginTabRegistry.ts resolves and TypeScript compiles.
In `@src/styles/_workspace.scss`:
- Line 271: The rule `overflow-x: false;` is invalid and should be replaced with
a valid CSS value; locate the `overflow-x: false;` occurrence in _workspace.scss
and change it to the intended behavior—for example use `overflow-x: hidden;` to
suppress horizontal scrolling or `overflow-x: auto;`/`overflow-x: visible;` if
you want scrolling/visible overflow instead—so the browser honors the setting.
---
Nitpick comments:
In `@src/commons/sagas/helpers/conductorEvaluatorCache.ts`:
- Around line 73-80: resolveWebPluginUrl currently reads state via
store.getState() which breaks the saga effect model; change it to not access
store directly by either (A) making resolveWebPluginUrl a generator saga that
uses the redux-saga select effect to read pluginDirectory.pluginMap (and update
callers such as loadWebPlugin to call it with yield call or yield*), or (B)
convert resolveWebPluginUrl into a pure async helper that accepts the pluginMap
(or a snapshot of pluginDirectory) as a parameter and have the saga caller pass
state to it via yield select; update all callers (e.g., loadWebPlugin)
accordingly so no function directly calls store.getState().
In `@src/features/conductor/makePluginTabFrom.tsx`:
- Line 20: The current cast "id: pluginTab.id as SideContentType" in
makePluginTabFrom forces free-form pluginTab.id into the SideContentType enum
and breaks type safety; either (A) expand the SideContentType enum to include
plugin-specific variants (e.g., add PLUGIN_<name> entries or a PLUGIN_PREFIX
strategy and map pluginTab.id to that) or (B) change the tab id typing so
makePluginTabFrom produces a string id instead of SideContentType (update the
Tab/SideContent consumer types to accept string), or if you intentionally accept
the risk add a clear inline comment above the cast explaining why the unsafe
cast is necessary and the potential collision risk. Ensure you update references
to SideContentType or tab id usages so type signatures remain consistent when
choosing A or B.
In `@src/features/conductor/pluginTabRegistry.ts`:
- Around line 65-68: The comment on getPluginTabs incorrectly claims a "stable
reference between changes" — the registration logic reassigns pluginTabs using
the spread pattern (pluginTabs = [...pluginTabs, tab]) so the array reference
changes on registration; update the docstring for getPluginTabs (and any similar
comments) to remove or reword the "stable reference" phrase (e.g., "Returns the
current snapshot of registered plugin tabs") or alternatively, if you need a
stable reference, change the registration logic to mutate the existing
pluginTabs array instead of reassigning it; reference the getPluginTabs function
and the pluginTabs reassignment in the registration code when making the change.
In `@src/pages/playground/Playground.tsx`:
- Around line 979-983: The code hardcodes the stepper tab id string in multiple
places; create a single exported constant (e.g., STEPPER_TAB_ID) and use it
instead of the literal 'stepper' wherever the conductor tab is checked—replace
the comparisons using (selectedTab as string) === 'stepper' and any other
occurrence in Playground.tsx (and import from the plugin/contracts module if
there is a shared constants file) so the conductorEnabled check and the tab
panel styling/contract all reference the same identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: faf1bb8e-b4b9-43da-8bd7-35e3da40e53f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
package.jsonpublic/evaluators/js-slang/SourceStepperEvaluator1.jspublic/evaluators/js-slang/SourceStepperEvaluator2.jspublic/evaluators/py-slang/PyCseEvaluator1.cjspublic/evaluators/py-slang/PyCseEvaluator1.jspublic/evaluators/py-slang/PyCseEvaluator2.cjspublic/evaluators/py-slang/PyCseEvaluator2.jspublic/evaluators/py-slang/PyCseEvaluator3.cjspublic/evaluators/py-slang/PyCseEvaluator3.jspublic/evaluators/py-slang/PyCseEvaluator4.cjspublic/evaluators/py-slang/PyCseEvaluator4.jspublic/evaluators/py-slang/PySvmlEvaluator.cjspublic/evaluators/py-slang/PySvmlEvaluator.jspublic/evaluators/py-slang/PySvmlSinterEvaluator.cjspublic/evaluators/py-slang/PySvmlSinterEvaluator.jspublic/evaluators/py-slang/PyWasmEvaluator.cjspublic/evaluators/py-slang/PyWasmEvaluator.jspublic/evaluators/py-slang/PythonStepperEvaluator1.cjspublic/evaluators/py-slang/PythonStepperEvaluator1.jspublic/index.htmlpublic/languages/directory.jsonpublic/plugins/directory.jsonpublic/plugins/stepper/index.mjspublic/shims/blueprintjs-core.mjspublic/shims/react-jsx-runtime.mjspublic/shims/react.mjssrc/bootstrap/conductorSharedDeps.tssrc/commons/sagas/WorkspaceSaga/helpers/evalCode.tssrc/commons/sagas/WorkspaceSaga/index.tssrc/commons/sagas/helpers/conductorEvaluatorCache.tssrc/features/conductor/makePluginTabFrom.tsxsrc/features/conductor/pluginTabRegistry.tssrc/index.tsxsrc/pages/playground/Playground.tsxsrc/styles/StepperPopover.scsssrc/styles/_workspace.scss
| "@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", |
There was a problem hiding this comment.
🧩 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-runRepository: 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 || trueRepository: 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
fiRepository: 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 || trueRepository: 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 || trueRepository: 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]);
}
NODERepository: source-academy/frontend
Length of output: 8441
Fix CI to satisfy portal: dependency targets (or update the portal paths).
package.jsonpoints to@sourceacademy/conductor(portal:../conductor) and@sourceacademy/plugin-directory(portal:../plugin-directory), which requireconductor/andplugin-directory/to exist as sibling directories of the frontend checkout.- CI (
.github/workflows/ci.ymlandbuild-development.yml) only checks out this single repo and runsyarn install --immutable, without fetching/creating those sibling directories—so theportal:targets will be missing in CI. - In the current checkout,
../conductorand../plugin-directorydo 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
| "@types/estree": "1.0.9", | ||
| "vite": "^8.0.0" | ||
| "vite": "^8.0.0", | ||
| "js-slang": "portal:/Users/shreyjain/Downloads/source-academy/js-slang" |
There was a problem hiding this comment.
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.
| "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.
| @@ -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__; | |||
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
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.
| [WorkspaceActions.evalEditor.type]: { | ||
| takeLatest: ({ payload: { workspaceLocation } }) => evalEditorSaga(workspaceLocation), | ||
| }, |
There was a problem hiding this comment.
🧩 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 srcRepository: 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 200Repository: 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 200Repository: 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.
| const { Component } = pluginTab; | ||
| return { | ||
| label: pluginTab.label, | ||
| iconName: pluginTab.iconName as IconName, |
There was a problem hiding this comment.
Unsafe cast to IconName may cause silent failures.
The plugin's iconName is cast to BlueprintJS's IconName type without validation. If a plugin provides an invalid icon name, Blueprint will fail to render the icon (typically falling back silently). Consider validating against Blueprint's icon set or documenting this requirement for plugin authors.
🤖 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/features/conductor/makePluginTabFrom.tsx` at line 17, The code unsafely
casts pluginTab.iconName to Blueprint's IconName in makePluginTabFrom, which can
silently fail; replace the direct cast with a runtime validation: check
pluginTab.iconName against Blueprint's allowed icon names (or a maintained
whitelist) using a helper like isValidIconName and only assign it as IconName if
valid, otherwise set a safe default icon or log a warning; update the assignment
that currently reads "iconName: pluginTab.iconName as IconName" to perform this
validation and fallback before returning the tab object.
| /** Blueprint icon name for the tab. */ | ||
| iconName: string; | ||
| /** The React component rendered as the tab body. Self-contained; takes no props. */ | ||
| Component: React.ComponentType; |
There was a problem hiding this comment.
Missing React import for React.ComponentType.
Line 18 references React.ComponentType but React is not imported at the top of the file. This will cause a TypeScript compilation error.
🔧 Proposed fix
/**
* A generic registry of side-content tabs contributed by Conductor **web plugins**.
*
* Conductor itself has no UI mechanism, so when the host loads a web plugin that exposes a `tab`
* (see {`@link` ConductorPluginTab}), we record it here. The Playground renders these tabs via
* `useSyncExternalStore`. This module is deliberately plugin-agnostic — it knows nothing about the
* stepper or any specific plugin, so adding another web plugin needs no changes here.
*/
+import type React from 'react';
+
export type ConductorPluginTab = {📝 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.
| Component: React.ComponentType; | |
| /** | |
| * A generic registry of side-content tabs contributed by Conductor **web plugins**. | |
| * | |
| * Conductor itself has no UI mechanism, so when the host loads a web plugin that exposes a `tab` | |
| * (see {`@link` ConductorPluginTab}), we record it here. The Playground renders these tabs via | |
| * `useSyncExternalStore`. This module is deliberately plugin-agnostic — it knows nothing about the | |
| * stepper or any specific plugin, so adding another web plugin needs no changes here. | |
| */ | |
| import type React from 'react'; | |
| export type ConductorPluginTab = { | |
| Component: React.ComponentType; |
🤖 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/features/conductor/pluginTabRegistry.ts` at line 18, The file references
React.ComponentType but React is not imported; add an import to satisfy the type
reference (either add "import React from 'react'" at the top or, preferably for
types-only usage, add "import type { ComponentType } from 'react'" and replace
React.ComponentType with ComponentType) so that the symbol React.ComponentType
(or ComponentType) used in pluginTabRegistry.ts resolves and TypeScript
compiles.
| export function registerPluginTabIfPresent(plugin: unknown): boolean { | ||
| const tab = (plugin as TabContributingPlugin | null)?.tab; | ||
| if ( | ||
| tab && | ||
| typeof tab.id === 'string' && | ||
| typeof tab.label === 'string' && | ||
| typeof tab.Component === 'function' | ||
| ) { | ||
| registerPluginTab(tab); | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Incomplete runtime validation for plugin tab structure.
The function validates id, label, and Component but does not check iconName, even though ConductorPluginTab requires it. A plugin providing a tab without iconName would pass validation here but fail later when the tab is rendered.
🛡️ Add validation for iconName
export function registerPluginTabIfPresent(plugin: unknown): boolean {
const tab = (plugin as TabContributingPlugin | null)?.tab;
if (
tab &&
typeof tab.id === 'string' &&
typeof tab.label === 'string' &&
+ typeof tab.iconName === 'string' &&
typeof tab.Component === 'function'
) {
registerPluginTab(tab);
return true;
}
return false;
}🤖 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/features/conductor/pluginTabRegistry.ts` around lines 44 - 56,
registerPluginTabIfPresent currently checks tab.id, tab.label, and tab.Component
but omits validating tab.iconName so a plugin missing iconName can slip through
and later fail; update registerPluginTabIfPresent to also verify that (plugin as
TabContributingPlugin | null)?.tab?.iconName exists and is a string before
calling registerPluginTab, referencing the
TabContributingPlugin/ConductorPluginTab shape and the tab.iconName property to
ensure full runtime validation.
| color: $code-color-result; | ||
| text-align: justify; | ||
| overflow-x: auto; | ||
| overflow-x: false; |
There was a problem hiding this comment.
Use a valid CSS value for overflow-x on Line 271.
overflow-x: false; is invalid and gets ignored, which can reintroduce unintended horizontal overflow behavior.
🐛 Suggested fix
- overflow-x: false;
+ overflow-x: auto;📝 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.
| overflow-x: false; | |
| overflow-x: auto; |
🤖 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/styles/_workspace.scss` at line 271, The rule `overflow-x: false;` is
invalid and should be replaced with a valid CSS value; locate the `overflow-x:
false;` occurrence in _workspace.scss and change it to the intended behavior—for
example use `overflow-x: hidden;` to suppress horizontal scrolling or
`overflow-x: auto;`/`overflow-x: visible;` if you want scrolling/visible
overflow instead—so the browser honors the setting.
This PR integrates the dynamic plugin loading framework for the Conductor stepper plugin. Key changes include:
BrowserHostPlugin.conductorSharedDeps).public/directory.