diff --git a/frontend/src/lib/components/flows/flowModuleNextId.test.ts b/frontend/src/lib/components/flows/flowModuleNextId.test.ts new file mode 100644 index 0000000000..dcf007e925 --- /dev/null +++ b/frontend/src/lib/components/flows/flowModuleNextId.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from 'vitest' + +import type { OpenFlow } from '$lib/gen' +import type { FlowState } from './flowState' +import { nextId } from './flowModuleNextId' + +function flowWith(ids: string[]): OpenFlow { + return { + summary: '', + value: { + modules: ids.map((id) => ({ id, value: { type: 'identity' } as any })) + } + } as OpenFlow +} + +function stateWith(keys: string[]): FlowState { + return Object.fromEntries(keys.map((k) => [k, {}])) as FlowState +} + +describe('nextId', () => { + it('produces a, b, c, ... for a fresh flow', () => { + expect(nextId(stateWith(['failure']), flowWith([]))).toBe('a') + expect(nextId(stateWith(['a', 'failure']), flowWith(['a']))).toBe('b') + expect(nextId(stateWith(['a', 'b', 'c', 'failure']), flowWith(['a', 'b', 'c']))).toBe('d') + }) + + it('ignores the reserved failure/preprocessor keys always present in flowState', () => { + expect(nextId(stateWith(['failure', 'preprocessor']), flowWith([]))).toBe('a') + }) + + // Regression: copy ids ("z2"), subflow result keys and other non-canonical keys land in + // flowState; charsToNumber on them used to leak into the max and made new steps jump to + // garbage ids like "bzw". + it('is not poisoned by copy ids', () => { + const ids = ['a', 'b', 'c'] + const state = stateWith([...ids, 'c2', 'a2', 'z2', 'c10', 'failure']) + expect(nextId(state, flowWith(ids))).toBe('d') + }) + + it('is not poisoned by subflow result keys', () => { + const ids = ['a', 'b'] + const state = stateWith([...ids, 'subflow:abcd', 'Result', 'failure']) + expect(nextId(state, flowWith(ids))).toBe('c') + }) + + // A step renamed to a long lowercase word ("process") is a valid base-26 string and would + // otherwise inflate the max; the length cutoff keeps such renames out of the sequence. + it('is not poisoned by renames to long lowercase words or underscored ids', () => { + const ids = ['a', 'b'] + const state = stateWith([...ids, 'process', 'my_step', 'failure']) + expect(nextId(state, flowWith(ids))).toBe('c') + }) +}) diff --git a/frontend/src/lib/components/flows/flowModuleNextId.ts b/frontend/src/lib/components/flows/flowModuleNextId.ts index e10c900982..48b2eb5ac2 100644 --- a/frontend/src/lib/components/flows/flowModuleNextId.ts +++ b/frontend/src/lib/components/flows/flowModuleNextId.ts @@ -1,19 +1,35 @@ import type { OpenFlow } from '$lib/gen' import { dfs } from './dfs' import type { FlowState } from './flowState' -import { charsToNumber, numberToChars } from './idUtils' +import { charsToNumber, forbiddenIds, numberToChars } from './idUtils' + +const reservedIds = new Set(forbiddenIds) + +// Returns the base-26 value of a key only if it is a short, auto-generated step id +// (a, b, ..., z, aa, ...). flowState/module-id keys also include copy ids ("a2"), subflow +// result keys ("subflow:..."), reserved keys and user-renamed ids; feeding those through +// charsToNumber yields meaningless (often huge) numbers that would poison id generation and +// make new steps jump to ids like "bzw". Short non-canonical keys are rejected via a +// round-trip check; longer keys are skipped entirely, which also leaves user renames to long +// lowercase words (e.g. "process") out of the sequence. +function autoIdNumber(key: string): number | undefined { + if (key.length >= 4 || reservedIds.has(key)) { + return undefined + } + const num = charsToNumber(key) + if (num < 0 || numberToChars(num) !== key) { + return undefined + } + return num +} // Computes the next available id export function nextId(flowState: FlowState, fullFlow: OpenFlow): string { const allIds = dfs(fullFlow.value.modules, (fm) => fm.id) const max = allIds.concat(Object.keys(flowState)).reduce((acc, key) => { - if (key.length >= 4) { - return acc - } else { - const num = charsToNumber(key) - return Math.max(acc, num + 1) - } + const num = autoIdNumber(key) + return num === undefined ? acc : Math.max(acc, num + 1) }, 0) return numberToChars(max) }