From 3da85070966c5a7803313483ec1f61cbb9d426e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20GS=20Pereira?= Date: Sat, 1 Aug 2026 17:02:19 -0300 Subject: [PATCH 1/2] fix(graphical): never report a POU as saved when its flow write-back fails (DOPE-495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_018Btq4UkctubeNQvh2cYAUv --- .../services/__tests__/save-actions.test.ts | 135 ++++++++++++++++-- src/frontend/services/save-actions.ts | 40 ++++-- .../store/__tests__/flow-writeback.test.ts | 58 +++++++- .../store/__tests__/shared-slice.test.ts | 10 ++ .../store/slices/shared/flow-writeback.ts | 44 ++++-- src/frontend/store/slices/shared/slice.ts | 5 +- src/frontend/store/slices/shared/types.ts | 2 +- 7 files changed, 257 insertions(+), 37 deletions(-) diff --git a/src/frontend/services/__tests__/save-actions.test.ts b/src/frontend/services/__tests__/save-actions.test.ts index dac639efe..6ce34a3f8 100644 --- a/src/frontend/services/__tests__/save-actions.test.ts +++ b/src/frontend/services/__tests__/save-actions.test.ts @@ -1,22 +1,129 @@ /** * 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 + + 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) + }) + }) + + 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) + }) }) }) diff --git a/src/frontend/services/save-actions.ts b/src/frontend/services/save-actions.ts index 5daf9e5e7..723416e55 100644 --- a/src/frontend/services/save-actions.ts +++ b/src/frontend/services/save-actions.ts @@ -324,8 +324,9 @@ export async function executeSaveProject( ): 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 @@ export async function executeSaveProject( } const { project, pendingDeletions } = state const { setEditingState } = state.workspaceActions - const { setAllToSaved } = state.fileActions + const { setAllToSaved, updateFile } = state.fileActions const { markAllSaved } = state.snapshotActions const deletionsBeforeSave = [...pendingDeletions] @@ -439,21 +440,34 @@ export async function executeSaveProject( }) 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 (staleFlows.includes(flow.name)) continue state.ladderFlowActions.setFlowUpdated({ editorName: flow.name, updated: false }) } for (const flow of state.fbdFlows) { state.fbdFlowActions.clearSelections({ editorName: flow.name }) + if (staleFlows.includes(flow.name)) continue state.fbdFlowActions.setFlowUpdated({ editorName: flow.name, updated: false }) } + 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 +482,9 @@ export async function executeSaveProject( 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({ @@ -497,7 +513,7 @@ export async function executeSaveFile( capabilities: PlatformCapabilities, ): Promise<{ success: boolean }> { // See executeSaveProject — same pending write-back flush requirement. - flushFlowWriteBacks(openPLCStoreBase.getState) + const staleFlows = flushFlowWriteBacks(openPLCStoreBase.getState) const state = openPLCStoreBase.getState() // See executeSaveProject for rationale — same persist gate. if (!state.workspace.canEdit) { @@ -524,6 +540,12 @@ export async function executeSaveFile( 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 diff --git a/src/frontend/store/__tests__/flow-writeback.test.ts b/src/frontend/store/__tests__/flow-writeback.test.ts index 649595c8c..36c5d60e0 100644 --- a/src/frontend/store/__tests__/flow-writeback.test.ts +++ b/src/frontend/store/__tests__/flow-writeback.test.ts @@ -173,11 +173,13 @@ describe('flow write-back scheduler', () => { } as unknown as LadderFlowType) store.getState().ladderFlowActions.setFlowUpdated({ editorName: 'Main', updated: true }) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) scheduleFlowWriteBack(getState, 'Main', 'ld') vi.advanceTimersByTime(FLOW_WRITEBACK_DEBOUNCE_MS) expect(ladderBody('Main')?.rungs).toHaveLength(0) expect(store.getState().ladderFlows.find((f) => f.name === 'Main')?.updated).toBe(true) + warn.mockRestore() }) }) @@ -210,13 +212,67 @@ describe('flow write-back scheduler', () => { expect(ladderBody('B')?.rungs).toHaveLength(1) }) - it('is a no-op when nothing is pending', () => { + it('leaves the other language untouched when scoped to one POU', () => { makeDirtyLadderPou('Main') + store.getState().pouActions.create({ type: 'program', name: 'FbdMain', language: 'fbd' }) + store.getState().fbdFlowActions.startFBDRung({ editorName: 'FbdMain' }) + store.getState().fbdFlowActions.setFlowUpdated({ editorName: 'FbdMain', updated: true }) + + expect(flushFlowWriteBacks(getState, 'Main')).toEqual([]) + expect(store.getState().fbdFlows.find((f) => f.name === 'FbdMain')?.updated).toBe(true) + }) + + it('writes back an updated flow that has no pending timer', () => { + makeDirtyLadderPou('Main') + + expect(flushFlowWriteBacks(getState)).toEqual([]) + expect(ladderBody('Main')?.rungs).toHaveLength(1) + expect(store.getState().ladderFlows.find((f) => f.name === 'Main')?.updated).toBe(false) + }) + + it('is a no-op when no flow is marked updated', () => { + makeDirtyLadderPou('Main') + flushFlowWriteBacks(getState) const bodyBefore = ladderBody('Main') + expect(flushFlowWriteBacks(getState)).toEqual([]) + expect(ladderBody('Main')).toBe(bodyBefore) + }) + + it('reports the POU and leaves the body stale when the flow fails validation', () => { + makeDirtyLadderPou('Main') flushFlowWriteBacks(getState) + const bodyBefore = ladderBody('Main') + + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const flow = store.getState().ladderFlows.find((f) => f.name === 'Main') + store.getState().ladderFlowActions.setRungs({ + editorName: 'Main', + // `defaultBounds` is required by the schema — dropping it makes the flow invalid. + rungs: (flow?.rungs ?? []).map(({ defaultBounds: _defaultBounds, ...rung }) => rung) as never, + }) + expect(flushFlowWriteBacks(getState)).toEqual(['Main']) expect(ladderBody('Main')).toBe(bodyBefore) + expect(store.getState().ladderFlows.find((f) => f.name === 'Main')?.updated).toBe(true) + expect(warn).toHaveBeenCalled() + warn.mockRestore() + }) + + it('reports a failing FBD flow', () => { + store.getState().pouActions.create({ type: 'program', name: 'FbdMain', language: 'fbd' }) + store.getState().fbdFlowActions.startFBDRung({ editorName: 'FbdMain' }) + + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + // `nodes` is required by the schema — dropping it makes the flow invalid. + store.getState().fbdFlowActions.setRung({ + editorName: 'FbdMain', + rung: { comment: '', edges: [], selectedNodes: [] } as never, + }) + + expect(flushFlowWriteBacks(getState)).toEqual(['FbdMain']) + expect(warn).toHaveBeenCalled() + warn.mockRestore() }) }) diff --git a/src/frontend/store/__tests__/shared-slice.test.ts b/src/frontend/store/__tests__/shared-slice.test.ts index 0b7720d91..f55b72520 100644 --- a/src/frontend/store/__tests__/shared-slice.test.ts +++ b/src/frontend/store/__tests__/shared-slice.test.ts @@ -2158,6 +2158,16 @@ describe('createSharedSlice', () => { expect(store.getState().undoRedo['P1'].savedAtDepth).toBe(1) expect(store.getState().undoRedo['P2'].savedAtDepth).toBe(2) }) + + it('skips the excluded POUs', () => { + store.getState().snapshotActions.pushToHistory('P1', { variables: [], body: 'v1' }) + store.getState().snapshotActions.pushToHistory('P2', { variables: [], body: 'v1' }) + + store.getState().snapshotActions.markAllSaved(['P2']) + + expect(store.getState().undoRedo['P1'].savedAtDepth).toBe(1) + expect(store.getState().undoRedo['P2'].savedAtDepth).toBe(0) + }) }) // ----------------------------------------------------------------------- diff --git a/src/frontend/store/slices/shared/flow-writeback.ts b/src/frontend/store/slices/shared/flow-writeback.ts index 07e3cd491..605d54d9e 100644 --- a/src/frontend/store/slices/shared/flow-writeback.ts +++ b/src/frontend/store/slices/shared/flow-writeback.ts @@ -34,13 +34,14 @@ type GetWriteBackState = () => SharedRootState const pendingWriteBacks = new Map }>() -function runWriteBack(getState: GetWriteBackState, pouName: string, language: FlowLanguage): void { +/** @returns `false` when the flow is invalid and `pou.body.value` is left stale. */ +function runWriteBack(getState: GetWriteBackState, pouName: string, language: FlowLanguage): boolean { const state = getState() const flow = language === 'ld' ? state.ladderFlows.find((f) => f.name === pouName) : state.fbdFlows.find((f) => f.name === pouName) - if (!flow?.updated) return + if (!flow?.updated) return true // Validate with zod but persist the raw object (minus the transient // `updated` flag). Using the parsed result would silently strip every @@ -48,13 +49,20 @@ function runWriteBack(getState: GetWriteBackState, pouName: string, language: Fl // byte-drifting the serialized POU vs. the loaded disk copy — phantom // "Modified" entries in Source Control (see DOPE-477). const schema = language === 'ld' ? zodLadderFlowSchema : zodFBDFlowSchema - if (!schema.safeParse(flow).success) return + const validation = schema.safeParse(flow) + if (!validation.success) { + console.warn(`[flow-writeback] "${pouName}" (${language}) failed validation — body left stale`, { + issues: validation.error.issues, + }) + return false + } const { updated: _updated, ...flowBody } = flow state.projectActions.updatePou({ name: pouName, content: { language, value: flowBody } }) const flowActions = language === 'ld' ? state.ladderFlowActions : state.fbdFlowActions flowActions.setFlowUpdated({ editorName: pouName, updated: false }) + return true } /** @@ -78,14 +86,30 @@ export function scheduleFlowWriteBack(getState: GetWriteBackState, pouName: stri pendingWriteBacks.set(pouName, { language, timer }) } -/** Run pending write-backs immediately — all of them, or a single POU's. */ -export function flushFlowWriteBacks(getState: GetWriteBackState, pouName?: string): void { - for (const [name, pending] of [...pendingWriteBacks]) { - if (pouName !== undefined && name !== pouName) continue - clearTimeout(pending.timer) - pendingWriteBacks.delete(name) - runWriteBack(getState, name, pending.language) +/** + * Run pending write-backs immediately — all of them, or a single POU's. + * + * Every `updated` flow is written back, not just the ones with a live timer: + * a timer that already fired and failed validation leaves no pending entry + * behind, so a pending-only sweep would report success for a POU whose body + * is still stale (DOPE-495). + * + * @returns names of POUs whose body could not be updated. + */ +export function flushFlowWriteBacks(getState: GetWriteBackState, pouName?: string): string[] { + cancelFlowWriteBacks(pouName) + + const state = getState() + const failed: string[] = [] + 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) + } + for (const flow of state.fbdFlows) { + if (pouName !== undefined && flow.name !== pouName) continue + if (flow.updated && !runWriteBack(getState, flow.name, 'fbd')) failed.push(flow.name) } + return failed } /** Drop pending write-backs without running them (project open). */ diff --git a/src/frontend/store/slices/shared/slice.ts b/src/frontend/store/slices/shared/slice.ts index 016c920bb..8c07a150f 100644 --- a/src/frontend/store/slices/shared/slice.ts +++ b/src/frontend/store/slices/shared/slice.ts @@ -979,10 +979,11 @@ const createSharedSlice: StateCreator = (s ) }, - markAllSaved: () => { + markAllSaved: (except) => { setState( produce((state: SharedRootState) => { - for (const history of Object.values(state.undoRedo)) { + for (const [pouName, history] of Object.entries(state.undoRedo)) { + if (except?.includes(pouName)) continue history.savedAtDepth = history.past.length } }), diff --git a/src/frontend/store/slices/shared/types.ts b/src/frontend/store/slices/shared/types.ts index ba3efb36a..f3d3b824c 100644 --- a/src/frontend/store/slices/shared/types.ts +++ b/src/frontend/store/slices/shared/types.ts @@ -117,7 +117,7 @@ export type EtherCATDeviceActions = { export type SnapshotActions = { pushToHistory: (pouName: string, snapshot: PouHistorySnapshot) => void markSaved: (pouName: string) => void - markAllSaved: () => void + markAllSaved: (except?: readonly string[]) => void undo: (pouName: string) => void redo: (pouName: string) => void } From 4e11afa999a33c7036e91d37f94bcad730e09534 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20GS=20Pereira?= Date: Tue, 4 Aug 2026 14:31:58 -0300 Subject: [PATCH 2/2] fix(graphical): stop an invalid flow from blocking saves and history (DOPE-495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01VKiQpeqRqne8nLj2fUGH21 --- .../_templates/accelerator-handler.tsx | 18 +++++++--- src/frontend/hooks/use-pou-snapshot.ts | 5 +-- .../services/__tests__/save-actions.test.ts | 33 +++++++++++++++++++ src/frontend/services/save-actions.ts | 12 ++++--- .../store/__tests__/shared-slice.test.ts | 29 ++++++++++++++++ .../store/slices/shared/flow-writeback.ts | 8 ++++- src/frontend/store/slices/shared/slice.ts | 16 +++++---- src/frontend/store/slices/shared/types.ts | 6 ++-- 8 files changed, 108 insertions(+), 19 deletions(-) diff --git a/src/frontend/components/_templates/accelerator-handler.tsx b/src/frontend/components/_templates/accelerator-handler.tsx index 1d5136a1e..a28bd40d5 100644 --- a/src/frontend/components/_templates/accelerator-handler.tsx +++ b/src/frontend/components/_templates/accelerator-handler.tsx @@ -288,21 +288,31 @@ const AcceleratorHandler = () => { /** * 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) diff --git a/src/frontend/hooks/use-pou-snapshot.ts b/src/frontend/hooks/use-pou-snapshot.ts index 1ba92a930..e7f715e90 100644 --- a/src/frontend/hooks/use-pou-snapshot.ts +++ b/src/frontend/hooks/use-pou-snapshot.ts @@ -24,8 +24,9 @@ export function usePouSnapshot() { 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 diff --git a/src/frontend/services/__tests__/save-actions.test.ts b/src/frontend/services/__tests__/save-actions.test.ts index 6ce34a3f8..f1f67823a 100644 --- a/src/frontend/services/__tests__/save-actions.test.ts +++ b/src/frontend/services/__tests__/save-actions.test.ts @@ -100,6 +100,23 @@ describe('save-actions', () => { 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', () => { @@ -125,5 +142,21 @@ describe('save-actions', () => { 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) + }) }) }) diff --git a/src/frontend/services/save-actions.ts b/src/frontend/services/save-actions.ts index 723416e55..6e852ddf0 100644 --- a/src/frontend/services/save-actions.ts +++ b/src/frontend/services/save-actions.ts @@ -439,6 +439,8 @@ export async function executeSaveProject( deleted: deletionsBeforeSave, }) + const isStale = new Set(staleFlows) + state.projectActions.clearPendingDeletions() setEditingState(staleFlows.length > 0 ? 'unsaved' : 'saved') setAllToSaved() @@ -449,14 +451,15 @@ export async function executeSaveProject( // 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 (staleFlows.includes(flow.name)) continue + 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 (staleFlows.includes(flow.name)) continue + 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 }) } @@ -512,8 +515,9 @@ export async function executeSaveFile( projectPort: ProjectPort, capabilities: PlatformCapabilities, ): Promise<{ success: boolean }> { - // See executeSaveProject — same pending write-back flush requirement. - const staleFlows = 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) { diff --git a/src/frontend/store/__tests__/shared-slice.test.ts b/src/frontend/store/__tests__/shared-slice.test.ts index f55b72520..87c7f22c0 100644 --- a/src/frontend/store/__tests__/shared-slice.test.ts +++ b/src/frontend/store/__tests__/shared-slice.test.ts @@ -8,6 +8,7 @@ import { createEditorSlice } from '../slices/editor/slice' import { createFBDFlowSlice } from '../slices/fbd/slice' import { createFileSlice } from '../slices/file/slice' import { createHistorySlice } from '../slices/history/slice' +import type { LadderFlowType } from '../slices/ladder' import { createLadderFlowSlice } from '../slices/ladder/slice' import { createLibrarySlice } from '../slices/library/slice' import { createModalSlice } from '../slices/modal/slice' @@ -1104,6 +1105,34 @@ describe('createSharedSlice', () => { expect(store.getState().undoRedo['Main'].past).toHaveLength(0) }) + it('does nothing when the POU flow fails its write-back', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + store.getState().pouActions.create({ type: 'program', name: 'Graphical', language: 'ld' }) + store.getState().snapshotActions.pushToHistory('Graphical', snapshot1) + // `rungs` entries without `defaultBounds` fail the ladder schema, so the + // body stays stale and a snapshot here would pair it with a fresh flow. + store.getState().ladderFlowActions.addLadderFlow({ + name: 'Graphical', + updated: true, + rungs: [{ id: 'r1', comment: '', nodes: [], edges: [] }], + } as unknown as LadderFlowType) + store.getState().ladderFlowActions.setFlowUpdated({ editorName: 'Graphical', updated: true }) + + // `false` is what drives the "History unavailable" toast in the UI. + expect(store.getState().snapshotActions.undo('Graphical')).toBe(false) + expect(store.getState().snapshotActions.redo('Graphical')).toBe(false) + + expect(store.getState().undoRedo['Graphical'].past).toHaveLength(1) + expect(store.getState().undoRedo['Graphical'].future).toHaveLength(0) + warn.mockRestore() + }) + + it('reports success when there is simply nothing to undo', () => { + expect(store.getState().snapshotActions.undo('Main')).toBe(true) + store.getState().snapshotActions.pushToHistory('Main', snapshot1) + expect(store.getState().snapshotActions.undo('Main')).toBe(true) + }) + it('undo falls back to empty array when POU has no interface', () => { // Manually strip the POU interface to test the ?? [] fallback const pous = store.getState().project.data.pous.map((p) => { diff --git a/src/frontend/store/slices/shared/flow-writeback.ts b/src/frontend/store/slices/shared/flow-writeback.ts index 605d54d9e..b439232b0 100644 --- a/src/frontend/store/slices/shared/flow-writeback.ts +++ b/src/frontend/store/slices/shared/flow-writeback.ts @@ -96,17 +96,23 @@ export function scheduleFlowWriteBack(getState: GetWriteBackState, pouName: stri * * @returns names of POUs whose body could not be updated. */ -export function flushFlowWriteBacks(getState: GetWriteBackState, pouName?: string): string[] { +export function flushFlowWriteBacks(getState: GetWriteBackState, pouName?: string): readonly string[] { cancelFlowWriteBacks(pouName) const state = getState() + // Deleting a POU leaves its flow behind, so an invalid one would be reported + // stale on every future save — blocking saves and builds for good. It has no + // body left to write back, so it isn't a write-back failure. + const livePous = new Set(state.project.data.pous.map((pou) => pou.name)) const failed: string[] = [] for (const flow of state.ladderFlows) { if (pouName !== undefined && flow.name !== pouName) continue + if (!livePous.has(flow.name)) continue if (flow.updated && !runWriteBack(getState, flow.name, 'ld')) failed.push(flow.name) } for (const flow of state.fbdFlows) { if (pouName !== undefined && flow.name !== pouName) continue + if (!livePous.has(flow.name)) continue if (flow.updated && !runWriteBack(getState, flow.name, 'fbd')) failed.push(flow.name) } return failed diff --git a/src/frontend/store/slices/shared/slice.ts b/src/frontend/store/slices/shared/slice.ts index 8c07a150f..cc2b78116 100644 --- a/src/frontend/store/slices/shared/slice.ts +++ b/src/frontend/store/slices/shared/slice.ts @@ -993,14 +993,16 @@ const createSharedSlice: StateCreator = (s undo: (pouName) => { // A debounced graphical write-back may still be pending — flush it so // the redo snapshot below can't pair a stale body with a fresh flow. - flushFlowWriteBacks(getState, pouName) + // A failed flush leaves the body stale, and capturing it would restore + // the file to "saved" over content that never reached disk (DOPE-495). + if (flushFlowWriteBacks(getState, pouName).length > 0) return false const state = getState() const history = state.undoRedo[pouName] - if (!history || history.past.length === 0) return + if (!history || history.past.length === 0) return true const snapshot = history.past[history.past.length - 1] const pou = state.project.data.pous.find((p) => p.name === pouName) - if (!pou) return + if (!pou) return true // Save current state to future. Plain references — the store is // immer-managed (frozen, copy-on-write), so later edits can never @@ -1046,18 +1048,19 @@ const createSharedSlice: StateCreator = (s if (afterUndo?.savedAtDepth !== null && afterUndo?.savedAtDepth === afterUndo?.past.length) { getState().fileActions.updateFile({ name: pouName, saved: true }) } + return true }, redo: (pouName) => { // See undo — same pending write-back consistency requirement. - flushFlowWriteBacks(getState, pouName) + if (flushFlowWriteBacks(getState, pouName).length > 0) return false const state = getState() const history = state.undoRedo[pouName] - if (!history || history.future.length === 0) return + if (!history || history.future.length === 0) return true const snapshot = history.future[history.future.length - 1] const pou = state.project.data.pous.find((p) => p.name === pouName) - if (!pou) return + if (!pou) return true // Save current state to past. Plain references — see undo. const currentSnapshot: PouHistorySnapshot = { @@ -1101,6 +1104,7 @@ const createSharedSlice: StateCreator = (s if (afterRedo?.savedAtDepth !== null && afterRedo?.savedAtDepth === afterRedo?.past.length) { getState().fileActions.updateFile({ name: pouName, saved: true }) } + return true }, }, }) diff --git a/src/frontend/store/slices/shared/types.ts b/src/frontend/store/slices/shared/types.ts index f3d3b824c..5025c4129 100644 --- a/src/frontend/store/slices/shared/types.ts +++ b/src/frontend/store/slices/shared/types.ts @@ -118,8 +118,10 @@ export type SnapshotActions = { pushToHistory: (pouName: string, snapshot: PouHistorySnapshot) => void markSaved: (pouName: string) => void markAllSaved: (except?: readonly string[]) => void - undo: (pouName: string) => void - redo: (pouName: string) => void + /** @returns `false` when the POU's graphical body is stale, so history was left untouched. */ + undo: (pouName: string) => boolean + /** @returns `false` when the POU's graphical body is stale, so history was left untouched. */ + redo: (pouName: string) => boolean } export type OpenProjectResponseData = {