fix(graphical): never report a POU as saved when its flow write-back fails (DOPE-495)

src/frontend/components/_templates/accelerator-handler.tsx

@@ -1,3 +1,3 @@
 import { useCallback, useEffect, useRef, useState } from 'react'
 
 import {
@@ -288,21 +288,31 @@
   /**
    * Undo / Redo
    */
+  // An invalid graphical body blocks history changes outright, so say so rather
+  // than letting the shortcut look broken (DOPE-495).
+  const notifyStaleBody = useCallback((pouName: string) => {
+    toast({
+      title: 'History unavailable',
+      description: `The graphical body of "${pouName}" is invalid, so its history was not changed.`,
+      variant: 'fail',
+    })
+  }, [])
+
   useEffect(() => {
     const unsub = accelerator.onUndo(() => {
       if (!meta?.name) return
-      undo(meta.name)
+      if (!undo(meta.name)) notifyStaleBody(meta.name)
     })
     return unsub
-  }, [meta.name, isMonacoFocused, accelerator, undo])
+  }, [meta.name, isMonacoFocused, accelerator, undo, notifyStaleBody])
 
   useEffect(() => {
     const unsub = accelerator.onRedo(() => {
       if (!meta?.name) return
-      redo(meta.name)
+      if (!redo(meta.name)) notifyStaleBody(meta.name)
     })
     return unsub
-  }, [meta.name, isMonacoFocused, accelerator, redo])
+  }, [meta.name, isMonacoFocused, accelerator, redo, notifyStaleBody])
 
   /**
    * Quit app (Ctrl+Q on Windows/Linux)

src/frontend/hooks/use-pou-snapshot.ts

@@ -1,3 +1,3 @@
 import { useCallback } from 'react'
 
 import { useOpenPLCStore } from '../store'
@@ -24,8 +24,9 @@
   const captureAndPush = useCallback(
     (pouName: string) => {
       // A debounced graphical write-back may still be pending — flush it so
-      // the snapshot can't pair a stale body with a fresh flow.
-      flushFlowWriteBacks(useOpenPLCStore.getState, pouName)
+      // the snapshot can't pair a stale body with a fresh flow. A failed flush
+      // leaves the body stale, so there is nothing coherent to capture.
+      if (flushFlowWriteBacks(useOpenPLCStore.getState, pouName).length > 0) return
       const { project, ladderFlows, fbdFlows } = useOpenPLCStore.getState()
       const pou = project.data.pous.find((p) => p.name === pouName)
       if (!pou) return

src/frontend/services/__tests__/save-actions.test.ts

@@ -1,22 +1,162 @@
 /**
  * save-actions.ts test file
  *
- * All exported functions (executeSaveProject, executeSaveFile, executeSaveActiveFile)
- * depend on `openPLCStoreBase.getState()` to read the Zustand store and on
- * `projectPort` (an async interface) to persist data. They also call `toast()`
- * which mutates module-level state.
- *
- * Because these functions are NOT pure (they read/mutate external state),
- * they cannot be tested without mocking. The pure helpers they delegate to
- * (sanitizePou, collectDebugVariables, serializePouToText, etc.) are covered
- * by their own dedicated test suites.
- *
- * SKIPPED: requires jest.mock / vi.mock for openPLCStoreBase and projectPort.
+ * The pure helpers these functions delegate to (sanitizePou,
+ * collectDebugVariables, serializePouToText, …) are covered by their own
+ * suites. The cases below drive the real store singleton to pin the DOPE-495
+ * contract: a graphical flow that fails schema validation keeps a stale
+ * `pou.body.value`, so it must never be reported as saved.
  */
 
+import type { PlatformCapabilities } from '../../../middleware/shared/ports/platform-capabilities'
+import type { ProjectPort } from '../../../middleware/shared/ports/project-port'
+import { openPLCStoreBase } from '../../store'
+import type { LadderFlowType } from '../../store/slices/ladder'
+import { getMemoryState } from '../../utils/toast'
+import { executeSaveFile, executeSaveProject } from '../save-actions'
+
+const capabilities = { isNativeApplication: true } as PlatformCapabilities
+
+const lastToast = () => getMemoryState().toasts[0]
+
+function makeProjectPort(): ProjectPort {
+  return {
+    saveProject: vi.fn().mockResolvedValue({ success: true }),
+    saveFile: vi.fn().mockResolvedValue({ success: true }),
+  } as unknown as ProjectPort
+}
+
+function createLadderPou(name: string) {
+  const state = openPLCStoreBase.getState()
+  state.pouActions.create({ type: 'program', name, language: 'ld' })
+  state.ladderFlowActions.startLadderRung({
+    editorName: name,
+    rungId: `rung_${name}_1`,
+    defaultBounds: [300, 100],
+    reactFlowViewport: [300, 100],
+  })
+  state.ladderFlowActions.setFlowUpdated({ editorName: name, updated: true })
+}
+
+/** Drop `defaultBounds` / `reactFlowViewport` so the flow fails the zod guard. */
+function corruptFlow(name: string) {
+  const flow = openPLCStoreBase.getState().ladderFlows.find((f) => f.name === name)
+  openPLCStoreBase.getState().ladderFlowActions.addLadderFlow({
+    name,
+    updated: true,
+    rungs: (flow?.rungs ?? []).map((rung) => ({ id: rung.id, comment: '', nodes: [], edges: [] })),
+  } as unknown as LadderFlowType)
+  openPLCStoreBase.getState().ladderFlowActions.setFlowUpdated({ editorName: name, updated: true })
+}
+
+const flowUpdated = (name: string) => openPLCStoreBase.getState().ladderFlows.find((f) => f.name === name)?.updated
+const fileSaved = (name: string) => openPLCStoreBase.getState().files[name]?.saved
+
 describe('save-actions', () => {
-  it('is skipped because all exported functions require store mocking', () => {
-    // Intentionally empty — see file-level comment.
-    expect(true).toBe(true)
+  let warn: ReturnType<typeof vi.spyOn>
+
+  beforeEach(() => {
+    warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    openPLCStoreBase.getState().ladderFlowActions.clearLadderFlows()
+  })
+
+  afterEach(() => {
+    warn.mockRestore()
+  })
+
+  describe('executeSaveProject', () => {
+    it('reports success and clears the updated flag for a valid flow', async () => {
+      createLadderPou('ValidPou')
+
+      const result = await executeSaveProject(makeProjectPort(), capabilities)
+
+      expect(result.success).toBe(true)
+      expect(flowUpdated('ValidPou')).toBe(false)
+    })
+
+    it('does not report a POU as saved when its flow fails validation', async () => {
+      createLadderPou('BrokenPou')
+      corruptFlow('BrokenPou')
+
+      const result = await executeSaveProject(makeProjectPort(), capabilities)
+
+      expect(result.success).toBe(false)
+      // Keeping `updated` set is what lets a later edit retry the write-back.
+      expect(flowUpdated('BrokenPou')).toBe(true)
+      expect(fileSaved('BrokenPou')).toBe(false)
+      expect(lastToast()).toMatchObject({ title: 'Some changes were not saved', variant: 'fail' })
+    })
+
+    it('still saves the valid POUs alongside a failing one', async () => {
+      createLadderPou('GoodPou')
+      createLadderPou('BadPou')
+      corruptFlow('BadPou')
+
+      const projectPort = makeProjectPort()
+      const result = await executeSaveProject(projectPort, capabilities)
+
+      expect(result.success).toBe(false)
+      expect(projectPort.saveProject).toHaveBeenCalled()
+      expect(flowUpdated('GoodPou')).toBe(false)
+      expect(fileSaved('GoodPou')).toBe(true)
+    })
+
+    it('stops blocking saves once the POU behind an invalid flow is deleted', async () => {
+      createLadderPou('Doomed')
+      corruptFlow('Doomed')
+      createLadderPou('Healthy')
+
+      expect((await executeSaveProject(makeProjectPort(), capabilities)).success).toBe(false)
+
+      // Deleting the POU is the user's only escape hatch, and it leaves the
+      // flow behind — the save must stop reporting it.
+      openPLCStoreBase.getState().pouActions.delete('Doomed')
+
+      const result = await executeSaveProject(makeProjectPort(), capabilities)
+
+      expect(result.success).toBe(true)
+      expect(fileSaved('Healthy')).toBe(true)
+    })
+  })
+
+  describe('executeSaveFile', () => {
+    it('refuses to write the stale body of a failing flow', async () => {
+      createLadderPou('BrokenFile')
+      corruptFlow('BrokenFile')
+
+      const projectPort = makeProjectPort()
+      const result = await executeSaveFile('BrokenFile', projectPort, capabilities)
+
+      expect(result.success).toBe(false)
+      expect(projectPort.saveFile).not.toHaveBeenCalled()
+      expect(flowUpdated('BrokenFile')).toBe(true)
+    })
+
+    it('writes a valid flow normally', async () => {
+      createLadderPou('ValidFile')
+
+      const projectPort = makeProjectPort()
+      const result = await executeSaveFile('ValidFile', projectPort, capabilities)
+
+      expect(result.success).toBe(true)
+      expect(projectPort.saveFile).toHaveBeenCalled()
+      expect(flowUpdated('ValidFile')).toBe(false)
+    })
+
+    it('leaves an unrelated failing POU untouched', async () => {
+      createLadderPou('TargetFile')
+      createLadderPou('Unrelated')
+      corruptFlow('Unrelated')
+
+      const projectPort = makeProjectPort()
+      const result = await executeSaveFile('TargetFile', projectPort, capabilities)
+
+      expect(result.success).toBe(true)
+      expect(projectPort.saveFile).toHaveBeenCalled()
+      // The flush is scoped to the target, so the unrelated flow is never
+      // validated and never warns.
+      expect(warn).not.toHaveBeenCalled()
+      expect(flowUpdated('Unrelated')).toBe(true)
+    })
   })
 })

src/frontend/services/save-actions.ts

@@ -1,3 +1,3 @@
 /**
  * Shared save actions for the OpenPLC editor.
  *
@@ -324,8 +324,9 @@
 ): Promise<{ success: boolean }> {
   // Run any pending debounced graphical write-backs before reading state:
   // a save landing inside the debounce window must serialize the fresh
-  // POU bodies, not the pre-edit ones.
-  flushFlowWriteBacks(openPLCStoreBase.getState)
+  // POU bodies, not the pre-edit ones.  Flows that fail validation keep a
+  // stale body, so they must not be reported as saved (DOPE-495).
+  const staleFlows = flushFlowWriteBacks(openPLCStoreBase.getState)
   const state = openPLCStoreBase.getState()
   // Persist gate.  Every save path — Ctrl+S, File → Save, auto-save after
   // a rename/delete, the AI panel — funnels through here.  When the viewer
@@ -339,7 +340,7 @@
   }
   const { project, pendingDeletions } = state
   const { setEditingState } = state.workspaceActions
-  const { setAllToSaved } = state.fileActions
+  const { setAllToSaved, updateFile } = state.fileActions
   const { markAllSaved } = state.snapshotActions
 
   const deletionsBeforeSave = [...pendingDeletions]
@@ -438,22 +439,38 @@
         deleted: deletionsBeforeSave,
       })
 
+      const isStale = new Set(staleFlows)
+
       state.projectActions.clearPendingDeletions()
-      setEditingState('saved')
+      setEditingState(staleFlows.length > 0 ? 'unsaved' : 'saved')
       setAllToSaved()
-      markAllSaved()
+      markAllSaved(staleFlows)
 
-      // Reset graphical flow state: clear selections and updated flags
+      // Reset graphical flow state: clear selections and updated flags.
+      // A stale flow keeps `updated` set and its file dirty — clearing them
+      // would strand the in-memory edit with no way back to disk.
       for (const flow of state.ladderFlows) {
         state.ladderFlowActions.clearSelections({ editorName: flow.name })
+        if (isStale.has(flow.name)) continue
         state.ladderFlowActions.setFlowUpdated({ editorName: flow.name, updated: false })
       }
       for (const flow of state.fbdFlows) {
         state.fbdFlowActions.clearSelections({ editorName: flow.name })
+        if (isStale.has(flow.name)) continue
         state.fbdFlowActions.setFlowUpdated({ editorName: flow.name, updated: false })
       }
+      // Must stay after `setAllToSaved()` above, which marks every file saved.
+      for (const name of staleFlows) {
+        updateFile({ name, saved: false })
+      }
 
-      if (!capabilities.isNativeApplication) {
+      if (staleFlows.length > 0) {
+        toast({
+          title: 'Some changes were not saved',
+          description: `The graphical body of ${staleFlows.join(', ')} is invalid and could not be written to disk. Every other file was saved.`,
+          variant: 'fail',
+        })
+      } else if (!capabilities.isNativeApplication) {
         toast({
           title: 'Changes saved!',
           description: 'The project was saved successfully!',
@@ -468,7 +485,9 @@
         variant: 'fail',
       })
     }
-    return { success: res.success }
+    // A stale flow means the user's graphical edit never reached disk, so
+    // callers that gate on the save (build, close-project) must not proceed.
+    return { success: res.success && staleFlows.length === 0 }
   } catch {
     setEditingState('unsaved')
     toast({
@@ -496,8 +515,9 @@
   projectPort: ProjectPort,
   capabilities: PlatformCapabilities,
 ): Promise<{ success: boolean }> {
-  // See executeSaveProject — same pending write-back flush requirement.
-  flushFlowWriteBacks(openPLCStoreBase.getState)
+  // See executeSaveProject — same pending write-back flush requirement, scoped
+  // to the target so a single-file save doesn't touch unrelated POUs.
+  const staleFlows = flushFlowWriteBacks(openPLCStoreBase.getState, fileName)
   const state = openPLCStoreBase.getState()
   // See executeSaveProject for rationale — same persist gate.
   if (!state.workspace.canEdit) {
@@ -524,6 +544,12 @@
     return { success: false }
   }
 
+  // Writing the stale body would overwrite disk with pre-edit content and
+  // then report success — abort instead (DOPE-495).
+  if (staleFlows.includes(fileName)) {
+    return fail(`The graphical body of "${fileName}" is invalid, so the file was not written to disk.`)
+  }
+
   try {
     // Use the same canonical serializer as the full-project save path so
     // both flows agree on what bytes hit disk. For POUs and JSON files this