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 10 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. 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.
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
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The evaluators and the stepper web-plugin bundle are built from their source repos (js-slang, py-slang, plugins) and deployed to GitHub Pages. The frontend references those URLs through the hosted language- and plugin-directories, whose flag defaults already point at source-academy.github.io. The local copies under public/evaluators, public/languages and public/plugins were only a turnkey local-test harness and must not live in the frontend repo. Untrack them and gitignore the dev-fixture paths so dev.sh can keep copying them locally without re-adding them to the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Fix absolute js-slang portal path to relative (breaks CI for other devs) - Fix corrupted pluginTabRegistry.ts type definition; add iconName validation - Wrap conduit.terminate() in try-catch to prevent stuck running state - Guard registerPluginTabIfPresent against stale conductor race condition - Add defensive checks in shim files for missing globalThis bootstrap vars - Extract CONDUCTOR_STEPPER_TAB_ID constant to avoid hardcoded string drift - Add flagConductorEnable side-effects to set local directory URLs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tor preload - Load Conductor web plugins dynamically and surface their tabs through a shared SideContentManager (ITabService): the host factory-imports the bundle, registers it with the tab service, and SideContentProvider renders the service's tabs. Replaces the bespoke pluginTabRegistry/makePluginTabFrom. Aligns with the upstream dynamic-tab mechanism (frontend source-academy#3977 + plugins source-academy#25); widens SideContentType to SideContentTabId so plugin tabs can use free-form string ids. - Fix the conductor run-button hang: the eval saga now always terminates — a OneShot result triggers cleanup, and a terminal STATUS or a timeout unblocks it, regardless of the worker's conductor version. - Fix evaluator selection: selecting an evaluator (e.g. Stepper) now preloads it, so Run uses the chosen evaluator and its web plugin/tab loads. Previously only language changes preloaded, so picking Stepper never took effect. - Dev hardening: unregister stale service workers + clear caches in dev, and serve a real 404 (not index.html) for missing /evaluators|/plugins|/languages files, so a bad path is a loud error instead of a silent worker parse failure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2a97767 to
17de643
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the local portal: references with the published artifacts so the dependencies resolve in CI instead of failing with "Manifest not found": - @sourceacademy/conductor -> ^0.5.0 (npm) - @sourceacademy/plugin-directory -> git tag 0.0.3 NOTE: plugin-directory 0.0.3 must be tagged/released (from plugin-directory source-academy#18) before this resolves; update the version here if the release differs. yarn.lock must be regenerated once that ref exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…conductor's tab Make the Stepper (and any Conductor web plugin) tab actually appear and stick when the conductor framework is enabled. - Migrate conductorEvaluatorCache off the deleted pluginTabRegistry (which broke the build) to the SideContentManager/ITabService tab mechanism. - Add importExternalWebPlugin: web plugin bundles are emitted as `default` require-wrappers `(require) => moduleExports`, but Conductor's importExternalPlugin only reads a named `.plugin` export, so registration threw and no tab showed. The new loader detects the require-wrapper, injects the host's React/jsx-runtime/Blueprint via a require shim, and registers the class with the host (still supports a plain `.plugin` export). - Add DeferredConductorTabService: conductors are preloaded and a warm spare is created after every Run; the spare's web plugin re-registered the same-id tab with an empty body, clobbering the running conductor's populated tab (steps flashed, then reverted to the welcome view). Each conductor now buffers its tabs and only the active (selected/running) conductor surfaces them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This PR integrates the dynamic plugin loading framework for the Conductor stepper plugin. Key changes include:
BrowserHostPlugin.conductorSharedDeps).public/shims/*so dynamically-imported web-plugin bundles share the frontend's single React tree and Blueprint styling.A note on evaluators and plugins (re: review feedback)
The evaluator bundles and the stepper web-plugin bundle are not committed to the frontend. They are built from their source repos (js-slang, py-slang, the plugins monorepo) and deployed to GitHub Pages, then referenced via the hosted language- and plugin-directories. The relevant feature flags already default to those hosted directories:
directory.language.url→https://source-academy.github.io/language-directory/directory.jsondirectory.plugin.url→https://source-academy.github.io/plugin-directory/directory.jsonso the frontend never needs the files locally. The local copies under
public/evaluators,public/languagesandpublic/pluginswere only a turnkey local-test harness; they have been untracked and gitignored (dev.shstill copies them for local testing without re-adding them to the repo).