diff --git a/docs/workflow-designer-features.md b/docs/workflow-designer-features.md index b41a8a74..7aad04e8 100644 --- a/docs/workflow-designer-features.md +++ b/docs/workflow-designer-features.md @@ -123,7 +123,7 @@ described under [AI features](ai-features.md). | Shortcut | Action | |---|---| | `Ctrl+S` | Save. | -| `Ctrl+E` / `Ctrl+U` | Acquire or release the edit lock. | +| `Ctrl+E` / `Ctrl+U` | Acquire or release the edit lock. Releasing asks before it discards unsaved changes. | | `Ctrl+Shift+S` | Publish, enable or disable, depending on state. | | `Ctrl+Enter` | Run. | | `Ctrl+Shift+Enter` | Debug run, Expert mode. | diff --git a/src/nodepilot-docs-ui/content/de/designer/overview.md b/src/nodepilot-docs-ui/content/de/designer/overview.md index eeac4537..5aef0511 100644 --- a/src/nodepilot-docs-ui/content/de/designer/overview.md +++ b/src/nodepilot-docs-ui/content/de/designer/overview.md @@ -28,7 +28,7 @@ Features, die nur im Expert-Modus sichtbar sind, sind in den folgenden Seiten mi - `canWrite = roleCanWrite && isLockedByMe` — beide Bedingungen nötig. - **Lock** `Ctrl+E` → atomar `IsEnabled=false` + Lock-Fields. 409 wenn schon gelockt. -- **Unlock** `Ctrl+U` → Lock freigeben (`IsEnabled` bleibt). +- **Unlock** `Ctrl+U` → Lock freigeben (`IsEnabled` bleibt). Ungespeicherte Änderungen werden nach Rückfrage verworfen, der Canvas zeigt danach den gespeicherten Stand. Dasselbe gilt für das Wiederherstellen einer Version im Diff-Dialog. - **Publish** `Ctrl+Shift+S` → atomar Save + Enable + Unlock (durch Pre-Publish-Checklist). - **Disable/Enable** — Kill-Switch (Disable **nicht** lock-gegated). - **Force-Unlock** (Admin) `Ctrl+Shift+U` — bricht fremden Lock. diff --git a/src/nodepilot-docs-ui/content/en/designer/overview.md b/src/nodepilot-docs-ui/content/en/designer/overview.md index 488e5490..8a9947ae 100644 --- a/src/nodepilot-docs-ui/content/en/designer/overview.md +++ b/src/nodepilot-docs-ui/content/en/designer/overview.md @@ -28,7 +28,7 @@ Features that are only visible in expert mode are marked **(Expert)** on the fol - `canWrite = roleCanWrite && isLockedByMe` — both conditions are required. - **Lock** `Ctrl+E` → atomically sets `IsEnabled=false` + the lock fields. 409 if already locked. -- **Unlock** `Ctrl+U` → releases the lock (`IsEnabled` stays as it is). +- **Unlock** `Ctrl+U` → releases the lock (`IsEnabled` stays as it is). Unsaved changes are discarded after a confirmation, and the canvas then shows the saved version. Restoring a version from the diff dialog does the same. - **Publish** `Ctrl+Shift+S` → atomically save + enable + unlock (through the pre-publish checklist). - **Disable/Enable** — the kill switch (disable is **not** lock-gated). - **Force unlock** (admin) `Ctrl+Shift+U` — breaks someone else's lock. diff --git a/src/nodepilot-ui/src/__tests__/components/designer/EditorHeader.test.tsx b/src/nodepilot-ui/src/__tests__/components/designer/EditorHeader.test.tsx index 1e15ba7f..ea1fe672 100644 --- a/src/nodepilot-ui/src/__tests__/components/designer/EditorHeader.test.tsx +++ b/src/nodepilot-ui/src/__tests__/components/designer/EditorHeader.test.tsx @@ -61,6 +61,7 @@ function defaultProps(over: Partial = {}): EditorHeaderProps { save: vi.fn(), lock: vi.fn(), unlock: vi.fn(), + replaceDraft: vi.fn(), forceUnlock: vi.fn(), disable: vi.fn(), requestPublish: vi.fn(), diff --git a/src/nodepilot-ui/src/__tests__/components/overlays/WorkflowDiffModal.test.tsx b/src/nodepilot-ui/src/__tests__/components/overlays/WorkflowDiffModal.test.tsx index b9927d02..b500faf5 100644 --- a/src/nodepilot-ui/src/__tests__/components/overlays/WorkflowDiffModal.test.tsx +++ b/src/nodepilot-ui/src/__tests__/components/overlays/WorkflowDiffModal.test.tsx @@ -35,7 +35,13 @@ function edge(id: string, source: string, target: string, label = 'On Success'): return { id, source, target, type: 'labeled', data: { label } }; } -function renderModal(currentNodes: Node[], currentEdges: Edge[], opts: { canRestore?: boolean; onClose?: () => void } = {}) { +const runOperation = async (operation: () => Promise) => { await operation(); }; + +function renderModal(currentNodes: Node[], currentEdges: Edge[], opts: { + canRestore?: boolean; + onClose?: () => void; + replaceDraft?: (operation: () => Promise) => Promise; +} = {}) { patchFetch(); const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); return render( @@ -44,6 +50,7 @@ function renderModal(currentNodes: Node[], currentEdges: Edge[], opts: { canRest workflowId="wf-1" currentDefinition={{ nodes: currentNodes, edges: currentEdges }} canRestore={opts.canRestore} + replaceDraft={opts.replaceDraft ?? runOperation} onClose={opts.onClose ?? vi.fn()} /> @@ -232,6 +239,23 @@ describe('WorkflowDiffModal', () => { expect(onClose).toHaveBeenCalledOnce(); }); + it('restore_runsTheRollbackThroughReplaceDraft', async () => { + const replaceDraft = vi.fn(runOperation); + server.use( + http.get(`${BASE}/api/workflows/wf-1/versions`, () => + HttpResponse.json([{ version: 2, isCurrent: true }, { version: 1, isCurrent: false }])), + http.get(`${BASE}/api/workflows/wf-1/versions/1`, () => + HttpResponse.json({ definition: { nodes: [], edges: [] } })), + http.post(`${BASE}/api/workflows/wf-1/rollback/1`, () => HttpResponse.json({ ok: true })), + ); + + renderModal([], [], { canRestore: true, replaceDraft }); + fireEvent.click(await screen.findByText('Version 1')); + fireEvent.click(await screen.findByRole('button', { name: /Restore v1/ })); + + await waitFor(() => expect(replaceDraft).toHaveBeenCalledOnce()); + }); + it('closeButton_callsOnClose', async () => { server.use(http.get(`${BASE}/api/workflows/wf-1/versions`, () => HttpResponse.json([]))); const onClose = vi.fn(); @@ -239,7 +263,7 @@ describe('WorkflowDiffModal', () => { const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); render( - + ); diff --git a/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx b/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx index a5066302..8290f692 100644 --- a/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx +++ b/src/nodepilot-ui/src/__tests__/hooks/useWorkflowPersistence.test.tsx @@ -290,6 +290,40 @@ describe('useWorkflowPersistence — revision-safe requests', () => { expect(api.post).not.toHaveBeenCalled(); }); + it('replaceDraft waits for the running save, skips its follow-up and then drops the draft', async () => { + const saving = deferred(); + vi.mocked(api.put).mockImplementationOnce(() => saving.promise); + const operation = vi.fn().mockResolvedValue(undefined); + const { result } = renderPersistence(); + act(() => { result.current.syncFromServer('First', 4); result.current.markDirty(); result.current.save(); }); + await waitFor(() => expect(api.put).toHaveBeenCalledTimes(1)); + // A newer edit would normally trigger a follow-up save of the same draft. + act(() => result.current.markDirty()); + + let replaced!: Promise; + act(() => { replaced = result.current.replaceDraft(operation); }); + expect(operation).not.toHaveBeenCalled(); + await act(async () => { saving.resolve({}); await replaced; }); + + expect(operation).toHaveBeenCalledTimes(1); + expect(api.put).toHaveBeenCalledTimes(1); + expect(result.current.isDirty).toBe(false); + expect(result.current.syncFromServer('Server state', 4)).toBe(true); + }); + + it('replaceDraft keeps the draft dirty when the operation fails', async () => { + const { result } = renderPersistence(); + act(() => { result.current.syncFromServer('First', 4); result.current.markDirty(); }); + + await act(async () => { + await expect(result.current.replaceDraft(() => Promise.reject(new Error('unlock failed')))) + .rejects.toThrow('unlock failed'); + }); + + expect(result.current.isDirty).toBe(true); + expect(result.current.syncFromServer('Server state', 4)).toBe(false); + }); + it('drops queued Publish when the editor unmounts while its save is pending', async () => { const saving = deferred(); vi.mocked(api.put).mockImplementationOnce(() => saving.promise); diff --git a/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx b/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx index 08b6ba5e..97c6480d 100644 --- a/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx +++ b/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx @@ -2158,6 +2158,60 @@ describe('WorkflowEditorPage — characterization (autosave + run + unload)', () await waitFor(() => expect(putCalled).toBe(true)); }); + it('End editing on a dirty draft discards it and shows the saved workflow', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + let putCalled = false; + server.use( + http.put(`${BASE}/api/workflows/wf-smoke-1`, async () => { putCalled = true; return HttpResponse.json(MOCK_WORKFLOW); }), + http.post(`${BASE}/api/workflows/wf-smoke-1/unlock`, () => HttpResponse.json(MOCK_DISABLED)), + ); + renderPage(); + await waitFor(() => expect(screen.getByDisplayValue('Smoke Workflow')).toBeInTheDocument()); + fireEvent.change(screen.getByDisplayValue('Smoke Workflow'), { target: { value: 'Discarded draft' } }); + + server.use(http.get(`${BASE}/api/workflows/wf-smoke-1`, () => HttpResponse.json(MOCK_DISABLED))); + fireEvent.click(screen.getByRole('button', { name: /^End/i })); + + await waitFor(() => expect(screen.getByDisplayValue('Smoke Workflow')).toBeInTheDocument()); + await act(async () => { vi.advanceTimersByTime(6000); }); + expect(putCalled).toBe(false); + const unload = new Event('beforeunload', { cancelable: true }); + globalThis.dispatchEvent(unload); + expect(unload.defaultPrevented).toBe(false); + }); + + it('restoring a version over a dirty draft adopts the restored version and never saves the draft', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + const putNames: string[] = []; + const restored = { ...MOCK_WORKFLOW, name: 'Restored v1', version: 3 }; + server.use( + http.put(`${BASE}/api/workflows/wf-smoke-1`, async ({ request }) => { + putNames.push(((await request.json()) as { name: string }).name); + return new HttpResponse(null, { status: 204 }); + }), + http.get(`${BASE}/api/workflows/wf-smoke-1/versions`, () => + HttpResponse.json([{ version: 2, isCurrent: true }, { version: 1, isCurrent: false }])), + http.get(`${BASE}/api/workflows/wf-smoke-1/versions/1`, () => + HttpResponse.json({ definition: { nodes: [], edges: [] } })), + http.post(`${BASE}/api/workflows/wf-smoke-1/rollback/1`, () => { + server.use(http.get(`${BASE}/api/workflows/wf-smoke-1`, () => HttpResponse.json(restored))); + return HttpResponse.json(restored); + }), + ); + renderPage(); + await waitFor(() => expect(screen.getByDisplayValue('Smoke Workflow')).toBeInTheDocument()); + fireEvent.change(screen.getByDisplayValue('Smoke Workflow'), { target: { value: 'Unsaved draft' } }); + + await openToolsMenu(); + fireEvent.click(screen.getByTitle(/Diff against/)); + fireEvent.click(await screen.findByText('Version 1')); + fireEvent.click(await screen.findByRole('button', { name: /Restore v1/ })); + + await waitFor(() => expect(screen.getByDisplayValue('Restored v1')).toBeInTheDocument()); + await act(async () => { vi.advanceTimersByTime(6000); }); + expect(putNames).not.toContain('Unsaved draft'); + }); + it('Test on a dirty, writable workflow saves (PUT) before calling /execute', async () => { const lockedEnabled = { ...MOCK_WORKFLOW, isEnabled: true }; // locked-by-test-user + enabled const order: string[] = []; diff --git a/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx b/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx index 719c50c2..41f76c5b 100644 --- a/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx +++ b/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx @@ -308,6 +308,7 @@ export function EditorOverlays({ workflowId={id} currentDefinition={{ nodes, edges }} canRestore={canWrite} + replaceDraft={lifecycle.replaceDraft} onClose={() => setDiffOpen(false)} /> )} diff --git a/src/nodepilot-ui/src/components/designer/header/LifecycleControls.tsx b/src/nodepilot-ui/src/components/designer/header/LifecycleControls.tsx index 8c41a22c..67ac4507 100644 --- a/src/nodepilot-ui/src/components/designer/header/LifecycleControls.tsx +++ b/src/nodepilot-ui/src/components/designer/header/LifecycleControls.tsx @@ -36,10 +36,7 @@ export function LifecycleControls({ workflow, lifecycle }: Readonly { - if (isDirty && !(await confirmDialog(t('editor:endEditingDirtyConfirm')))) return; - onUnlock(); - }} + onClick={onUnlock} disabled={isUnlocking} className="flex items-center justify-center rounded-md h-9 w-9 bg-transparent hover:bg-surface-high text-on-surface-variant transition-colors disabled:opacity-50" title={t('editor:endEditingTitleFull')} diff --git a/src/nodepilot-ui/src/components/designer/overlays/WorkflowDiffModal.tsx b/src/nodepilot-ui/src/components/designer/overlays/WorkflowDiffModal.tsx index 68c25add..bd17330a 100644 --- a/src/nodepilot-ui/src/components/designer/overlays/WorkflowDiffModal.tsx +++ b/src/nodepilot-ui/src/components/designer/overlays/WorkflowDiffModal.tsx @@ -21,11 +21,14 @@ type WorkflowVersionRow = { * Version diff. User picks a historical version from the timeline; the modal compares * that saved definition with the current working copy. The actual diff rendering is * delegated to the shared, ID-stable {@link DefinitionDiffViewer} (handles/positions-aware). + * A restore runs through `replaceDraft`, so the editor's draft is replaced by the restored + * version instead of being autosaved over it. */ -export function WorkflowDiffModal({ workflowId, currentDefinition, canRestore = false, onClose }: Readonly<{ +export function WorkflowDiffModal({ workflowId, currentDefinition, canRestore = false, replaceDraft, onClose }: Readonly<{ workflowId: string; currentDefinition: { nodes: Node[]; edges: Edge[] }; canRestore?: boolean; + replaceDraft: (operation: () => Promise) => Promise; onClose: () => void; }>) { const { t } = useTranslation('editor'); @@ -53,7 +56,8 @@ export function WorkflowDiffModal({ workflowId, currentDefinition, canRestore = const rollbackMutation = useMutation({ mutationFn: async (version: number) => { setRollbackError(null); - await api.post(`/workflows/${workflowId}/rollback/${version}`, { reason: t('diff.rollbackReason', { version }) }); + await replaceDraft(() => + api.post(`/workflows/${workflowId}/rollback/${version}`, { reason: t('diff.rollbackReason', { version }) })); }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['workflow-versions', workflowId] }); diff --git a/src/nodepilot-ui/src/hooks/useEditorLifecycle.ts b/src/nodepilot-ui/src/hooks/useEditorLifecycle.ts index a3b46c8a..b4fc9cf1 100644 --- a/src/nodepilot-ui/src/hooks/useEditorLifecycle.ts +++ b/src/nodepilot-ui/src/hooks/useEditorLifecycle.ts @@ -4,6 +4,7 @@ import type { Workflow } from '../types/api'; import { lintWorkflow, type LintResult } from '../lib/workflowLint'; import { getPrePublishLint } from '../lib/prePublishChecks'; import { confirmDialog } from '../stores/confirmStore'; +import { toast } from '../stores/toastStore'; import type { useWorkflowLock } from './useWorkflowLock'; import type { useWorkflowPersistence } from './useWorkflowPersistence'; @@ -31,7 +32,10 @@ export interface EditorLifecycle { isForceUnlocking: boolean; save: () => void; lock: () => void; + /** Ends editing: asks before discarding a dirty draft, then unlocks and adopts the server state. */ unlock: () => void; + /** See useWorkflowPersistence.replaceDraft; used by version restore. */ + replaceDraft: (operation: () => Promise) => Promise; forceUnlock: () => void; disable: () => void; /** @@ -48,7 +52,7 @@ interface UseEditorLifecycleArgs { canWrite: boolean; isTidying: boolean; lockState: ReturnType; - persistence: Pick, 'isDirty' | 'isSaving' | 'isPublishing' | 'save' | 'publish'>; + persistence: Pick, 'isDirty' | 'isSaving' | 'isPublishing' | 'save' | 'publish' | 'replaceDraft'>; /** The graph as it is right now, for the pre-publish lint. */ getLiveGraph: () => { nodes: Node[]; edges: Edge[] }; allWorkflows: Array<{ id: string; name: string }> | undefined; @@ -66,10 +70,19 @@ export function useEditorLifecycle({ }: UseEditorLifecycleArgs) { const { t } = useTranslation(['editor', 'common']); const { - isLockedByMe, isLockedByOther, lock, unlock, forceUnlock, disable, enable, + isLockedByMe, isLockedByOther, lock, unlockAsync, forceUnlock, disable, enable, isLocking, isUnlocking, isForceUnlocking, isDisabling, isEnabling, } = lockState; - const { isDirty, isSaving, isPublishing, save, publish } = persistence; + const { isDirty, isSaving, isPublishing, save, publish, replaceDraft } = persistence; + + const endEditing = async () => { + if (isDirty && !(await confirmDialog(t('editor:endEditingDirtyConfirm')))) return; + try { + await replaceDraft(unlockAsync); + } catch (err) { + toast.error((err as Error).message); + } + }; // On a productive workflow the toggle is a kill switch and stays direct: stopping production // is not an "is everything ready" question. @@ -107,7 +120,8 @@ export function useEditorLifecycle({ isUnlocking: isUnlocking || isTidying, isPublishing: isPublishing || isTidying, isEnabling, isDisabling, isForceUnlocking, - save, lock, unlock, forceUnlock, disable, + save, lock, forceUnlock, disable, replaceDraft, + unlock: () => { void endEditing(); }, requestPublish: () => { void requestPublish(); }, }; @@ -121,7 +135,7 @@ export function useEditorLifecycle({ if (roleCanWrite && !isLockedByMe && !isLockedByOther && !isLocking) lock(); }, triggerUnlock: () => { - if (isLockedByMe && !isUnlocking && !isTidying) unlock(); + if (isLockedByMe && !isUnlocking && !isTidying) void endEditing(); }, triggerForceUnlock: async () => { if (!isAdmin || !isLockedByOther || isForceUnlocking) return; diff --git a/src/nodepilot-ui/src/hooks/useWorkflowLock.ts b/src/nodepilot-ui/src/hooks/useWorkflowLock.ts index df2d6b60..c8759bce 100644 --- a/src/nodepilot-ui/src/hooks/useWorkflowLock.ts +++ b/src/nodepilot-ui/src/hooks/useWorkflowLock.ts @@ -41,7 +41,7 @@ export function useWorkflowLock({ workflowId, workflow, currentUserId, roleCanWr const canWrite = roleCanWrite && isLockedByMe; const lock = useCallback(() => { lockMutation.mutate(); }, [lockMutation]); - const unlock = useCallback(() => { unlockMutation.mutate(); }, [unlockMutation]); + const unlockAsync = useCallback(() => unlockMutation.mutateAsync(), [unlockMutation]); const forceUnlock = useCallback(() => { forceUnlockMutation.mutate(); }, [forceUnlockMutation]); const disable = useCallback(() => { disableMutation.mutate(); }, [disableMutation]); const enable = useCallback(() => { enableMutation.mutate(); }, [enableMutation]); @@ -52,7 +52,7 @@ export function useWorkflowLock({ workflowId, workflow, currentUserId, roleCanWr isLockedByOther, canWrite, lock, - unlock, + unlockAsync, forceUnlock, disable, enable, diff --git a/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts b/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts index bd4a28b7..b4f8126f 100644 --- a/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts +++ b/src/nodepilot-ui/src/hooks/useWorkflowPersistence.ts @@ -63,6 +63,8 @@ export function useWorkflowPersistence({ const dirtyRef = useRef(false); const savingRef = useRef(false); const publishRef = useRef(false); + // True while replaceDraft runs: no autosave and no follow-up save may write the old draft. + const holdSavesRef = useRef(false); const syncedWorkflowIdRef = useRef(undefined); const renderedWorkflowIdRef = useRef(workflowId); const draftGenerationRef = useRef(0); @@ -143,6 +145,7 @@ export function useWorkflowPersistence({ // A route change owns a different draft. Never carry a follow-up across it. if (draftRef.current.workflowId !== snapshot.workflowId) return response; + if (holdSavesRef.current) return response; if (revisionRef.current === snapshot.revision) { updateDirty(false); return response; @@ -183,7 +186,9 @@ export function useWorkflowPersistence({ useEffect(() => { if (!isDirty || isSaving || isPublishQueued || hasVersionConflict || suspendAutoSave || !workflowId) return; if (autosaveTimer.current) clearTimeout(autosaveTimer.current); - autosaveTimer.current = setTimeout(() => { void saveLatest().catch(() => undefined); }, 5000); + autosaveTimer.current = setTimeout(() => { + if (!holdSavesRef.current) void saveLatest().catch(() => undefined); + }, 5000); return () => { if (autosaveTimer.current) clearTimeout(autosaveTimer.current); }; }, [isDirty, isSaving, isPublishQueued, hasVersionConflict, suspendAutoSave, nodes, edges, name, workflowId, saveLatest]); @@ -253,6 +258,26 @@ export function useWorkflowPersistence({ return true; }, [updateDirty]); + /** + * Runs a server operation that replaces the draft (end editing, version restore). Autosave and + * follow-up saves stop first and a running save is awaited, so the old draft cannot land after + * the operation. Only on success is the draft dropped, which lets the next server sync replace + * the canvas; on failure the draft stays dirty and the next edit or Save writes it. + */ + const replaceDraft = useCallback(async (operation: () => Promise): Promise => { + holdSavesRef.current = true; + if (autosaveTimer.current) clearTimeout(autosaveTimer.current); + try { + if (saveLoopRef.current) await saveLoopRef.current.catch(() => undefined); + await operation(); + draftGenerationRef.current += 1; + revisionRef.current += 1; + updateDirty(false); + } finally { + holdSavesRef.current = false; + } + }, [updateDirty]); + const save = useCallback(() => { void saveLatest().catch(() => undefined); }, [saveLatest]); const saveAsync = useCallback(() => saveLatest(), [saveLatest]); @@ -322,6 +347,7 @@ export function useWorkflowPersistence({ save, saveAsync, publish, + replaceDraft, syncFromServer, }; }