Skip to content

feat(stepper): integrate Conductor plugin tabs and dynamic loading of web plugins#3959

Open
Shrey5132 wants to merge 1 commit into
source-academy:masterfrom
Shrey5132:feat/conductor-stepper
Open

feat(stepper): integrate Conductor plugin tabs and dynamic loading of web plugins#3959
Shrey5132 wants to merge 1 commit into
source-academy:masterfrom
Shrey5132:feat/conductor-stepper

Conversation

@Shrey5132

Copy link
Copy Markdown
Contributor

This PR integrates the dynamic plugin loading framework for the Conductor stepper plugin. Key changes include:

  • Importing and registering external web plugins dynamically using BrowserHostPlugin.
  • Exposing shared dependencies globally via a bootstrap script (conductorSharedDeps).
  • Injecting custom stepper plugin tabs into the SideContent of the Playground when Conductor is enabled.
  • Hiding legacy stepper tabs/resizing when the Conductor stepper is active to match legacy behaviors.
  • Bundling local web plugins and evaluators under the public/ directory.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

Release Notes

  • New Features

    • Added Python-like language evaluation support
    • Introduced Stepper plugin with step-by-step execution visualization, including interactive step navigation and detailed explanation panels
  • Refactor

    • Integrated Conductor framework for dynamic plugin management and improved evaluation handling

Walkthrough

This 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.

Changes

Conductor Web Plugin Integration and Python Stepper

Layer / File(s) Summary
Workspace and dependency setup
package.json
Portal references for local Conductor and plugin-directory packages; js-slang resolution override added.
Shared dependency injection bootstrap
src/bootstrap/conductorSharedDeps.ts, public/shims/*, public/index.html, src/index.tsx
Exposes host React, JSX runtime, and BlueprintJS on globalThis; import-map and shims enable dynamically loaded plugins to reuse host instances; early bootstrap before plugin loading.
Plugin tab registry and conversion
src/features/conductor/pluginTabRegistry.ts, src/features/conductor/makePluginTabFrom.tsx
In-memory registry with listener-based notifications for plugin tabs; factory converts plugin descriptors to SideContentTab objects with deduplication and runtime shape validation.
Dynamic plugin loading in conductor
src/commons/sagas/helpers/conductorEvaluatorCache.ts
Resolves plugin web URLs from directory, imports external bundles, registers contributed tabs, and clears tabs on conductor cleanup.
REPL execution termination refactor
src/commons/sagas/WorkspaceSaga/index.ts, src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts
Switches to dedicated termination event channel instead of beginInterruptExecution; error and status handlers invoke termination callback; filters undefined evaluator results.
Language and evaluator configuration
public/languages/directory.json, public/plugins/directory.json
Defines three language sources (source-1, source-2, python-1) with evaluators; registers stepper plugin with web resolution path.
Python stepper evaluator
public/evaluators/py-slang/PythonStepperEvaluator1.js, public/evaluators/py-slang/PythonStepperEvaluator1.cjs
Self-contained bundled evaluator with lexer/parser, AST-based step reduction engine, stepper plugin integration for recording steps, and conductor error reporting.
Stepper plugin React UI
public/plugins/stepper/index.mjs
Renders step-by-step evaluation visualization with slider navigation, explanation panel, syntax-highlighted expression renderer with AST support, keyboard controls, and hover popovers.
Playground UI integration and styling
src/pages/playground/Playground.tsx, src/styles/StepperPopover.scss, src/styles/_workspace.scss
Conditionally displays plugin tabs when Conductor enabled; hides legacy SubstVisualizer and adjusts REPL/resizing when stepper active; adds CSS for stepper display and tab panel styling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description clearly summarizes the key changes and objectives, but lacks the structured format required by the template (Type of change, How to test, Checklist sections). Add structured sections: select applicable Type of change, describe testing steps/configuration, and complete the checklist with testing and documentation status.
Docstring Coverage ⚠️ Warning Docstring coverage is 8.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main objective: integrating Conductor plugin tabs and dynamic web plugin loading, which is central to all changes in the PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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']);

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);
    }

Comment thread package.json
"@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"

Comment on lines +96 to +97
const plugin = await hostPlugin.importAndRegisterExternalPlugin(url);
registerPluginTabIfPresent(plugin);

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

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;

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.

medium

false is not a valid CSS value for overflow-x. Valid values include visible, hidden, scroll, and auto. If the goal is to disable horizontal scrolling, please use hidden.

Suggested change
overflow-x: false;
overflow-x: hidden;

*/
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];

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.

medium

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.

Suggested change
const url = store.getState().pluginDirectory.pluginMap[pluginId]?.resolutions?.[PluginType.WEB];
const url = store.getState().pluginDirectory.pluginMap?.[pluginId]?.resolutions?.[PluginType.WEB];

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Nitpick comments (4)
src/pages/playground/Playground.tsx (1)

979-983: ⚡ Quick win

Deduplicate 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 win

Direct Redux store access breaks saga pattern and testability.

resolveWebPluginUrl directly calls store.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 saga select() effects or passing the state snapshot as a parameter.

♻️ Refactor to use saga effects

Option 1: Make it a generator and use select effect:

-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 loadWebPlugin to be a saga as well and call it with yield 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 tradeoff

Free-form plugin IDs cast to SideContentType enum.

Plugin IDs are intentionally free-form strings but are cast to the SideContentType enum. This works at runtime but breaks type safety—if SideContentType is used elsewhere as a discriminated union key, TypeScript won't catch collisions. Consider either:

  1. Extending SideContentType to include a plugin namespace (e.g., PLUGIN_ prefix), or
  2. Refactoring the side-content system to accept string IDs 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 win

Misleading 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5e2b146 and 3b27216.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (36)
  • package.json
  • public/evaluators/js-slang/SourceStepperEvaluator1.js
  • public/evaluators/js-slang/SourceStepperEvaluator2.js
  • public/evaluators/py-slang/PyCseEvaluator1.cjs
  • public/evaluators/py-slang/PyCseEvaluator1.js
  • public/evaluators/py-slang/PyCseEvaluator2.cjs
  • public/evaluators/py-slang/PyCseEvaluator2.js
  • public/evaluators/py-slang/PyCseEvaluator3.cjs
  • public/evaluators/py-slang/PyCseEvaluator3.js
  • public/evaluators/py-slang/PyCseEvaluator4.cjs
  • public/evaluators/py-slang/PyCseEvaluator4.js
  • public/evaluators/py-slang/PySvmlEvaluator.cjs
  • public/evaluators/py-slang/PySvmlEvaluator.js
  • public/evaluators/py-slang/PySvmlSinterEvaluator.cjs
  • public/evaluators/py-slang/PySvmlSinterEvaluator.js
  • public/evaluators/py-slang/PyWasmEvaluator.cjs
  • public/evaluators/py-slang/PyWasmEvaluator.js
  • public/evaluators/py-slang/PythonStepperEvaluator1.cjs
  • public/evaluators/py-slang/PythonStepperEvaluator1.js
  • public/index.html
  • public/languages/directory.json
  • public/plugins/directory.json
  • public/plugins/stepper/index.mjs
  • public/shims/blueprintjs-core.mjs
  • public/shims/react-jsx-runtime.mjs
  • public/shims/react.mjs
  • src/bootstrap/conductorSharedDeps.ts
  • src/commons/sagas/WorkspaceSaga/helpers/evalCode.ts
  • src/commons/sagas/WorkspaceSaga/index.ts
  • src/commons/sagas/helpers/conductorEvaluatorCache.ts
  • src/features/conductor/makePluginTabFrom.tsx
  • src/features/conductor/pluginTabRegistry.ts
  • src/index.tsx
  • src/pages/playground/Playground.tsx
  • src/styles/StepperPopover.scss
  • src/styles/_workspace.scss

Comment thread package.json
Comment on lines +49 to +51
"@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",

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

Comment thread package.json
"@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.

⚠️ 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.

Comment thread public/shims/react.mjs
@@ -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.

Comment on lines +488 to +492
// 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();

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.

Comment on lines +139 to +141
[WorkspaceActions.evalEditor.type]: {
takeLatest: ({ payload: { workspaceLocation } }) => evalEditorSaga(workspaceLocation),
},

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.

const { Component } = pluginTab;
return {
label: pluginTab.label,
iconName: pluginTab.iconName as IconName,

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 | 🟡 Minor | ⚡ Quick win

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;

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

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.

Suggested change
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.

Comment on lines +44 to +56
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;
}

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 | 🟡 Minor | ⚡ Quick win

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;

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 | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant