Add reactions for status channel changes and refactor UI handling of __error and __result channel#3745
Conversation
![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 refactors the dummy conductor evaluator to utilize the conductor runner API and introduces new interpreter actions for handling streaming results and execution status. Key changes include the addition of appendInterpreterResult, appendInterpreterError, and setIsRunning actions, along with corresponding reducer logic and saga updates to manage runner statuses. Review feedback suggests removing redundant properties in action creators, maintaining consistent relative import paths, and enhancing type safety by replacing any with specific error types in the workspace reducer.
yytelliot
force-pushed
the
status-channel
branch
from
25a45fd to
e5f1431
Compare
yytelliot
force-pushed
the
status-channel
branch
from
7b99109 to
c13993a
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…ademy#3731) * Elongate works? * Format fix * Fixes for TS typing * Sentry error and possible render isLive fix * Fix formatting error * Sentry --------- Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com> Co-authored-by: Martin Henz <henz@comp.nus.edu.sg>
Coverage Report for CI Build 24340336543Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.006%) to 40.892%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
Just the one comment, but otherwise LGTM and will merge anyway since it's an example. Thanks!
| const CONDUCTOR_RUNNER_URL = | ||
| 'https://cdn.jsdelivr.net/npm/@sourceacademy/conductor@latest/dist/conductor/runner/index.js'; | ||
| const CONDUCTOR_TYPES_URL = | ||
| 'https://cdn.jsdelivr.net/npm/@sourceacademy/conductor@latest/dist/conductor/types/index.js'; |
There was a problem hiding this comment.
Aren't we using GH instead of NPM?
| new MessageEvent('message', { | ||
| data: event.data | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: When replaying buffered MessageEvent objects, the ports property is omitted, causing any transferred MessagePort objects to be lost.
Severity: HIGH
Suggested Fix
When creating the new MessageEvent for replay, ensure that the ports property from the original buffered event is included in the event's constructor options, alongside the data property. This will preserve any transferred MessagePort objects.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: public/evaluators/debug/dummy-conductor-evaluator.js#L181-L185
Potential issue: When replaying buffered `MessageEvent` objects that arrive before
asynchronous imports are complete, the `ports` property is not copied from the original
event to the replayed event. This leads to the permanent loss of any `MessagePort`
objects that were transferred. As a consequence, communication handshakes that rely on
these ports, such as channel-attach, can fail silently, as the necessary communication
channel is never established.
5 participants
Description
Added status handling in the frontend. Frontend state now reacts to changes in conductor evaluator status, specifically STOPPED, ERROR and RUNNING, instead of being inferred from the result and error channels.
Added a new workspace reducer setIsRunning as the manager for isRunning state. Added new reducers appendInterpreterResult and appendIntepreterError as new reducers for the result and error channel so they do not stop execution.
Note that this change will cause the status of evaluators that do not send status updates to be inaccurately reflected in the frontend. Evaluator implementers will have to update their program state accordingly so that changes in state can now be reflected on the frontend.
Type of change
How to test
Use updateStatus(RunnerStatus.RUNNING, true) and updateStatus(RunnerStatus.RUNNING, false)
See if the run button updates accordingly
Use updateStatus(RunnerStatus.Running, true)
Conduit instance should be terminated and new lines are no longer accepted in the REPL
UI state gets cleared: UI elements return to their state before execution
Checklist