fix(runtime-v4): switch-in-STOP warning, mid-transition gating, and Connect button regressions - #996
Merged
Merged
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.
TRANSITIONING means a start or stop is already underway: the runtime answers COMMAND:BUSY to everything except PING and STATUS, and the state it will settle on is not decided yet — so the icon is drawn from a state that is about to change and a click cannot do what it appears to. Folded into the existing plcControlBlocked / plcControlBlockedReason pair rather than added as a second mechanism, so the tooltip explains this the same way it explains a missing connection: 'PLC is changing state...'. handlePlcControl carries the same guard. Status arrives by poll, so a render can be up to one interval stale; the guard closes the window where the button still looks live and covers callers that are not the click.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe deployment flow distinguishes uploaded programs that did not start because the PLC switch is in STOP. Compiler upload checks accept both started and uploaded outcomes. PLC controls are disabled during transitions. Connection status and custom font class merging are updated. ChangesPLC deployment and control flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant deployRuntimeProgram
participant startPlcAfterBuild
participant PLC
Compiler->>deployRuntimeProgram: deploy runtime program
deployRuntimeProgram->>startPlcAfterBuild: start uploaded program
startPlcAfterBuild->>PLC: send start request
PLC-->>startPlcAfterBuild: return ERROR_SWITCH_STOP
startPlcAfterBuild-->>deployRuntimeProgram: return SWITCH_IN_STOP
deployRuntimeProgram-->>Compiler: return UPLOADED_NOT_STARTED
Compiler->>Compiler: accept with deployReachedDevice
Possibly related PRs
Suggested labels: 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 |
![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: 1
🤖 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 115-121: Update the plcControlBlockedReason conditional in the
workspace activity bar so deviceConnectionStatus !== 'connected' is checked
before plcTransitioning, ensuring disconnected targets display 'Connect to the
target first' even during transitions while preserving the unsupported-target
reason.
🪄 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: 76c2d255-1aa8-4cef-a18c-5c85b371b749
📒 Files selected for processing (5)
src/backend/editor/compiler/editor-compiler-platform-port.tssrc/backend/shared/library/__tests__/start-plc-after-build.test.tssrc/backend/shared/library/deploy-runtime-program.tssrc/backend/shared/library/start-plc-after-build.tssrc/frontend/components/_organisms/workspace-activity-bar/default.tsx
…icate status The button rendered noticeably larger and heavier than the buttons beside it. The cause is cn(): it runs twMerge, which does not know this project's custom cp-* font scale, so it read `text-cp-sm` as conflicting with `text-white` and dropped the size class entirely. The button fell back to the browser default -- larger than the 10px intended AND larger than its 14px neighbours. Sibling brand buttons on this screen escape it only because they pass a plain string rather than cn(). Now h-8/text-sm, matching those siblings, and a size twMerge understands. Also removes both redundant "Connected" labels. The button label already is the status: "Disconnect" can only be shown while connected, "Connect" only while disconnected, so the green "● Connected" beside it and the grey "Connected" after it said the same thing twice more. "PLC: RUNNING" stays -- that is the program's state, which the button says nothing about. "● Connection failed" stays too: the button reads "Connect" whether the last attempt failed or never happened, so that one carries information the label does not.
My previous attempt made the Connect button WORSE, not better. The diagnosis was right -- twMerge silently drops `text-cp-sm` because it cannot tell a custom size from a text colour, so `text-cp-sm text-white` keeps only the colour -- but the fix was wrong: I moved the button to text-sm (14px) when main renders it at text-cp-sm (10px). It stayed too big, just for a new reason. main gets away with the same class string only because it passes a plain string instead of calling cn(). So the bug is in cn(), not in any one button: extendTailwindMerge now declares cp-xs / cp-sm / cp-base as font sizes, and `text-cp-sm text-white` keeps both. Size-versus-size still dedupes correctly. The buttons are back to main's exact classes, which now render as intended. This also silently repairs every other call site that puts a cp-* size and a text colour through cn() -- there is no warning when it happens, only a wrong size.
thiagoralves
changed the title
JoaoGSP
reviewed
Aug 7, 2026
Both files this PR touches sit under an enforced 100% functions/lines
threshold, and both shipped a change no test executed.
`deployReachedDevice` had 0% function coverage — nothing called it. It now
has direct assertions for all three outcome classes, driven off a
`Record<DeployRuntimeProgramOutcome, boolean>` so a new outcome fails to
compile until someone decides which side of "did the program reach the
device?" it falls on. The `SWITCH_IN_STOP -> UPLOADED_NOT_STARTED` mapping
was statement-covered but never asserted; a `deployRuntimeProgram` scenario
now scripts `START:ERROR_SWITCH_STOP` and checks the outcome, that start is
asked exactly once, and that the refusal logs as a warning and not an error.
`cn()`'s font-scale fix shipped with `cn.test.ts` untouched. The suite now
pins the exact regression — `cn('text-cp-sm', 'text-white')` keeps both
classes, `cn('text-cp-xs', 'text-cp-base')` last-wins — and reads the scale
out of tailwind.config.ts rather than restating it, so the "keep this list
in step" comment on `cn()` is enforced instead of hoped for. Verified it
fails against plain twMerge.
Both files stay byte-identical with openplc-web#655.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reason chain asked about TRANSITIONING before the connection, so a disconnected target whose last polled `plcStatus` was TRANSITIONING got "PLC is changing state..." — a state we last saw, not the reason the button is inert. `plcStatus` is polled and survives the drop, so the stale value outlives the session that produced it. Reordering to match `plcControlBlocked`'s own order — unsupported, then no session, then a transition in flight — makes the tail unreachable except for the one reason that is left. Also unwraps `plcControlBlocked` onto one line: at 118 chars it fits inside Prettier's 120, and the manual wrap has been failing the format gate (which `sync` and `complete-build` both hang off) since 20ff67e. Found by CodeRabbit on #996. Mirrored byte-identically in openplc-web#655. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JoaoGSP
self-requested a review
August 7, 2026 15:36
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
2 participants
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.
Run/stop UX fixes for Runtime v4 targets, plus two UI regressions found while testing, rebased cleanly onto
development.Why this replaces
feature/runtime-run-stop-stateThat branch had
feat/always-on-debuggermerged into it, so merging it would have dragged 74 commits / 126 files / +13,576 lines of unreviewed licensing work intodevelopmentas a side effect. That branch has no PR of its own.Everything else on it turned out to be already in development via #980:
refusedBySwitch,switchPosition, FC 0x4b,warnSwitchInStop,debuggerTransports,acquireDebugChannel, the Baremetal run/stop machine and the MBAP floor fix. Every remaining firmware difference belongs to always-on-debugger (license_blob.h,license_gate.h,license_store.h).So this is the genuine remainder, cherry-picked onto development.
1. A mode switch in STOP is a warning, not a failed upload
Uploading with the switch in STOP printed a successful compile followed by
Failed to start PLC/Failed to upload to runtime/Stopping compilation process. The program reached the device and compiled there; only the start was declined, which is what the switch is for.startPlcAfterBuildnow treatsERROR_SWITCH_STOPas its own outcome and reports it as a warning — "Program uploaded. The PLC was not started because the mode switch is in STOP — move it to RUN to start." — without retrying, since nothing changes until someone moves the switch.deployRuntimeProgrammaps it toUPLOADED_NOT_STARTED, which counts as a successful upload."Did the program reach the device?" is now one shared predicate,
deployReachedDevice(), instead ofoutcome === 'STARTED're-derived in two places here and a third in web.2. Run/stop blocked while the runtime is mid-transition
TRANSITIONINGmeans the runtime answersCOMMAND:BUSYto everything but PING and STATUS, and the icon is drawn from a state about to change. The button goes inert with the tooltip "PLC is changing state...", folded into development's existingplcControlBlockedpair beside the not-connected and unsupported-target cases.handlePlcControlcarries the same guard, since status is polled and a render can be one interval stale.3. Connect button font, and duplicate status labels
Both regressions arrived with
231e11d9bin #980, not from this work.The font.
cn()runstwMerge, which does not know this project's customcp-*font scale — it readtext-cp-smas conflicting withtext-whiteand dropped the size, leaving the button at the browser default.mainescapes this only because it passes a plain string rather than callingcn(). Fixed incn()itself viaextendTailwindMerge, declaringcp-xs/cp-sm/cp-baseas font sizes, so the button's original classes render correctly again. This silently repairs every other call site with the same pattern — a failure mode with no warning and no build error, just the wrong size.The duplicates. The button label is the status: "Disconnect" only renders while connected. The green
● Connectedinside the button and the greyConnectedfromDeviceConnectedIndicator(used at two call sites) both said it again, so all three are gone.PLC: RUNNINGstays — that is the program's state, which the button says nothing about — as does● Connection failed, since the button reads "Connect" whether the last attempt failed or never happened.Verification
tsc --noEmitclean apart from 4 pre-existing ReactFlow errors in the graphical editor.mainon hardware.Paired with openplc-web#655
These 8 files are byte-identical across both branches:
start-plc-after-build.ts·deploy-runtime-program.ts·start-plc-after-build.test.ts·board.tsx·device-connect-button/index.tsx·device-connect-button.test.tsx·workspace-activity-bar/default.tsx·cn.tsOnly the platform adapters differ, which is expected.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
UI Improvements