From c9ddaacae8fc054f947317efd7c7c1272ffdee8b Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:24:47 +0100 Subject: [PATCH 1/4] Keep workflow references and aliases intact in designer edits Renaming an output variable now rewrites every reference through one traversal (templates in any field, dynamic machine/credential bindings, breakpoint conditions, decision cases and edge conditions). Copy, paste and duplicate allocate a fresh alias per copy and rewrite the copy's own references. The server rejects two active nodes with the same alias, case-insensitively, and ignores disabled nodes when indexing aliases. Inserting an activity on a conditional edge keeps the guard in front of the new activity. Command-palette edge deletion goes through the normal delete command, and property edits snapshot history before the first change of a typing burst. Clipboard copies also mask recognised secrets in edge data and string arrays. Closes #689, #703, #693, #694, #595. Refs #574. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 15 +++++ .../WorkflowDefinitions/WorkflowAnalyzer.cs | 25 ------- .../WorkflowDefinitionDocument.cs | 8 ++- .../WorkflowDefinitionStructuralValidator.cs | 9 ++- src/nodepilot-ui/CLAUDE.md | 2 + .../__tests__/hooks/useCanvasConnect.test.tsx | 7 +- .../hooks/useWorkflowHistory.test.tsx | 20 ++++++ .../__tests__/lib/editorGraphHelpers.test.ts | 26 +++---- .../__tests__/lib/workflowFragment.test.ts | 34 ++++++++++ .../pages/WorkflowEditorPage.test.tsx | 44 ++++++++++++ .../components/designer/EditorOverlays.tsx | 5 +- .../src/hooks/useCanvasConnect.ts | 8 +-- .../src/hooks/useNodeOperations.ts | 16 ++--- .../src/hooks/useWorkflowClipboard.ts | 3 +- .../src/hooks/useWorkflowHistory.ts | 31 +++++++-- .../src/lib/editorGraphHelpers.ts | 39 ++--------- src/nodepilot-ui/src/lib/workflowFragment.ts | 60 +++++++---------- .../src/lib/workflowReferences.ts | 67 +++++++++++++++++++ .../src/pages/WorkflowEditorPage.tsx | 23 ++----- .../WorkflowAnalyzerTests.cs | 25 +++++++ 20 files changed, 314 insertions(+), 153 deletions(-) create mode 100644 src/nodepilot-ui/src/lib/workflowReferences.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 18978165..468622bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,22 @@ Planned version: **1.5.4**. own session file, so profiles such as `prod.a` and `prod_a` no longer share one login (#620). - A `matches` condition with an invalid pattern, a regex timeout or an input longer than 1,048,576 characters fails the run and names the edge, instead of counting as "no match" (#680). +- Two active nodes can no longer share an output variable, regardless of case: saving, importing + or an MCP change with such a pair is rejected. A disabled node is ignored, so a disabled copy can + no longer take over the alias of the active one (#703). ### Fixed +- **Designer** + - Renaming an output variable updates every reference to it: templates in any field, a dynamic + target machine or credential, breakpoint conditions, decision cases and edge conditions (#689). + - Copying, pasting and duplicating give each copy its own output variable (`name_2`) and rewrite + the references inside the copy, so it no longer takes over the original's bindings (#703). + - Inserting an activity on a conditional edge keeps the condition in front of the new activity, + so the branch still runs only when the condition holds (#693). + - Deleting an edge from the command palette marks the workflow as changed and can be undone + (#694). + - Undo right after typing in a property field restores the value from before the edit (#595). - **Running workflows** - A breakpoint condition can use the run's inputs (`{{manual.*}}`) (#685). - Input values that look like `{{…}}` stay plain text and no longer fail the step as unresolved @@ -66,6 +79,8 @@ Planned version: **1.5.4**. ### Security +- Copying nodes to the clipboard also masks recognised secrets in edge conditions and in lists, not + only in node fields (#574). - The CLI, the MCP server and the Python admin scripts under `scripts/` no longer follow redirects with the login token, so the token is never sent to another host (#711). - Logging out with the CLI also ends the MCP server's session right away (#712). diff --git a/src/NodePilot.Core/WorkflowDefinitions/WorkflowAnalyzer.cs b/src/NodePilot.Core/WorkflowDefinitions/WorkflowAnalyzer.cs index 15816eca..668c6266 100644 --- a/src/NodePilot.Core/WorkflowDefinitions/WorkflowAnalyzer.cs +++ b/src/NodePilot.Core/WorkflowDefinitions/WorkflowAnalyzer.cs @@ -69,7 +69,6 @@ public static AnalysisResult Analyze(JsonElement definition) if (FindCycle(doc.Adjacency) is { Count: > 0 } cycle) findings.Add(new Finding("error", "cycle", null, $"Cycle detected: {string.Join(" → ", cycle)} → {cycle[0]}. The engine has no inDegree fallback; cyclic graphs Fail.")); - AddDuplicateOutputVariableFindings(doc, findings); AddDuplicatePublishedParamFindings(doc, findings); AddUnresolvedReferenceFindings(doc, definition, findings); @@ -278,30 +277,6 @@ private static void AddUnresolvedReferenceFindings( } } - private static void AddDuplicateOutputVariableFindings(WorkflowDefinitionDocument doc, List findings) - { - var seen = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var node in doc.Nodes) - { - if (doc.DisabledNodeIds.Contains(node.Id)) continue; - if (IsAnnotation(node.Type)) continue; - var outputVariable = node.Data.OutputVariable; - if (string.IsNullOrWhiteSpace(outputVariable)) continue; - - if (seen.TryGetValue(outputVariable, out var firstNodeId)) - { - findings.Add(new Finding( - "error", - "dup-output-variable", - node.Id, - $"outputVariable '{outputVariable}' is already used by node '{firstNodeId}' — downstream references {{{{{outputVariable}.output}}}} are ambiguous.")); - } - else - { - seen[outputVariable] = node.Id; - } - } - } private static void AddStartJobRunspaceFindings(WorkflowDefinitionDocument doc, List findings) { diff --git a/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionDocument.cs b/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionDocument.cs index 06b9b4f9..7d0fbb4f 100644 --- a/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionDocument.cs +++ b/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionDocument.cs @@ -192,8 +192,12 @@ public static Dictionary BuildOutputVariableAliasMap(IReadOnlyLi var dict = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var node in allNodes) { - if (node.Data.OutputVariable is { Length: > 0 } variable && variable != node.Id) - dict[variable] = node.Id; + if (node.Data.Disabled) continue; + if (node.Data.OutputVariable is { Length: > 0 } variable) + { + if (!dict.TryAdd(variable, node.Id)) + throw new InvalidOperationException($"Duplicate active output variable '{variable}'."); + } } return dict; diff --git a/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionStructuralValidator.cs b/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionStructuralValidator.cs index 335f1c13..3e05d124 100644 --- a/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionStructuralValidator.cs +++ b/src/NodePilot.Core/WorkflowDefinitions/WorkflowDefinitionStructuralValidator.cs @@ -18,7 +18,7 @@ public static WorkflowDefinitionValidationResult Validate(JsonElement definition return WorkflowDefinitionValidationResult.Invalid("root must be a JSON object"); var nodeIds = new HashSet(StringComparer.Ordinal); - var outputVariables = new HashSet(StringComparer.Ordinal); + var outputVariables = new HashSet(StringComparer.OrdinalIgnoreCase); var activityTypeByNodeId = new Dictionary(StringComparer.Ordinal); if (definition.TryGetProperty("nodes", out var nodes)) { @@ -162,8 +162,11 @@ private static WorkflowDefinitionValidationResult ValidateNode( return WorkflowDefinitionValidationResult.Invalid(error!); } - if (data.TryGetProperty("outputVariable", out var outputVariable) && IsNonEmptyString(outputVariable)) - outputVariables.Add(outputVariable.GetString()!); + var disabled = data.TryGetProperty("disabled", out var disabledValue) && disabledValue.ValueKind == JsonValueKind.True; + if (!disabled && data.TryGetProperty("outputVariable", out var outputVariable) && IsNonEmptyString(outputVariable) + && !outputVariables.Add(outputVariable.GetString()!)) + return WorkflowDefinitionValidationResult.Invalid( + $"duplicate active output variable '{outputVariable.GetString()}'", "dup-output-variable", id); return WorkflowDefinitionValidationResult.Valid; } diff --git a/src/nodepilot-ui/CLAUDE.md b/src/nodepilot-ui/CLAUDE.md index 532a62bf..edafb49e 100644 --- a/src/nodepilot-ui/CLAUDE.md +++ b/src/nodepilot-ui/CLAUDE.md @@ -13,6 +13,8 @@ Applies to `src/nodepilot-ui/`. Project-wide rules are in the root `CLAUDE.md`. - UI strings via `react-i18next` (namespaces per `src/i18n/locales/{de,en}/`, default DE) — new visible strings in **both** languages. - **Colours always via design tokens/CSS variables** — never hardcode Tailwind colour literals (`text-gray-900`, `bg-white` break the dark skins). Native `` value (yyyy-MM-dd) -> ISO UTC midnight; empty stays null. */ const toIsoExpiry = (d: string) => (d ? `${d}T00:00:00Z` : null); @@ -69,7 +71,7 @@ export function CredentialsPage() { }); const createMutation = useMutation({ - mutationFn: () => + mutationFn: (_current: () => boolean) => api.post('/credentials', { name: form.name, username: form.username, @@ -77,8 +79,10 @@ export function CredentialsPage() { domain: form.domain || null, expiresAt: toIsoExpiry(form.expiresAt), }), - onSuccess: () => { + onSuccess: (_result, current) => { queryClient.invalidateQueries({ queryKey: ['credentials'] }); + // A response for a closed panel must not close the record opened after it. + if (!current()) return; setShowCreate(false); setForm(emptyForm); }, @@ -86,7 +90,7 @@ export function CredentialsPage() { }); const updateMutation = useMutation({ - mutationFn: (id: string) => + mutationFn: ({ id }: { id: string; current: () => boolean }) => api.put(`/credentials/${id}`, { name: form.name, username: form.username, @@ -95,16 +99,19 @@ export function CredentialsPage() { domain: form.domain || null, expiresAt: resolveExpiry(form.expiresAt), }), - onSuccess: () => { + onSuccess: (_result, request) => { queryClient.invalidateQueries({ queryKey: ['credentials'] }); + toast.success(t('credentials:saved')); + if (!request.current()) return; setEditingId(null); setForm(emptyForm); setOriginalExpiresAt(null); - toast.success(t('credentials:saved')); }, onError: (err: Error) => toast.error(t('common:saveFailed', { message: err.message })), }); + const saving = createMutation.isPending || updateMutation.isPending; + const deleteMutation = useMutation({ mutationFn: (id: string) => api.delete(`/credentials/${id}`), onSuccess: () => queryClient.invalidateQueries({ queryKey: ['credentials'] }), @@ -134,7 +141,8 @@ export function CredentialsPage() { aria-label={t(editingId !== null ? 'credentials:edit' : 'credentials:addCredential')} className="bg-surface-low rounded-md p-3 mb-4 space-y-3" > -
+ {/* Locked while saving, so the response never discards edits made after the submit. */} +
- +
)} {showDialog && ( - setShowDialog(false)} maxWidth="max-w-3xl"> +

{form.id ? t('customActivities:editTitle') : t('customActivities:create')}

@@ -489,8 +496,8 @@ export function CustomActivitiesPage() {
- - + diff --git a/src/nodepilot-ui/src/pages/GlobalVariablesPage.tsx b/src/nodepilot-ui/src/pages/GlobalVariablesPage.tsx index a6d11975..9b007b49 100644 --- a/src/nodepilot-ui/src/pages/GlobalVariablesPage.tsx +++ b/src/nodepilot-ui/src/pages/GlobalVariablesPage.tsx @@ -4,6 +4,7 @@ import { useEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { useColumnResize } from '../hooks/useColumnResize'; import { api } from '../api/client'; +import { useAsyncIdentity } from '../hooks/useAsyncIdentity'; import { DependencyLink } from '../components/dependencies/DependencyLink'; import { ModalShell } from '../components/common/ModalShell'; import { MobileCardList } from '../components/common/MobileCardList'; @@ -80,6 +81,7 @@ export function GlobalVariablesPage() { const [selectedFolderId, setSelectedFolderId] = useState(ROOT_FOLDER_ID); const [form, setForm] = useState(() => emptyForm(ROOT_FOLDER_ID)); const [showDialog, setShowDialog] = useState(false); + const captureSession = useAsyncIdentity(showDialog ? form.id ?? '' : null); const [search, setSearch] = useState(''); const [sortBy, setSortBy] = useState('name'); @@ -122,7 +124,7 @@ export function GlobalVariablesPage() { }); const saveMutation = useMutation({ - mutationFn: async (f: FormState) => { + mutationFn: async ({ form: f }: { form: FormState; current: () => boolean }) => { if (f.id) { // For secret variables where the value wasn't re-entered, send null so the server // keeps the existing ciphertext. Any other case ships the current value. @@ -144,10 +146,11 @@ export function GlobalVariablesPage() { }); } }, - onSuccess: () => { + onSuccess: (_result, request) => { queryClient.invalidateQueries({ queryKey: ['global-variables'] }); queryClient.invalidateQueries({ queryKey: ['global-folders'] }); - setShowDialog(false); + // A closed dialog must not close the one opened after it. + if (request.current()) setShowDialog(false); }, onError: (err: Error) => toast.error(t('common:saveFailed', { message: err.message })), }); @@ -693,7 +696,8 @@ export function GlobalVariablesPage() { {form.id ? t('globals:editTitle') : t('globals:createTitle')} -
+ {/* Locked while saving, so the response never discards edits made after the submit. */} +
-
+
@@ -764,7 +759,9 @@ function MachineDialog({ onSaved: () => void; }>) { const { t } = useTranslation(['machines', 'common']); + const queryClient = useQueryClient(); const isEdit = !!machine; + const captureSession = useAsyncIdentity(machine?.id); const [name, setName] = useState(machine?.name ?? ''); const [hostname, setHostname] = useState(machine?.hostname ?? ''); @@ -794,6 +791,7 @@ function MachineDialog({ } setError(null); setPending(true); + const current = captureSession(); try { const body = { name: name.trim(), @@ -808,8 +806,11 @@ function MachineDialog({ } else { await api.post('/machines', body); } - onSaved(); + queryClient.invalidateQueries({ queryKey: ['machines'] }); + // A closed dialog must not close the one opened after it. + if (current()) onSaved(); } catch (err) { + if (!current()) return; setError((err as Error).message); setPending(false); } @@ -831,7 +832,8 @@ function MachineDialog({
-
+ {/* Locked while saving, so the response never discards edits made after the submit. */} +
-
+ {error &&

{error}

} diff --git a/src/nodepilot-ui/src/pages/MaintenanceWindowsPage.tsx b/src/nodepilot-ui/src/pages/MaintenanceWindowsPage.tsx index 871500a3..56c43500 100644 --- a/src/nodepilot-ui/src/pages/MaintenanceWindowsPage.tsx +++ b/src/nodepilot-ui/src/pages/MaintenanceWindowsPage.tsx @@ -12,6 +12,7 @@ import { useCallback, useMemo, useState } from 'react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { api } from '../api/client'; +import { useAsyncIdentity } from '../hooks/useAsyncIdentity'; import { ModalShell } from '../components/common/ModalShell'; import { MobileCardList } from '../components/common/MobileCardList'; import { sharedFoldersApi } from '../api/sharedFolders'; @@ -118,6 +119,7 @@ export function MaintenanceWindowsPage() { const isMobile = useIsMobile(); const [form, setForm] = useState(emptyForm); const [showDialog, setShowDialog] = useState(false); + const captureSession = useAsyncIdentity(showDialog ? form.id ?? '' : null); const [search, setSearch] = useState(''); const { data: windows, isLoading } = useQuery({ @@ -152,13 +154,15 @@ export function MaintenanceWindowsPage() { }); const saveMutation = useMutation({ - mutationFn: async (f: FormState) => { + mutationFn: async ({ form: f }: { form: FormState; current: () => boolean }) => { const body = buildBody(f); if (f.id) await api.put(`/maintenance-windows/${f.id}`, body); else await api.post('/maintenance-windows', body); }, - onSuccess: () => { + onSuccess: (_result, request) => { queryClient.invalidateQueries({ queryKey: ['maintenance-windows'] }); + // A closed dialog must not close or reset the one opened after it. + if (!request.current()) return; setShowDialog(false); setForm(emptyForm()); }, @@ -486,7 +490,8 @@ export function MaintenanceWindowsPage() { {form.id ? t('maintenance:editTitle') : t('maintenance:createTitle')} -
+ {/* Locked while saving, so the response never discards edits made after the submit. */} +
)} -
+
- diff --git a/src/nodepilot-ui/src/pages/UsersPage.tsx b/src/nodepilot-ui/src/pages/UsersPage.tsx index a0a4d740..77d9f0a6 100644 --- a/src/nodepilot-ui/src/pages/UsersPage.tsx +++ b/src/nodepilot-ui/src/pages/UsersPage.tsx @@ -17,6 +17,7 @@ import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useMemo, useState } from 'react'; import { useTranslation, Trans } from 'react-i18next'; import { useColumnResize } from '../hooks/useColumnResize'; +import { useAsyncIdentity } from '../hooks/useAsyncIdentity'; import { api } from '../api/client'; import { ModalShell } from '../components/common/ModalShell'; import { MobileCardList } from '../components/common/MobileCardList'; @@ -142,6 +143,7 @@ export function UsersPage() { const currentUsername = useAuthStore((s) => s.username); const isMobile = useIsMobile(); const [dialog, setDialog] = useState(null); + const captureSession = useAsyncIdentity(dialog); const [search, setSearch] = useState(''); const [sortBy, setSortBy] = useState('username'); @@ -166,19 +168,20 @@ export function UsersPage() { }); const createMutation = useMutation({ - mutationFn: (body: CreateUserPayload) => api.post('/users', body), - onSuccess: () => { + mutationFn: ({ body }: { body: CreateUserPayload; current: () => boolean }) => api.post('/users', body), + onSuccess: (_result, request) => { queryClient.invalidateQueries({ queryKey: ['users'] }); - setDialog(null); + // A closed dialog must not close the one opened after it. + if (request.current()) setDialog(null); }, onError: (err: Error) => toast.error(t('common:createFailed', { message: err.message })), }); const updateMutation = useMutation({ - mutationFn: ({ id, body }: { id: string; body: UpdateUserPayload }) => api.put(`/users/${id}`, body), - onSuccess: () => { + mutationFn: ({ id, body }: { id: string; body: UpdateUserPayload; current: () => boolean }) => api.put(`/users/${id}`, body), + onSuccess: (_result, request) => { queryClient.invalidateQueries({ queryKey: ['users'] }); - setDialog(null); + if (request.current()) setDialog(null); }, onError: (err: Error) => toast.error(t('common:updateFailed', { message: err.message })), }); @@ -505,7 +508,7 @@ export function UsersPage() { {dialog?.kind === 'create' && ( setDialog(null)} - onSubmit={(body) => createMutation.mutate(body)} + onSubmit={(body) => createMutation.mutate({ body, current: captureSession() })} pending={createMutation.isPending} /> )} @@ -514,7 +517,7 @@ export function UsersPage() { user={dialog.user} isSelf={dialog.user.username === currentUsername} onCancel={() => setDialog(null)} - onSubmit={(body) => updateMutation.mutate({ id: dialog.user.id, body })} + onSubmit={(body) => updateMutation.mutate({ id: dialog.user.id, body, current: captureSession() })} pending={updateMutation.isPending} /> )} @@ -522,7 +525,7 @@ export function UsersPage() { setDialog(null)} - onSubmit={(password) => updateMutation.mutate({ id: dialog.user.id, body: { password } })} + onSubmit={(password) => updateMutation.mutate({ id: dialog.user.id, body: { password }, current: captureSession() })} pending={updateMutation.isPending} /> )} @@ -556,7 +559,7 @@ function CreateUserDialog({ return ( -
+
{t('users:breakGlassHint')} -
+