Ev3 new support - #4025
Conversation
Introduces a new remoteExecutionConductor feature that replaces the js-slang compilation step in the existing EV3 remote execution flow with a conductor-based pipeline using py-slang's EV3Engine. New files: - src/features/remoteExecutionConductor/flagConductorEv3Enable.ts: feature flag and selector to gate the new pipeline - src/features/remoteExecutionConductor/RemoteExecutionConductorActions.ts: conductor-specific redux actions for connect, disconnect, and run - src/features/remoteExecutionConductor/RemoteExecutionConductorSaga.ts: saga handling the conductor EV3 run flow, reusing existing SlingClient connection infrastructure from RemoteExecutionSaga tailored for the Conductor framework - src/features/remoteExecutionConductor/createEv3Conductor.ts: creates a conductor Conduit with a Web Worker loading ev3-pyslang.js, wires receiveResult to forward compiled SVML to the EV3 via SlingClient.sendRun() Modified files: - src/commons/utils/ActionsHelper.ts: registered RemoteExecutionConductorActions - src/commons/sagas/MainSaga.ts: forked RemoteExecutionConductorSaga alongside the legacy RemoteExecutionSaga - src/commons/sagas/WorkspaceSaga/helpers/evalEditorSaga.ts: added flag check to dispatch remoteExecConductorRun vs remoteExecRun based on conductor.ev3.enable feature flag
Added remoteExecutionConductor to support the new Ev3 remote runner under the conductor framework
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a conductor-based EV3 remote execution pipeline with a browser worker evaluator, typed messaging, device-event bridging, Redux actions and sagas, feature-flag routing, and a VS Code references setting. ChangesEV3 conductor execution
Editor configuration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Conductor-based EV3 execution pipeline using py-slang, adding the necessary actions, sagas, types, and a web plugin, integrated via a new feature flag. Key feedback points out a critical compilation error caused by an accidental import path replacement in evalEditor.ts, a potential runtime ReferenceError from using Node's Buffer in a browser environment, and a potential stale reference issue when reusing the active conductor across different client sessions.
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.
| const plugin = conduit.registerPlugin(Ev3WebPlugin); | ||
|
|
||
| plugin.onResult = (svml: string) => { | ||
| client.sendRun(Buffer.from(svml)); |
There was a problem hiding this comment.
Using Buffer.from in a browser environment can lead to a runtime ReferenceError: Buffer is not defined because Buffer is a Node.js global and is not polyfilled by default in modern bundlers. Consider using the standard Web API TextEncoder instead.
| client.sendRun(Buffer.from(svml)); | |
| client.sendRun(new TextEncoder().encode(svml)); |
There was a problem hiding this comment.
Necessary due to the need to decode the svml, and that sendRun needs a Buffer argument
| if (!activeConductor) { | ||
| activeConductor = yield call(createEv3Conductor, session.connection.client); | ||
| } |
There was a problem hiding this comment.
Reusing activeConductor across different runs without checking if the session.connection.client has changed can lead to stale references. If the connection is re-established or a different device is connected, the active conductor will still use the old client instance passed during its creation. Consider recreating the conductor if the client instance changes.
Modified the remote Conductor Tab to support peripheral usage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/features/remoteExecutionConductor/createEv3Conductor.ts (1)
33-36: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueWrap
client.sendRunagainst synchronous failures.
sendRunis an external device write invoked from an async plugin callback; if it throws, the error is unhandled in this callback. Consider atry/catchthat surfaces the failure viaactions.evalInterpreterError(consistent with theonErrorpath) so a send failure doesn't silently drop.🤖 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/remoteExecutionConductor/createEv3Conductor.ts` around lines 33 - 36, Update the plugin.onResult callback to wrap client.sendRun in try/catch, and route any synchronous send failure through actions.evalInterpreterError consistently with the existing onError path. Preserve the base64 conversion and normal sendRun behavior when no exception occurs.src/features/remoteExecutionConductor/Ev3WebPlugin.ts (1)
3-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider more descriptive channel/plugin identifiers.
CHANNEL_ID = 'test'andWEB_ID = '__web_test'look like placeholders for what is a production EV3 execution channel. They must stay in sync with the worker (Du.channelAttach = ["test"]), so renaming both sides to something like'ev3'/'__web_ev3'would make the protocol contract clearer without behavior change.🤖 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/remoteExecutionConductor/Ev3WebPlugin.ts` around lines 3 - 15, Replace the placeholder identifiers CHANNEL_ID and WEB_ID in Ev3WebPlugin with descriptive EV3-specific values, such as an “ev3” channel and matching “__web_ev3” plugin ID. Update the corresponding worker Du.channelAttach value to the same channel identifier, preserving the existing protocol behavior and synchronization.
🤖 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 `@src/commons/sagas/WorkspaceSaga/helpers/evalEditor.ts`:
- Around line 58-59: Update evalEditor’s feature-flag selection to use the
EV3-specific flagConductorEv3Enable selector instead of flagConductorEnable, and
replace the corresponding import with the selector from the new flag module.
Keep the isConductorEv3 gating logic unchanged.
In `@src/features/remoteExecutionConductor/createEv3Conductor.ts`:
- Around line 46-49: Update the monitor handler in createEv3Conductor to return
when store.getState().session.remoteExecutionSession is absent instead of
asserting non-null, matching the onError and display handlers. Also validate
message[0] and the parsed port before calling substring or updating the session,
ignoring malformed monitor messages safely.
In `@src/features/remoteExecutionConductor/RemoteExecutionConductorSaga.ts`:
- Around line 28-30: Track the client associated with the active conductor and
update handleConductorRun to recreate the conductor when
session.connection.client changes, terminating the existing conduit before
replacement. Set the tracked client after creation, and clear both
activeConductor and the tracked client in handleConductorDisconnect.
---
Nitpick comments:
In `@src/features/remoteExecutionConductor/createEv3Conductor.ts`:
- Around line 33-36: Update the plugin.onResult callback to wrap client.sendRun
in try/catch, and route any synchronous send failure through
actions.evalInterpreterError consistently with the existing onError path.
Preserve the base64 conversion and normal sendRun behavior when no exception
occurs.
In `@src/features/remoteExecutionConductor/Ev3WebPlugin.ts`:
- Around line 3-15: Replace the placeholder identifiers CHANNEL_ID and WEB_ID in
Ev3WebPlugin with descriptive EV3-specific values, such as an “ev3” channel and
matching “__web_ev3” plugin ID. Update the corresponding worker Du.channelAttach
value to the same channel identifier, preserving the existing protocol behavior
and synchronization.
🪄 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: f3f9174b-d90c-4055-8c61-e3b22de5bece
📒 Files selected for processing (11)
.vscode/settings.jsonpublic/evaluators/ev3-remote-runner.jssrc/commons/sagas/MainSaga.tssrc/commons/sagas/WorkspaceSaga/helpers/evalEditor.tssrc/commons/utils/ActionsHelper.tssrc/features/remoteExecutionConductor/Ev3WebPlugin.tssrc/features/remoteExecutionConductor/RemoteExecutionConductorActions.tssrc/features/remoteExecutionConductor/RemoteExecutionConductorSaga.tssrc/features/remoteExecutionConductor/RemoteExecutionTypes.tssrc/features/remoteExecutionConductor/createEv3Conductor.tssrc/features/remoteExecutionConductor/flagConductorEv3Enable.ts
| const isConductorEv3: boolean = yield select(featureSelector(flagConductorEnable)); | ||
| if (isConductorEv3) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the EV3-specific feature flag selector.
This code incorrectly imports and selects the generic flagConductorEnable flag. It should use the newly introduced flagConductorEv3Enable flag so that the EV3 execution path is gated correctly and independently of other conductor features.
You can directly use the exported selector from the new flag module to simplify the select call.
💡 Proposed fix
- const isConductorEv3: boolean = yield select(featureSelector(flagConductorEnable));
+ const isConductorEv3: boolean = yield select(selectConductorEv3Enable);Remember to also update the imports at the top of the file:
-import { flagConductorEnable } from 'src/features/conductor/flagConductorEnable';
+import { selectConductorEv3Enable } from 'src/features/remoteExecutionConductor/flagConductorEv3Enable';🤖 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/evalEditor.ts` around lines 58 - 59,
Update evalEditor’s feature-flag selection to use the EV3-specific
flagConductorEv3Enable selector instead of flagConductorEnable, and replace the
corresponding import with the selector from the new flag module. Keep the
isConductorEv3 gating logic unchanged.
There was a problem hiding this comment.
I think I agree on this one, if we are just depending on conductor to be enabled, then there is not much point adding a new flag flagConductorEv3Enable instead.
| client.on('monitor', message => { | ||
| const port = message[0].split(':')[1]; | ||
| const key = `port${port.substring(port.length - 1)}` as keyof Ev3DevicePeripherals; | ||
| const currentSession = store.getState().session.remoteExecutionSession!; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard remoteExecutionSession in the monitor handler (inconsistent with the other handlers).
The onError (Line 40) and display (Line 85) handlers both guard if (!currentSession) return, but the monitor handler asserts non-null via !. A monitor event delivered after the session is cleared (e.g. after disconnect) will spread undefined into remoteExecUpdateSession, throwing at runtime. Line 47–48 (message[0].split(':')[1] → port.substring(...)) is also unguarded against a missing/misformatted message[0].
🛡️ Proposed guard
client.on('monitor', message => {
+ const currentSession = store.getState().session.remoteExecutionSession;
+ if (!currentSession) return;
const port = message[0].split(':')[1];
const key = `port${port.substring(port.length - 1)}` as keyof Ev3DevicePeripherals;
- const currentSession = store.getState().session.remoteExecutionSession!;📝 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.
| client.on('monitor', message => { | |
| const port = message[0].split(':')[1]; | |
| const key = `port${port.substring(port.length - 1)}` as keyof Ev3DevicePeripherals; | |
| const currentSession = store.getState().session.remoteExecutionSession!; | |
| client.on('monitor', message => { | |
| const currentSession = store.getState().session.remoteExecutionSession; | |
| if (!currentSession) return; | |
| const port = message[0].split(':')[1]; | |
| const key = `port${port.substring(port.length - 1)}` as keyof Ev3DevicePeripherals; |
🤖 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/remoteExecutionConductor/createEv3Conductor.ts` around lines 46
- 49, Update the monitor handler in createEv3Conductor to return when
store.getState().session.remoteExecutionSession is absent instead of asserting
non-null, matching the onError and display handlers. Also validate message[0]
and the parsed port before calling substring or updating the session, ignoring
malformed monitor messages safely.
| if (!activeConductor) { | ||
| activeConductor = yield call(createEv3Conductor, session.connection.client); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recreate the conductor if the connection client changes.
If a user connects to a different EV3 device, session.connection.client will change, but activeConductor will not be recreated because it is only cleared on an explicit disconnect action. As a result, the conductor will continue interacting with the stale client, and its event listeners will remain tied to the old connection.
Track the active client and recreate the conductor when the connection changes.
💡 Proposed fix
Declare a module-level variable to track the client at the top of the file:
let activeClient: any = null;Update the initialization logic in handleConductorRun:
if (!activeConductor || activeClient !== session.connection.client) {
if (activeConductor) {
activeConductor.conduit.terminate?.();
}
activeConductor = yield call(createEv3Conductor, session.connection.client);
activeClient = session.connection.client;
}Also, remember to clear activeClient in handleConductorDisconnect:
function* handleConductorDisconnect(): any {
activeConductor?.conduit.terminate?.();
activeConductor = null;
activeClient = null;
yield; // satisfies require-yield
}🤖 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/remoteExecutionConductor/RemoteExecutionConductorSaga.ts` around
lines 28 - 30, Track the client associated with the active conductor and update
handleConductorRun to recreate the conductor when session.connection.client
changes, terminating the existing conduit before replacement. Set the tracked
client after creation, and clear both activeConductor and the tracked client in
handleConductorDisconnect.
Addresses review nit: keep saga naming consistent with the other sagas in MainSaga.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U8B3rG53g2Fqvxn7Co1c4G
3 participants
Description
Created a new feature directory remoteExecutionConductor, which serves the same purpose as the remoteExecution feature in a manner that supports the conductor framework
Type of change
How to test
Checklist