fix(graphical): never report a POU as saved when its flow write-back fails (DOPE-495) - #973
Conversation
…fails (DOPE-495) The graphical flow write-back validates with zod before persisting into `pou.body.value`. On failure it returned silently, so the save flow went on to serialize the stale pre-edit body, mark every file saved, clear every `updated` flag and show "Changes saved!". The user's edit survived only in memory and died with the app. `runWriteBack` now reports failure and logs the zod issues; `flushFlowWriteBacks` returns the POUs whose body is still stale. The save paths handle those per-POU: the flow keeps `updated`, the file stays dirty, its undo baseline is not reset, and a failure toast names it — while every other POU still saves. Single-file save aborts before writing rather than overwriting disk with the stale body. Flush also had a hole: it swept only POUs with a live debounce timer, so a timer that had already fired and failed left nothing pending and the save reported success anyway. It now writes back every `updated` flow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Btq4UkctubeNQvh2cYAUv
…5-flow-writeback-silent-skip
WalkthroughGraphical flow write-backs now return validation failures. Project saves preserve stale flows as unsaved, while file saves reject stale graphical bodies before writing. Undo, redo, and snapshot capture also stop when write-back validation fails. ChangesGraphical flow save validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant executeSaveProject
participant flushFlowWriteBacks
participant ProjectPersistence
participant ToastState
User->>executeSaveProject: save project
executeSaveProject->>flushFlowWriteBacks: flush updated graphical flows
flushFlowWriteBacks-->>executeSaveProject: return stale POU names
executeSaveProject->>ProjectPersistence: persist valid project files
executeSaveProject->>ToastState: show partial-save failure
Possibly related PRs
Suggested reviewers: 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/store/slices/shared/slice.ts (1)
993-996: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck
flushFlowWriteBacksbefore building undo/redo snapshots.
flushFlowWriteBacksreturns the POUs whose write-back leftpou.body.valuestale.undoandredodiscard this and still read that stale body while pairing it with the fresh flow snapshot. IfpouNameis in the returned array, skip the history capture/restore for this operation or otherwise defer it until the pending edit is fixed; mirror the same check inredo.🤖 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/store/slices/shared/slice.ts` around lines 993 - 996, Update the undo and redo handlers around flushFlowWriteBacks to inspect its returned stale-POU list before capturing or restoring history snapshots. If pouName is included, skip or defer that operation until the pending body edit is synchronized; apply the same guard to both undo and redo so stale pou.body.value is never paired with a fresh flow snapshot.
🤖 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/services/save-actions.ts`:
- Line 516: Update the save flow around staleFlows and the fileName handling to
iterate over every entry returned by
flushFlowWriteBacks(openPLCStoreBase.getState), mirroring executeSaveProject.
For each failed write-back, call updateFile with saved: false and show the
corresponding failure toast, while preserving the existing target-file behavior.
---
Outside diff comments:
In `@src/frontend/store/slices/shared/slice.ts`:
- Around line 993-996: Update the undo and redo handlers around
flushFlowWriteBacks to inspect its returned stale-POU list before capturing or
restoring history snapshots. If pouName is included, skip or defer that
operation until the pending body edit is synchronized; apply the same guard to
both undo and redo so stale pou.body.value is never paired with a fresh flow
snapshot.
🪄 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 Plus
Run ID: 0529904d-4668-4cf7-a9ea-c9edb6d8d3b3
📒 Files selected for processing (7)
src/frontend/services/__tests__/save-actions.test.tssrc/frontend/services/save-actions.tssrc/frontend/store/__tests__/flow-writeback.test.tssrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/slices/shared/flow-writeback.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/shared/types.ts
|
Thanks — the unscoped flush is accurate, but I don't think the missing handling is a defect here. Not adopting, reasoning below.
That's why the loop is load-bearing in The toast would be a UX regression. The user pressed Ctrl+S on One real gap, narrow: when the debugger is visible, The unscoped flush itself is pre-existing (unchanged by this PR) and harmless — a single-file save has no reason to write back other POUs, but they flush on their own 200ms timer regardless, so scoping it would be a no-op refactor outside this ticket's scope. |
Review — one blocker, then good to goScoped deliberately to the changed lines. Where a finding depends on code this PR didn't touch, I say so. Mirror check: reviewed together with the web counterpart Autonomy-Logic/openplc-web#639 — patches match except for git's abbreviated blob hash length, and all 7 files are byte-identical ( How I verified: ran the three affected jest suites with the fix (177 pass), then reverted the sources and kept the new tests — 8 of 21 fail, so the tests genuinely pin the new behaviour. I also drove the real store singleton through The core fix is right, and the subtle part is the part that matters: 🔴 An invalid orphan flow blocks every save and every build, permanently
// flow-writeback.ts:104-111 — sweeps the store now, not the pending timers
for (const flow of state.ladderFlows) {
if (pouName !== undefined && flow.name !== pouName) continue
if (flow.updated && !runWriteBack(getState, flow.name, 'ld')) failed.push(flow.name)
}// save-actions.ts:487
return { success: res.success && staleFlows.length === 0 }The sweep never checks that a POU still exists for the flow. Probe: corrupt a flow, save, then delete the POU (the user's only escape hatch through the UI), then save twice more: What that costs the user, all traceable in code:
Before this diff an invalid orphan flow was silently skipped — wrong, but it blocked nothing. The blocking behaviour comes from these lines, so I'd fix it here. Smallest change, in the file you're already rewriting — a flow with no POU has no body to write back, so it isn't a write-back failure: export function flushFlowWriteBacks(getState: GetWriteBackState, pouName?: string): readonly string[] {
cancelFlowWriteBacks(pouName)
const state = getState()
const hasPou = (name: string) => state.project.data.pous.some((p) => p.name === name)
const failed: string[] = []
for (const flow of state.ladderFlows) {
if (pouName !== undefined && flow.name !== pouName) continue
if (!hasPou(flow.name)) continue // orphan flow (POU deleted) — nothing to write back
if (flow.updated && !runWriteBack(getState, flow.name, 'ld')) failed.push(flow.name)
}
// same for fbdFlowsThe leak itself ( 🟡 Worth changing1. const staleFlows = flushFlowWriteBacks(openPLCStoreBase.getState)On a single-file save this now writes back every dirty graphical POU in the project (previously: only those with a live timer). The resulting I checked what that actually causes: nothing is lost or misreported — an unrelated failing POU keeps const staleFlows = flushFlowWriteBacks(openPLCStoreBase.getState, fileName)2. No recovery path when a flow can't be repaired — Even without the orphan case: a flow that never validates again means no successful project save and no build, indefinitely. The toast names the POU, but the UI offers nothing to repair a corrupted flow (reload from disk, discard the graphical edit). Failing loudly is the right call; failing loudly with no way out turns a data bug into blocked work. Worth confirming as a product decision and, at minimum, pointing at the remedy in the toast text. 3. setAllToSaved() // :444 marks everything saved
markAllSaved(staleFlows) // :445 already takes the exceptions
...
for (const name of staleFlows) {
updateFile({ name, saved: false }) // :461 walks back :444 for the stale ones
}Two shapes for the same concern — 4. Test isolation — Driving the real store singleton is the right choice for pinning this contract, but 5. Changed branches with no coverage
🟢 Nits
What's good
Previous review — statusCodeRabbit reviewed 1. 2. That's the DOPE-495 lie back again, and it erases the protections this PR just added. The mechanism is pre-existing (the flush used to return undo: (pouName) => {
// A failed write-back leaves pou.body.value stale — capturing now would store
// an old body next to a fresh flow in the history (DOPE-495).
if (flushFlowWriteBacks(getState, pouName).length > 0) returnSolid work — the dead-timer hole is the kind of thing that's easy to miss and it's the actual root cause, the per-POU choice is right (one bad flow shouldn't hold every other POU's work hostage), and the reachability section in the description is honest about scope. The 🔴 is what I'd want fixed before merge, plus 🟡 1 since it's a one-liner that settles the open review thread. And whatever you change, mirror it byte-for-byte in the other repo. Review assisted by Claude Code. |
…(DOPE-495) Review follow-up on the flow write-back fix. Three consequences of sweeping every `updated` flow rather than only the ones with a live timer: Deleting a POU leaves its flow behind — `deleteElement` clears the project entry, model, file and tab, but not the flow, and `removeLadderFlow` has one caller (the AI tool executor). So an invalid orphan was reported stale on every later save and `success` stayed false for good: builds blocked, "save and close" never completing, and a toast naming a POU that no longer exists. Deleting the POU is the user's only escape hatch from a corrupted flow, and it was the one path still broken. A flow with no POU has no body to write back, so skip it. `executeSaveFile` flushed every dirty graphical POU on a single-file save. Nothing was misreported, but it validated and warned about POUs the user was not saving. Scope it to the target. undo, redo and snapshot capture discarded the flush result, which restored a file to "saved" over a body that never reached disk — the original bug by another route, and worse: it clears `updated`, so nothing retries. All three now bail. `undo`/`redo` return a boolean so the accelerator handler can raise a "History unavailable" toast instead of letting the shortcut look broken; `captureAndPush` stays silent because it fires on every edit. Follow-ups, including the `deleteElement` leak this only treats: DOPE-524. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VKiQpeqRqne8nLj2fUGH21
|
Review response is on the web-side mirror, to keep it in one place: https://github.com/Autonomy-Logic/openplc-web/pull/639#issuecomment-5182496393 Short version: the 🔴 orphan-flow guard and 🟡 1 (scoped flush) are in, plus the undo/redo/snapshot flush-result checks that CodeRabbit raised — landed here rather than deferred, since that path needs no POU deletion and clears All 9 files byte-identical with #639 ( Reply assisted by Claude Code. |
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/store/__tests__/shared-slice.test.ts`:
- Around line 1114-1118: Replace the `as unknown as LadderFlowType` assertion in
the `addLadderFlow` test setup with a properly typed fixture or dedicated test
seam that intentionally triggers the Zod validation failure. Preserve the
malformed-data scenario without bypassing the TypeScript model, and avoid all
type assertions except `as const`.
🪄 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: e09b9e85-1cd0-4f84-b313-4d6a8e2124c3
📒 Files selected for processing (8)
src/frontend/components/_templates/accelerator-handler.tsxsrc/frontend/hooks/use-pou-snapshot.tssrc/frontend/services/__tests__/save-actions.test.tssrc/frontend/services/save-actions.tssrc/frontend/store/__tests__/shared-slice.test.tssrc/frontend/store/slices/shared/flow-writeback.tssrc/frontend/store/slices/shared/slice.tssrc/frontend/store/slices/shared/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/services/save-actions.ts
- src/frontend/store/slices/shared/flow-writeback.ts
2 participants
Fixes DOPE-495.
Problem
The graphical flow write-back validates the flow with zod before persisting it into
pou.body.value. On failure it returned silently. The save flow then went on to serialize the stale pre-edit body, upload it, mark every file saved, clear everyupdatedflag, and show "Changes saved!".The user's graphical edit survived only in memory, and died with the app. Worse, every signal the user could check agreed with the lie: the tab went clean, the undo baseline reset, and Source Control adopted the stale content as the new baseline.
Fix
Per-POU, never global — one flow failing a defensive schema check must not hold every other POU's valid work hostage.
runWriteBackreturns a failure result and logs the zod issues (console.warn).flushFlowWriteBacksreturns the names of POUs whose body is still stale.executeSaveProject: stale POUs keepupdated, stay dirty, are excluded from the undosavedAtDepthreset, and get a failure toast naming them. Every other POU saves normally. Returnssuccess: falseso callers that gate on the save (build, close-project) don't proceed on a body that never reached disk.executeSaveFile: aborts before writing rather than overwriting disk with the stale body.markAllSavedtakes an optionalexceptlist.Bonus hole closed
flushFlowWriteBacksswept only POUs with a live debounce timer. A timer that had already fired and failed leaves no pending entry behind, so the save-time sweep came back empty and reported success anyway. It now writes back everyupdatedflow, which is also a stronger coherence guarantee for the undo/redo and snapshot callers.Review response
Landed on top of the original fix, all mirrored byte-for-byte.
An invalid orphan flow no longer blocks saves and builds for good. Deleting a POU leaves its flow behind in
ladderFlows/fbdFlows—deleteElementclears the project entry, model, file and tab, but not the flow, andremoveLadderFlowhas exactly one caller (the AI tool executor). Because the fix widened the sweep to everyupdatedflow, an invalid orphan was reported stale on every subsequent save, sosuccessstayedfalseforever: builds blocked atworkspace-activity-bar/default.tsx:186-188, "save and close" never completing atsave-changes-modal.tsx:58-59, and a toast naming a POU that no longer exists.That mattered more than its reachability suggests: deleting the POU is the user's only escape hatch from a corrupted flow, and it was the one path that stayed broken. A valid orphan already self-healed, so only the invalid case stuck.
flushFlowWriteBacksnow skips flows whose POU is gone — they have no body left to write back, so they were never write-back failures.executeSaveFilescopes its flush to the target. It was flushing every dirty graphical POU on a single-file save. Nothing was lost or misreported — an unrelated failing POU keepssaved: false/updated: true, andexecuteSaveFilenever marks other files saved — but it validated and warned about POUs the user wasn't saving.undo / redo / snapshot capture now respect the flush result.
snapshotActions.undo,redoandusePouSnapshot.captureAndPushall discarded it, which reproduced the original DOPE-495 lie by another route:Worse than the orphan case, in fact: it needs no POU deletion, and it clears
updated, so nothing retries. The module docblock already promised this invariant ("undo/redo and snapshot capture flush the affected POU so a history entry never pairs a stale body with a fresh flow") — it just wasn't enforced. All three now bail when the flush fails.undo/redoreturnboolean(falseonly for the stale-body case) so the UI can react, matchingrunWriteBack's shape.accelerator-handler.tsx— the only live entry point, since the Edit menu's items aredisabled— raises a "History unavailable" toast naming the POU, so the shortcut doesn't just look broken. Deliberately no toast oncaptureAndPush: it fires on every graphical edit and would spam one per keystroke.The toast lives in the component rather than the slice because no store slice imports
toasttoday, and keeping UI concerns out of the store preserves the layer split.Smaller changes in the same lines:
flushFlowWriteBacksreturnsreadonly string[], symmetric withmarkAllSaved(except?: readonly string[]); the twostaleFlows.includes()calls inside the flow loops became aSet; and a comment records that theupdateFile({ saved: false })loop must stay aftersetAllToSaved(). That ordering is already pinned by a test — reversing it failsdoes not report a POU as saved when its flow fails validation— so it stayed a comment rather than a newsetAllToSaved(except)parameter on a mirrored surface.Follow-ups
Tracked in DOPE-524, led by the
deleteElementflow leak — the root cause the guard above only treats.renameElementalready renames both flows, so delete is simply missing the equivalent. Also covers the missing recovery path for an unrepairable flow, the inaccurate "could not be written to disk" toast copy (the stale body was written by then), the unbounded name list, repeated zod dumps on every autosave, test isolation insave-actions.test.ts, two uncovered branches, and theflushFlowWriteBacksnaming. Every item needs a mirror PR.Reachability
Investigated before writing the fix; the card's "Low" holds for everything probed:
.ldprojects on disk (21/21/2 rungs) — all schema-validstartLadderRung,addNewElementfor contact/coil/block/variable — all validbuildNodes.tsxsetsdraggable/selectableNo live-UI repro found. Deeper paths (nested parallel, handle-branch, drag-n-drop, copy-paste, FBD) were not exhaustively probed — the new
console.warnplus the failure toast are precisely the instrument that will name the offending fields if one exists in the wild.Noted for later, deliberately not fixed here
parseGraphicalPouFromStringrunsJSON.parsestraight intobody.valuewith no schema validation — nothing between disk and the store checks the flow. That is what lets a corrupted or hand-edited.ld/.fbdreach the store schema-invalid in the first place, and it sits against this repo's own "validate external data at the boundary" rule.Left alone on purpose: repairing at load would byte-drift the serialized POU vs. the disk copy and reintroduce the phantom "Modified" entries DOPE-477 removed. Recording it here so we can revisit if this ever turns out to bite in practice.
Testing
flushFlowWriteBackscase'is a no-op when nothing is pending'encoded the exact hole described above and was rewritten to the new contract.save-actions.test.tswas a stub; it now drives the real store singleton through both save paths. It avoidsvi.mock/vi.hoistedon purpose — the editor aliasesvi = jest, and jest's hoisting plugin does not recognisevi.mock. Asserts against the exportedgetMemoryState()instead, so the file works byte-identically under both runners..ld(a node missing its requiredselectable): console warning, failure toast, tab stays dirty, other POUs save, single-file save refuses. Regression pass over normal save / undo-redo / debounce-window save / Source Control / build showed no change.flow-writeback.tsat 100% statements/lines/functions.tsc,prettier,eslintandvalidate:archclean in both repos — the editor'stscwas run explicitly, since the web project's config misses callback-variance errors that thereadonly string[]change could have introduced._templates/has no test directory and is not under a coverage threshold. Verified manually instead.src/frontend/services/st-lsp/__tests__/boot.test.ts(jest-only APIs) are unrelated — verified identical on a clean tree.Mirror
Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/639 — diffs are byte-identical across all 9 files.
🤖 Generated with Claude Code
https://claude.ai/code/session_018Btq4UkctubeNQvh2cYAUv
Review follow-up: https://claude.ai/code/session_01VKiQpeqRqne8nLj2fUGH21
Summary by CodeRabbit