fix(runtime-v4): report a switch in STOP as a warning, not a failed upload - #985
Closed
thiagoralves wants to merge 4 commits into
Closed
thiagoralves wants to merge 4 commits into
thiagoralves wants to merge 4 commits into
Conversation
…pload Uploading with the hardware mode switch in STOP ended with: Compilation completed successfully (exit code: 0). Failed to start PLC: START:ERROR_SWITCH_STOP Failed to upload to runtime. Stopping compilation process. All three lines after the first are wrong. The program did reach the device and compiled there; the runtime simply declined to start it, which is what a mode switch in STOP is for. Leaving the switch there while uploading is a normal thing to do, so the message should not send anyone looking for a problem. startPlcAfterBuild now recognises ERROR_SWITCH_STOP as its own outcome and explains it as a warning: "Program uploaded. The PLC was not started because the mode switch is in STOP -- move it to RUN to start." It is not retried either; nothing changes until a human moves the switch, so the 5 s BUSY loop had no business spinning on it. deployRuntimeProgram maps that to UPLOADED_NOT_STARTED, and both platform adapters treat it as a successful upload. Whether an outcome means "the program reached the device" now lives in one shared predicate, deployReachedDevice(), rather than being re-derived as `outcome === 'STARTED'` in each adapter -- the editor had it in two places and web in a third, which is how they would drift. Not addressed here, but noticed: START_TIMEOUT still maps to a failed upload, so a runtime that stays BUSY past the deadline reports "Failed to upload to runtime" after logging a warning that says otherwise. Same shape of bug, different trigger.
Contributor
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds a STOP-mode PLC start outcome, maps it to a successful upload without startup, updates runtime v3 and v4 upload handling, and blocks PLC controls during runtime transitions. ChangesPLC runtime behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant deployRuntimeProgram
participant startPlcAfterBuild
participant PLC
Compiler->>deployRuntimeProgram: deploy runtime program
deployRuntimeProgram->>startPlcAfterBuild: start uploaded program
startPlcAfterBuild->>PLC: request startup
PLC-->>startPlcAfterBuild: ERROR_SWITCH_STOP
startPlcAfterBuild-->>deployRuntimeProgram: SWITCH_IN_STOP
deployRuntimeProgram-->>Compiler: UPLOADED_NOT_STARTED
Compiler->>Compiler: deployReachedDevice returns true
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
TRANSITIONING means a start or stop is in flight: the runtime answers COMMAND:BUSY to everything except PING and STATUS, and the state it will settle on is not decided yet. Clicking the sidebar button then could only produce a confusing error -- and the icon is ambiguous anyway, since it is drawn from a state that is about to change. The button now goes inert until the runtime lands, with the tooltip saying "PLC is changing state..." instead of offering Start or Stop. handlePlcControl gets the same guard. Status is polled every 2 s, so a render can be up to that stale; the guard closes the window where the button looks live because the poll has not caught up yet, and covers any caller that is not the click. The condition lives in one derived flag rather than being repeated in the disabled prop, the className and the tooltip.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/frontend/components/_organisms/workspace-activity-bar/default.tsx (1)
428-432: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLatch a pending PLC command before awaiting the runtime.
runtime.startPlc()andruntime.stopPlc()sendruntime:start-plc/runtime:stop-plcwithout updatingplcStatus, which is only changed by polled status. The current guard andisPlcControlBlockedcan still allow a second click before the next poll, producing a second PLC command. Store an in-flight command state inhandlePlcControland include it in the disabled-state calculation.🤖 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/frontend/components/_organisms/workspace-activity-bar/default.tsx` around lines 428 - 432, Update handlePlcControl to latch an in-flight start/stop command before awaiting runtime.startPlc() or runtime.stopPlc(), clear it when the operation completes, and reject additional commands while latched. Include this pending-command state in isPlcControlBlocked so the controls disable immediately, independent of polled plcStatus.
🤖 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/frontend/components/_organisms/workspace-activity-bar/default.tsx`:
- Around line 931-942: Update the PlayButton invocation in the workspace
activity bar to provide an explicit aria-label matching the action-specific
label already used by its tooltip, so PLC start, stop, and transition states are
announced correctly instead of always “Play.”
- Around line 468-479: Update isPlcControlBlocked alongside isPlcTransitioning
to also disable PLC controls when jwtToken is absent, matching the guard in
handlePlcControl and preventing enabled no-op clicks on connected runtimes
without authentication.
---
Nitpick comments:
In `@src/frontend/components/_organisms/workspace-activity-bar/default.tsx`:
- Around line 428-432: Update handlePlcControl to latch an in-flight start/stop
command before awaiting runtime.startPlc() or runtime.stopPlc(), clear it when
the operation completes, and reject additional commands while latched. Include
this pending-command state in isPlcControlBlocked so the controls disable
immediately, independent of polled plcStatus.
🪄 Autofix
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 Plus
Run ID: 3e15c179-467d-4be3-a35a-d44d3d3bff5e
📒 Files selected for processing (1)
src/frontend/components/_organisms/workspace-activity-bar/default.tsx
Comment on lines
+468
to
+479
| /** | ||
| * Run/stop is blocked while the runtime is mid-transition. | ||
| * | ||
| * TRANSITIONING means a start or stop is in flight: the runtime answers | ||
| * COMMAND:BUSY to everything except PING and STATUS, and the state it will | ||
| * settle on is not decided yet. Clicking then cannot do what the icon says -- | ||
| * the icon itself is ambiguous, since it is drawn from a state that is about to | ||
| * change -- so the button goes inert until the runtime lands. | ||
| */ | ||
| const isPlcTransitioning = plcStatus === 'TRANSITIONING' | ||
| const isPlcControlBlocked = connectionStatus !== 'connected' || isPlcTransitioning | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching default.tsx:"
fd -a 'default\.tsx$' . | sed 's#^\./##' | head -50
echo
echo "Target file outline/size:"
wc -l src/frontend/components/_organisms/workspace-activity-bar/default.tsx 2>/dev/null || true
ast-grep outline src/frontend/components/_organisms/workspace-activity-bar/default.tsx --view compact 2>/dev/null | head -200 || true
echo
echo "Relevant lines 430-505:"
sed -n '430,505p' src/frontend/components/_organisms/workspace-activity-bar/default.tsx | nl -ba -v430
echo
echo "Search jwtToken and PLC control handlers/usages:"
rg -n "jwtToken|handlePlcControl|isPlcControlBlocked|useOpenPLCStore|plcStatus|connectionStatus|Transitioning" src/frontend/components/_organisms/workspace-activity-bar/default.tsx src -g '*.{ts,tsx}' | head -200Repository: Autonomy-Logic/openplc-editor
Length of output: 451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant lines 430-505:"
sed -n '430,505p' src/frontend/components/_organisms/workspace-activity-bar/default.tsx | awk '{printf "%6d\t%s\n", NR+429, $0}'
echo
echo "JWT/control related lines in target file:"
rg -n "jwtToken|handlePlcControl|isPlcControlBlocked|useOpenPLCStore|plcStatus|connectionStatus|Transitioning|TRANSACTION|TRANSITIONING" src/frontend/components/_organisms/workspace-activity-bar/default.tsx
echo
echo "Store/selector definitions for jwtToken and connectionStatus:"
rg -n "jwtToken|connectionStatus|plcStatus|useOpenPLCStore|createOpenPLCStore|PLCStore|store" src -g '*.{ts,tsx}' | head -300
echo
echo "Read likely store definitions (top 30 matches names):"
rg -l "jwtToken|connectionStatus" src -g '*.{ts,tsx}' | head -20 | while read -r f; do
echo "--- $f"
wc -l "$f"
rg -n "jwtToken|connectionStatus|plcStatus" "$f" | head -80
doneRepository: Autonomy-Logic/openplc-editor
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate store files:"
fd -i 'store\.(ts|tsx)$|index\.ts$' src/frontend/components/_organisms/workspace-activity-bar src/frontend/store 2>/dev/null | awk '{printf "%s\n",$0}' | head -100
echo
echo "Relevant workspace activity button context 905-950:"
sed -n '905,950p' src/frontend/components/_organisms/workspace-activity-bar/default.tsx | awk '{printf "%6d\t%s\n", NR+904, $0}'
echo
echo "Find runtimePort.startPlc/stopPlc and runtimeJwtToken usages:"
rg -n "startPlc|stopPlc|runtimeJwtToken|runtimeConnection\.jwtToken|jwtToken:" src -g '*.{ts,tsx}' | head -250
echo
echo "Inspect target runtime-port type/implementation slice:"
rg -n "startPlc|stopPlc|runtimeJwtToken|RuntimePort|runtimeConnection|jwtToken" src/middleware/shared/ports/runtime-port.ts src/main src/middleware -g '*.{ts,tsx}' | head -300Repository: Autonomy-Logic/openplc-editor
Length of output: 17971
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Runtime adapter startPlc/stopPlc:"
sed -n '120,155p' src/middleware/adapters/editor/runtime-adapter.ts | awk '{printf "%6d\t%s\n", NR+119, $0}'
echo
echo "Runtime adapter dependency/connection setup:"
rg -n "createEditorRuntimeAdapter|jwtToken|stringOrNull|runtimeConnection|connected|RuntimePort" src/middleware/adapters/editor/runtime-adapter.ts src/frontend/store src/frontend/store/slices/device -g '*.{ts,tsx}' | head -220
echo
echo "Device slice actions around jwtToken:"
sed -n '400,455p' src/frontend/store/slices/device/slice.ts | awk '{printf "%6d\t%s\n", NR+399, $0}'
echo
echo "Read-only verifier: handler disable-conditions vs runtime token dependency"
python3 - <<'PY'
from pathlib import Path
target = Path("src/frontend/components/_organisms/workspace-activity-bar/default.tsx")
text = target.read_text()
checks = {
"handlePlcControl returns when !jwtToken": "if (!jwtToken || connectionStatus !== 'connected') return" in text,
"handlePlcControl returns when disconnected": "if (!jwtToken || connectionStatus !== 'connected') return" in text,
"handlePlcControl returns when transitioning": "if (plcStatus === 'TRANSITIONING') return" in text,
"isPlcControlBlocked blocked disconnected": "connectionStatus !== 'connected'" in text,
"isPlcControlBlocked blocked transitioning": "isPlcTransitioning" in text,
"isPlcControlBlocked currently omits jwtToken": "const isPlcControlBlocked = connectionStatus !== 'connected' || isPlcTransitioning" in text,
}
for name, result in checks.items():
print(f"{name}: {result}")
PYRepository: Autonomy-Logic/openplc-editor
Length of output: 26706
Include the token check in the PLC control disabled state.
handlePlcControl exits when jwtToken is absent, but isPlcControlBlocked only blocks disconnected and transitioning states. A connected runtime without a token would keep the PLC button enabled for a no-op click.
Proposed fix
- const isPlcControlBlocked = connectionStatus !== 'connected' || isPlcTransitioning
+ const isPlcControlBlocked = connectionStatus !== 'connected' || !jwtToken || isPlcTransitioning📝 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
| /** | |
| * Run/stop is blocked while the runtime is mid-transition. | |
| * | |
| * TRANSITIONING means a start or stop is in flight: the runtime answers | |
| * COMMAND:BUSY to everything except PING and STATUS, and the state it will | |
| * settle on is not decided yet. Clicking then cannot do what the icon says -- | |
| * the icon itself is ambiguous, since it is drawn from a state that is about to | |
| * change -- so the button goes inert until the runtime lands. | |
| */ | |
| const isPlcTransitioning = plcStatus === 'TRANSITIONING' | |
| const isPlcControlBlocked = connectionStatus !== 'connected' || isPlcTransitioning | |
| /** | |
| * Run/stop is blocked while the runtime is mid-transition. | |
| * | |
| * TRANSITIONING means a start or stop is in flight: the runtime answers | |
| * COMMAND:BUSY to everything except PING and STATUS, and the state it will | |
| * settle on is not decided yet. Clicking then cannot do what the icon says -- | |
| * the icon itself is ambiguous, since it is drawn from a state that is about to | |
| * change -- so the button goes inert until the runtime lands. | |
| */ | |
| const isPlcTransitioning = plcStatus === 'TRANSITIONING' | |
| const isPlcControlBlocked = connectionStatus !== 'connected' || !jwtToken || isPlcTransitioning |
🤖 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/frontend/components/_organisms/workspace-activity-bar/default.tsx` around
lines 468 - 479, Update isPlcControlBlocked alongside isPlcTransitioning to also
disable PLC controls when jwtToken is absent, matching the guard in
handlePlcControl and preventing enabled no-op clicks on connected runtimes
without authentication.
Comment on lines
931
to
942
| <PlayButton | ||
| onClick={isSimulatorBoard ? () => void handleSimulatorControl() : () => void handlePlcControl()} | ||
| disabled={isSimulatorBoard ? isCompiling || isDebuggerProcessing : connectionStatus !== 'connected'} | ||
| disabled={isSimulatorBoard ? isCompiling || isDebuggerProcessing : isPlcControlBlocked} | ||
| className={cn( | ||
| isSimulatorBoard | ||
| ? isCompiling || isDebuggerProcessing | ||
| ? disabledButtonClass | ||
| : '' | ||
| : connectionStatus !== 'connected' | ||
| : isPlcControlBlocked | ||
| ? disabledButtonClass | ||
| : '', | ||
| )} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set an action-specific accessible name.
PlayButton supplies aria-label='Play'. This control can start the PLC, stop the PLC, or show a transition state. Screen readers will announce “Play” for the Stop PLC action. Pass the same action label used by the tooltip as aria-label.
Proposed fix
<PlayButton
+ aria-label={
+ isSimulatorBoard
+ ? simulatorRunning
+ ? 'Stop Simulator'
+ : 'Start Simulator'
+ : connectionStatus !== 'connected'
+ ? 'Connect to runtime first'
+ : isPlcTransitioning
+ ? 'PLC is changing state...'
+ : plcStatus === 'RUNNING'
+ ? 'Stop PLC'
+ : 'Start PLC'
+ }
onClick={isSimulatorBoard ? () => void handleSimulatorControl() : () => void handlePlcControl()}📝 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
| <PlayButton | |
| onClick={isSimulatorBoard ? () => void handleSimulatorControl() : () => void handlePlcControl()} | |
| disabled={isSimulatorBoard ? isCompiling || isDebuggerProcessing : connectionStatus !== 'connected'} | |
| disabled={isSimulatorBoard ? isCompiling || isDebuggerProcessing : isPlcControlBlocked} | |
| className={cn( | |
| isSimulatorBoard | |
| ? isCompiling || isDebuggerProcessing | |
| ? disabledButtonClass | |
| : '' | |
| : connectionStatus !== 'connected' | |
| : isPlcControlBlocked | |
| ? disabledButtonClass | |
| : '', | |
| )} | |
| <PlayButton | |
| aria-label={ | |
| isSimulatorBoard | |
| ? simulatorRunning | |
| ? 'Stop Simulator' | |
| : 'Start Simulator' | |
| : connectionStatus !== 'connected' | |
| ? 'Connect to runtime first' | |
| : isPlcTransitioning | |
| ? 'PLC is changing state...' | |
| : plcStatus === 'RUNNING' | |
| ? 'Stop PLC' | |
| : 'Start PLC' | |
| } | |
| onClick={isSimulatorBoard ? () => void handleSimulatorControl() : () => void handlePlcControl()} | |
| disabled={isSimulatorBoard ? isCompiling || isDebuggerProcessing : isPlcControlBlocked} | |
| className={cn( | |
| isSimulatorBoard | |
| ? isCompiling || isDebuggerProcessing | |
| ? disabledButtonClass | |
| : '' | |
| : isPlcControlBlocked | |
| ? disabledButtonClass | |
| : '', | |
| ) |
🤖 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/frontend/components/_organisms/workspace-activity-bar/default.tsx` around
lines 931 - 942, Update the PlayButton invocation in the workspace activity bar
to provide an explicit aria-label matching the action-specific label already
used by its tooltip, so PLC start, stop, and transition states are announced
correctly instead of always “Play.”
Pressing Start with the switch in STOP did nothing at all — no dialog, not even a log line. The button correctly failed to start the PLC and then said nothing about why. The refusal arrives as a SUCCESSFUL request carrying a refusal status: the HTTP call reached the runtime, which answered START:ERROR_SWITCH_STOP. handlePlcControl only inspected result.success, so the refusal was swallowed whole, and the follow-up getStatus() quietly re-rendered the button as Start again. It now reads the status. A switch refusal opens a dialog — "The mode switch on the device is in STOP, so the runtime refused to start the PLC. Move the switch to RUN and try again." — rather than a console line, because the user just asked for this explicitly, is watching the button, and is the only one who can resolve it. Any other unexpected status is logged as an error instead of vanishing. Deliberately different from the upload path, where the same refusal is a warning in the log: there the start is an automatic follow-on step and interrupting a build with a modal would be wrong.
… start" This reverts commit 82bcb65. The feature is not missing — it lives on feature/runtime-run-stop-state, which is the branch this was being tested on, and it is better than what I wrote there. That branch carries refusedBySwitch as a first-class field in the shared debug types, produced by BOTH transports (Modbus FC 0x4b status 0x86 and REST ERROR_SWITCH_STOP) as well as the simulator, and surfaces it through warnSwitchInStop(boardTarget, switchLabel), which names the switch using the label the VPP declares. It has tests. My version string-matched the REST status inside the component, covered neither Modbus nor the simulator, and hardcoded the wording. Keeping it would duplicate working code in a worse form and collide with that branch on merge.
Contributor
Author
|
Closing: this was based on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Uploading a program with the hardware mode switch in STOP ends like this:
Every line after the first is wrong. The program reached the device and compiled there. The runtime declined to start it, which is exactly what a mode switch in STOP is for — and leaving the switch there while uploading is normal, intentional behaviour, not a fault.
Fix
startPlcAfterBuildrecognisesERROR_SWITCH_STOPas its own outcome (SWITCH_IN_STOP) and explains it as a warning: "Program uploaded. The PLC was not started because the mode switch is in STOP — move it to RUN to start."deployRuntimeProgrammaps it toUPLOADED_NOT_STARTED, and both platform adapters treat that as a successful upload, so the pipeline no longer bails.deployReachedDevice(), instead of being re-derived asoutcome === 'STARTED'per adapter — the editor had that in two places and web in a third, which is how the two would drift.Paired PR
The changed logic is in shared surface code, so this ships as a matching pair:
fix/upload-warning-when-switch-in-stopfix/upload-warning-when-switch-in-stopstart-plc-after-build.tsanddeploy-runtime-program.tsare byte-identical across both repos before and after.Testing
ERROR_SWITCH_STOPyieldsSWITCH_IN_STOP, logs atwarning, and is fetched exactly once (not retried). Editor 8/8 jest, web 15/15 vitest.tsc --noEmitclean on web; on editor the only errors are 4 pre-existing ReactFlow typing errors in the graphical editor, none in files touched here.START:ERROR_SWITCH_STOPis returned by the START handler while the switch reads STOP.Noticed, not fixed here
START_TIMEOUTstill maps to a failed upload, so a runtime that stays BUSY past the deadline logs a warning saying it may still start and then reports "Failed to upload to runtime." Same shape of bug, different trigger — left out to keep this change to the reported one.🤖 Generated with Claude Code
Summary by CodeRabbit
Follow-up commit: run/stop blocked while transitioning
The sidebar run/stop button is now inert while
plcStatus === 'TRANSITIONING', with the tooltip reading "PLC is changing state..." rather than offering Start or Stop. Rationale: mid-transition the runtime refuses everything but PING and STATUS, and the icon is drawn from a state that is about to change, so a click cannot do what it appears to.handlePlcControlcarries the same guard. Status is polled every 2 s, so a render can be that stale; the guard closes the window where the button still looks live, and covers callers that are not the click. The condition is one derived flag, not repeated across thedisabledprop, theclassNameand the tooltip.No test added: there is no existing suite for
workspace-activity-bar(0 matching files), and extracting the logic purely for testability would be a larger change than the tweak. Verified viatsc --noEmit.