From 381ee0cd941bac340234d804473b0b9114948571 Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Tue, 27 Oct 2026 02:33:31 +0000 Subject: [PATCH 1/5] Add the editing core of the Designer: draft, stage text, findings and editor state The IO-free part of editing a workflow in the Designer, with no UI yet: - the draft of the stages, where each stage keeps an id while it is renamed - the text a save writes, which replaces only the block of stages - the findings of a check, put on the stage and field they are about - the state of the editor and its reducer, with the selectors on it The wire types gain the hash and read-only flag of a workflow and the path and severity of a finding, and the page reads them. --- workflow-canvas/src/api/http-api.test.ts | 63 ++- workflow-canvas/src/api/parse.ts | 24 +- workflow-canvas/src/api/types.ts | 21 + .../src/designer/Designer.test.tsx | 25 +- .../src/designer/core/draft.test.ts | 138 ++++++ workflow-canvas/src/designer/core/draft.ts | 121 +++++ .../src/designer/core/edit-reducer.test.ts | 450 ++++++++++++++++++ .../src/designer/core/edit-reducer.ts | 169 +++++++ .../src/designer/core/edit-state.ts | 89 ++++ .../src/designer/core/edit-view.test.ts | 168 +++++++ .../src/designer/core/edit-view.ts | 65 +++ .../src/designer/core/problems.test.ts | 120 +++++ workflow-canvas/src/designer/core/problems.ts | 101 ++++ .../src/designer/core/stage-text.test.ts | 172 +++++++ .../src/designer/core/stage-text.ts | 96 ++++ workflow-canvas/src/test-support/fixtures.ts | 11 +- workflow-canvas/stub-api/http/ui-wire.test.ts | 4 + .../test-support/designer-purity.test.ts | 49 ++ 18 files changed, 1875 insertions(+), 11 deletions(-) create mode 100644 workflow-canvas/src/designer/core/draft.test.ts create mode 100644 workflow-canvas/src/designer/core/draft.ts create mode 100644 workflow-canvas/src/designer/core/edit-reducer.test.ts create mode 100644 workflow-canvas/src/designer/core/edit-reducer.ts create mode 100644 workflow-canvas/src/designer/core/edit-state.ts create mode 100644 workflow-canvas/src/designer/core/edit-view.test.ts create mode 100644 workflow-canvas/src/designer/core/edit-view.ts create mode 100644 workflow-canvas/src/designer/core/problems.test.ts create mode 100644 workflow-canvas/src/designer/core/problems.ts create mode 100644 workflow-canvas/src/designer/core/stage-text.test.ts create mode 100644 workflow-canvas/src/designer/core/stage-text.ts create mode 100644 workflow-canvas/test-support/designer-purity.test.ts diff --git a/workflow-canvas/src/api/http-api.test.ts b/workflow-canvas/src/api/http-api.test.ts index 4a0c0f0e..19a1325f 100644 --- a/workflow-canvas/src/api/http-api.test.ts +++ b/workflow-canvas/src/api/http-api.test.ts @@ -105,35 +105,86 @@ describe('createHttpApi', () => { expect(await api.getWorkflow('t')).toEqual({ name: 'hello', text: 'name: hello\n', + hash: 'a'.repeat(64), + readOnly: false, view: DOCUMENT_BODY.view, diagnostics: [], }); }); + it('reads a read-only workflow', async () => { + const { api } = apiFor(reply({ ...DOCUMENT_BODY, read_only: true })); + + expect(await api.getWorkflow('t')).toMatchObject({ readOnly: true }); + }); + + it('refuses a workflow that does not say whether it is read-only', async () => { + const without: Record = { ...DOCUMENT_BODY }; + delete without.read_only; + const { api } = apiFor(reply(without)); + + await expect(api.getWorkflow('t')).rejects.toBeInstanceOf(ApiError); + }); + it('reads a workflow with no file as no text and no view', async () => { const none = { ...DOCUMENT_BODY, text: null, hash: null, view: null }; const { api } = apiFor(reply(none)); expect(await api.getWorkflow('t')).toMatchObject({ text: null, + hash: null, view: null, }); }); - it('reads the message and line of each diagnostic', async () => { + it('reads the message, line, path and severity of each diagnostic', async () => { const body = { ...DOCUMENT_BODY, view: null, diagnostics: [ - { code: 'MISSING_KEY', message: 'Missing key', line: 3 }, - { code: 'WRONG_TYPE', message: 'Wrong type' }, + { + code: 'MISSING_KEY', + severity: 'error', + path: 'stages.write', + message: 'Missing key', + line: 3, + }, + { + code: 'WRONG_TYPE', + severity: 'warning', + path: 'intent', + message: 'Wrong type', + }, ], }; const { api } = apiFor(reply(body)); expect((await api.getWorkflow('t')).diagnostics).toEqual([ - { message: 'Missing key', line: 3 }, - { message: 'Wrong type', line: undefined }, + { + message: 'Missing key', + line: 3, + path: 'stages.write', + severity: 'error', + }, + { + message: 'Wrong type', + line: undefined, + path: 'intent', + severity: 'warning', + }, + ]); + }); + + it('takes a severity it does not know as an error', async () => { + const body = { + ...DOCUMENT_BODY, + view: null, + diagnostics: [{ message: 'Odd', severity: 'fatal', path: '' }], + }; + const { api } = apiFor(reply(body)); + + expect((await api.getWorkflow('t')).diagnostics).toMatchObject([ + { severity: 'error' }, ]); }); @@ -147,7 +198,7 @@ describe('createHttpApi', () => { }; const { api } = apiFor(reply(body)); - expect((await api.getWorkflow('t')).diagnostics).toEqual([ + expect((await api.getWorkflow('t')).diagnostics).toMatchObject([ { message: 'Odd line', line: undefined }, ]); }, diff --git a/workflow-canvas/src/api/parse.ts b/workflow-canvas/src/api/parse.ts index 27e54e6d..81b273f3 100644 --- a/workflow-canvas/src/api/parse.ts +++ b/workflow-canvas/src/api/parse.ts @@ -6,6 +6,7 @@ import type { RunStatus, RunView, Session, + Severity, StageRunStatus, StageView, WorkflowDocument, @@ -32,6 +33,10 @@ function stringOf(value: unknown): string { return typeof value === 'string' ? value : fail(); } +function booleanOf(value: unknown): boolean { + return typeof value === 'boolean' ? value : fail(); +} + function arrayOf(value: unknown): unknown[] { return Array.isArray(value) ? value : fail(); } @@ -73,14 +78,28 @@ function lineOf(value: unknown): number | undefined { : undefined; } +const SEVERITIES: readonly Severity[] = ['error', 'warning', 'info']; + +// A severity that a later version adds is taken as an error, so a finding of +// a kind this page cannot judge never reads as harmless. +function severityOf(value: unknown): Severity { + return SEVERITIES.find((candidate) => candidate === value) ?? 'error'; +} + function parseDiagnostic(value: unknown): DiagnosticView { const diagnostic = objectOf(value); return { message: stringOf(diagnostic.message), line: lineOf(diagnostic.line), + path: typeof diagnostic.path === 'string' ? diagnostic.path : '', + severity: severityOf(diagnostic.severity), }; } +function parseDiagnostics(value: unknown): DiagnosticView[] { + return arrayOf(value).map(parseDiagnostic); +} + const RUN_STATUSES: readonly RunStatus[] = ['running', 'passed', 'halted']; const STAGE_STATUSES: readonly StageRunStatus[] = [ 'pending', @@ -175,11 +194,14 @@ export function parseSession(body: unknown): Session { export function parseWorkflowDocument(body: unknown): WorkflowDocument { const document = objectOf(body); const text = document.text; + const hash = document.hash; return { name: stringOf(document.name), text: text === null ? null : stringOf(text), + hash: hash === null ? null : stringOf(hash), + readOnly: booleanOf(document.read_only), view: parseView(document.view), - diagnostics: arrayOf(document.diagnostics).map(parseDiagnostic), + diagnostics: parseDiagnostics(document.diagnostics), }; } diff --git a/workflow-canvas/src/api/types.ts b/workflow-canvas/src/api/types.ts index a088d456..aebbc9a2 100644 --- a/workflow-canvas/src/api/types.ts +++ b/workflow-canvas/src/api/types.ts @@ -23,11 +23,21 @@ export interface WorkflowView { stages: StageView[]; } +/** How serious a finding of the check is. */ +export type Severity = 'error' | 'warning' | 'info'; + /** A problem the server found in the workflow text. */ export interface DiagnosticView { message: string; /** The line in the text, counted from 1; `undefined` when the reply has none. */ line: number | undefined; + /** + * The key path of the finding, such as `stages.write.agent` or + * `stages[1].name`; the empty string for the whole text. + */ + path: string; + /** A severity this page does not know counts as `error`. */ + severity: Severity; } /** What `GET /api/workflow` returns. */ @@ -35,11 +45,22 @@ export interface WorkflowDocument { name: string; /** `null` when the file does not exist yet. */ text: string | null; + /** The hash to send back as `base_hash`; `null` when there is no file. */ + hash: string | null; + /** True when the file cannot be saved. */ + readOnly: boolean; /** `null` when there is no text or the text has errors. */ view: WorkflowView | null; diagnostics: DiagnosticView[]; } +/** What `PUT /api/workflow` returns: the file as it was written. */ +export interface SaveResult { + hash: string; + view: WorkflowView; + diagnostics: DiagnosticView[]; +} + /** How far a stage of a run has got. `blocked` failed and stopped the run. */ export type StageRunStatus = 'pending' | 'running' | 'passed' | 'blocked'; diff --git a/workflow-canvas/src/designer/Designer.test.tsx b/workflow-canvas/src/designer/Designer.test.tsx index 725e01b0..1e20ce14 100644 --- a/workflow-canvas/src/designer/Designer.test.tsx +++ b/workflow-canvas/src/designer/Designer.test.tsx @@ -63,7 +63,14 @@ describe('Designer', () => { const broken = { ...HELLO_DOCUMENT, view: null, - diagnostics: [{ message: 'Missing key: intent', line: 2 }], + diagnostics: [ + { + message: 'Missing key: intent', + line: 2, + path: '', + severity: 'error' as const, + }, + ], }; render(); @@ -108,7 +115,14 @@ describe('Designer', () => { const broken = { ...HELLO_DOCUMENT, view: null, - diagnostics: [{ message: 'The file is empty', line: undefined }], + diagnostics: [ + { + message: 'The file is empty', + line: undefined, + path: '', + severity: 'error' as const, + }, + ], }; render(); @@ -121,7 +135,12 @@ describe('Designer', () => { }); it('lists every problem when the server repeats an identical one', () => { - const repeated = { message: 'Unknown key: owner', line: 3 }; + const repeated = { + message: 'Unknown key: owner', + line: 3, + path: 'owner', + severity: 'error' as const, + }; const broken = { ...HELLO_DOCUMENT, view: null, diff --git a/workflow-canvas/src/designer/core/draft.test.ts b/workflow-canvas/src/designer/core/draft.test.ts new file mode 100644 index 00000000..cdf30618 --- /dev/null +++ b/workflow-canvas/src/designer/core/draft.test.ts @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, expect, it } from 'vitest'; +import { + addStage, + canAddStage, + canRemoveStage, + DEFAULT_AGENT, + draftOf, + MAX_STAGES, + neighbourOf, + removeStage, + renameStage, + sameStages, + setAgent, + stagesOf, +} from './draft'; + +const TWO = [ + { name: 'draft', agent: 'k-developer' }, + { name: 'review', agent: 'k-quality-assurance' }, +]; + +describe('draftOf and stagesOf', () => { + it('gives each stage an id of its own and gives the stages back in order', () => { + const draft = draftOf(TWO); + + expect(draft.stages.map((stage) => stage.id)).toEqual(['s1', 's2']); + expect(stagesOf(draft)).toEqual(TWO); + }); +}); + +describe('addStage', () => { + it('adds a stage at the end with the first free name and the default agent', () => { + const draft = addStage(draftOf(TWO)); + + expect(stagesOf(draft).at(-1)).toEqual({ + name: 'agent-1', + agent: DEFAULT_AGENT, + }); + expect(draft.stages).toHaveLength(3); + }); + + it('skips a name that a stage already has', () => { + const taken = draftOf([ + { name: 'agent-1', agent: 'k-developer' }, + { name: 'agent-3', agent: 'k-developer' }, + ]); + + expect(stagesOf(addStage(taken)).at(-1)?.name).toBe('agent-2'); + }); + + it('gives a new stage an id that no earlier stage had, even after a delete', () => { + let draft = draftOf(TWO); + draft = removeStage(draft, 's2'); + draft = addStage(draft); + + expect(draft.stages.map((stage) => stage.id)).toEqual(['s1', 's3']); + }); + + it('adds nothing past the most stages the API allows', () => { + const full = draftOf( + Array.from({ length: MAX_STAGES }, (_, index) => ({ + name: `stage-${index}`, + agent: 'k-developer', + })), + ); + + expect(canAddStage(full)).toBe(false); + expect(addStage(full)).toBe(full); + expect(canAddStage(draftOf(TWO))).toBe(true); + }); +}); + +describe('renameStage and setAgent', () => { + it('change only the stage with the id', () => { + const draft = draftOf(TWO); + + expect(stagesOf(renameStage(draft, 's2', 'qa'))).toEqual([ + TWO[0], + { name: 'qa', agent: 'k-quality-assurance' }, + ]); + expect(stagesOf(setAgent(draft, 's1', 'k-architect'))).toEqual([ + { name: 'draft', agent: 'k-architect' }, + TWO[1], + ]); + }); + + it('keep the id of a stage that was renamed', () => { + const draft = renameStage(draftOf(TWO), 's1', 'first'); + + expect(draft.stages[0]?.id).toBe('s1'); + }); + + it('take the name as typed, even one the API would refuse', () => { + const draft = renameStage(draftOf(TWO), 's1', 'Not A Name'); + + expect(draft.stages[0]?.name).toBe('Not A Name'); + }); +}); + +describe('removeStage', () => { + it('removes the stage with the id', () => { + expect(stagesOf(removeStage(draftOf(TWO), 's1'))).toEqual([TWO[1]]); + }); + + it('keeps the last stage', () => { + const one = draftOf([TWO[0]!]); + + expect(canRemoveStage(one)).toBe(false); + expect(removeStage(one, 's1')).toBe(one); + expect(canRemoveStage(draftOf(TWO))).toBe(true); + }); +}); + +describe('neighbourOf', () => { + const three = draftOf([...TWO, { name: 'ship', agent: 'k-developer' }]); + + it('is the stage before, or the next one for the first stage', () => { + expect(neighbourOf(three, 's3')).toBe('s2'); + expect(neighbourOf(three, 's2')).toBe('s1'); + expect(neighbourOf(three, 's1')).toBe('s2'); + }); + + it('is nothing for an id the draft does not have', () => { + expect(neighbourOf(three, 'nope')).toBeUndefined(); + }); +}); + +describe('sameStages', () => { + it('compares names and agents in order', () => { + expect(sameStages(TWO, [...TWO])).toBe(true); + expect(sameStages(TWO, [...TWO].reverse())).toBe(false); + expect(sameStages(TWO, TWO.slice(0, 1))).toBe(false); + expect( + sameStages(TWO, [TWO[0]!, { ...TWO[1]!, agent: 'k-architect' }]), + ).toBe(false); + }); +}); diff --git a/workflow-canvas/src/designer/core/draft.ts b/workflow-canvas/src/designer/core/draft.ts new file mode 100644 index 00000000..3a4cc7e6 --- /dev/null +++ b/workflow-canvas/src/designer/core/draft.ts @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { StageView } from '../../api/types'; + +/** A stage being edited. The `id` stays when the stage is renamed. */ +export interface DraftStage { + id: string; + name: string; + agent: string; +} + +/** The stages of the workflow as edited, in run order. */ +export interface Draft { + stages: readonly DraftStage[]; + /** The number that the next new stage takes for its `id`. */ + nextId: number; +} + +/** The most stages a workflow may have, as the API allows. */ +export const MAX_STAGES = 40; + +/** The agent of a stage that was just added. */ +export const DEFAULT_AGENT = 'k-developer'; + +function idOf(number: number): string { + return `s${number}`; +} + +/** A draft that starts as the stages of a saved workflow. */ +export function draftOf(stages: readonly StageView[]): Draft { + return { + stages: stages.map((stage, index) => ({ ...stage, id: idOf(index + 1) })), + nextId: stages.length + 1, + }; +} + +/** The stages of a draft as the API names them. */ +export function stagesOf(draft: Draft): StageView[] { + return draft.stages.map(({ name, agent }) => ({ name, agent })); +} + +/** Whether two lists have the same stages, in the same order. */ +export function sameStages( + a: readonly StageView[], + b: readonly StageView[], +): boolean { + return ( + a.length === b.length && + a.every( + (stage, index) => + stage.name === b[index]?.name && stage.agent === b[index]?.agent, + ) + ); +} + +/** Whether another stage may be added. */ +export function canAddStage(draft: Draft): boolean { + return draft.stages.length < MAX_STAGES; +} + +/** Whether a stage may be deleted: a workflow keeps at least one. */ +export function canRemoveStage(draft: Draft): boolean { + return draft.stages.length > 1; +} + +// The first `agent-N` that no stage has, counting from 1. +function freeName(draft: Draft): string { + const taken = new Set(draft.stages.map((stage) => stage.name)); + let number = 1; + while (taken.has(`agent-${number}`)) number += 1; + return `agent-${number}`; +} + +/** Adds a stage with a free name and the default agent at the end. */ +export function addStage(draft: Draft): Draft { + if (!canAddStage(draft)) return draft; + const stage = { + id: idOf(draft.nextId), + name: freeName(draft), + agent: DEFAULT_AGENT, + }; + return { stages: [...draft.stages, stage], nextId: draft.nextId + 1 }; +} + +function change( + draft: Draft, + id: string, + edit: Partial>, +): Draft { + return { + ...draft, + stages: draft.stages.map((stage) => + stage.id === id ? { ...stage, ...edit } : stage, + ), + }; +} + +/** Gives the stage a new name, as typed: the check judges it, not this. */ +export function renameStage(draft: Draft, id: string, name: string): Draft { + return change(draft, id, { name }); +} + +/** Gives the stage another agent. */ +export function setAgent(draft: Draft, id: string, agent: string): Draft { + return change(draft, id, { agent }); +} + +/** Deletes the stage; the last one stays. */ +export function removeStage(draft: Draft, id: string): Draft { + if (!canRemoveStage(draft)) return draft; + return { ...draft, stages: draft.stages.filter((stage) => stage.id !== id) }; +} + +/** + * The stage to select once `id` is gone: the one before it, or the first + * when it was the first. `undefined` when the draft has no such stage. + */ +export function neighbourOf(draft: Draft, id: string): string | undefined { + const index = draft.stages.findIndex((stage) => stage.id === id); + if (index < 0) return undefined; + return (draft.stages[index - 1] ?? draft.stages[index + 1])?.id; +} diff --git a/workflow-canvas/src/designer/core/edit-reducer.test.ts b/workflow-canvas/src/designer/core/edit-reducer.test.ts new file mode 100644 index 00000000..47b2972e --- /dev/null +++ b/workflow-canvas/src/designer/core/edit-reducer.test.ts @@ -0,0 +1,450 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, expect, it } from 'vitest'; +import type { DiagnosticView, SaveResult } from '../../api/types'; +import { HELLO_DOCUMENT, HELLO_VIEW } from '../../test-support/fixtures'; +import { stagesOf } from './draft'; +import { reduce } from './edit-reducer'; +import { openDocument, type EditEvent, type EditState } from './edit-state'; + +function opened(): EditState { + const state = openDocument(HELLO_DOCUMENT); + if (!state) throw new Error('The sample cannot be edited.'); + return state; +} + +function run(state: EditState, ...events: EditEvent[]): EditState { + return events.reduce((current, event) => { + const next = reduce(current, event); + if (!next) throw new Error(`${event.type} left nothing to edit.`); + return next; + }, state); +} + +function finding(path: string, message = 'Wrong.'): DiagnosticView { + return { message, path, line: 2, severity: 'error' }; +} + +const SAVED_HASH = 'b'.repeat(64); + +function savedResult(stages = HELLO_VIEW.stages): SaveResult { + return { + hash: SAVED_HASH, + view: { ...HELLO_VIEW, stages }, + diagnostics: [], + }; +} + +describe('a read-only workflow', () => { + const readOnly = (): EditState => { + const state = openDocument({ ...HELLO_DOCUMENT, readOnly: true }); + if (!state) throw new Error('The sample cannot be edited.'); + return state; + }; + + it.each([ + ['add', { type: 'add' }], + ['rename', { type: 'rename', id: 's1', name: 'other' }], + ['set-agent', { type: 'set-agent', id: 's1', agent: 'k-tpm' }], + ['remove', { type: 'remove', id: 's1' }], + ] as const)('takes no %s', (_, event) => { + const state = readOnly(); + + expect(run(state, event)).toBe(state); + }); + + it('still lets a stage be selected', () => { + expect(run(readOnly(), { type: 'select', id: 's2' }).selectedId).toBe('s2'); + }); +}); + +describe('open', () => { + it('starts a draft of the saved stages with the first one selected', () => { + const state = opened(); + + expect(stagesOf(state.draft)).toEqual(HELLO_VIEW.stages); + expect(state.selectedId).toBe('s1'); + expect(state.revision).toBe(0); + expect(state.save).toEqual({ phase: 'idle' }); + }); + + it.each([ + ['no file', { text: null, hash: null, view: null }], + ['a file with errors', { view: null }], + ])('leaves nothing to edit for %s', (_, change) => { + expect(openDocument({ ...HELLO_DOCUMENT, ...change })).toBeUndefined(); + }); + + it('ignores every other event until a workflow is open', () => { + expect(reduce(undefined, { type: 'add' })).toBeUndefined(); + expect( + reduce(undefined, { type: 'open', document: HELLO_DOCUMENT }), + ).toBeDefined(); + }); +}); + +describe('editing the draft', () => { + it('adds a stage, selects it and counts a revision', () => { + const state = run(opened(), { type: 'add' }); + + expect(state.draft.stages.at(-1)).toMatchObject({ name: 'agent-1' }); + expect(state.selectedId).toBe(state.draft.stages.at(-1)?.id); + expect(state.revision).toBe(1); + }); + + it('renames a stage and changes the agent of a stage', () => { + const state = run( + opened(), + { type: 'rename', id: 's1', name: 'write' }, + { type: 'set-agent', id: 's2', agent: 'k-architect' }, + ); + + expect(stagesOf(state.draft)).toEqual([ + { name: 'write', agent: 'k-developer' }, + { name: 'review', agent: 'k-architect' }, + ]); + expect(state.revision).toBe(2); + }); + + it('does not count a revision when a value does not change', () => { + const start = opened(); + + expect(reduce(start, { type: 'rename', id: 's1', name: 'draft' })).toBe( + start, + ); + expect( + reduce(start, { type: 'set-agent', id: 's1', agent: 'k-developer' }), + ).toBe(start); + expect(reduce(start, { type: 'rename', id: 'nope', name: 'x' })).toBe( + start, + ); + }); + + it('deletes the selected stage and selects the one before it', () => { + const state = run( + opened(), + { type: 'select', id: 's2' }, + { type: 'remove', id: 's2' }, + ); + + expect(stagesOf(state.draft)).toEqual([HELLO_VIEW.stages[0]]); + expect(state.selectedId).toBe('s1'); + }); + + it('deletes the first stage and selects the next one', () => { + const state = run(opened(), { type: 'remove', id: 's1' }); + + expect(state.selectedId).toBe('s2'); + }); + + it('keeps the selection when another stage is deleted', () => { + const state = run(opened(), { type: 'remove', id: 's2' }); + + expect(state.selectedId).toBe('s1'); + }); + + it('keeps the last stage', () => { + const one = run(opened(), { type: 'remove', id: 's2' }); + + expect(reduce(one, { type: 'remove', id: 's1' })).toBe(one); + }); + + it('selects only a stage that is in the draft', () => { + const start = opened(); + + expect(run(start, { type: 'select', id: 's2' }).selectedId).toBe('s2'); + expect(reduce(start, { type: 'select', id: 'nope' })).toBe(start); + }); +}); + +describe('check', () => { + it('keeps the findings of the check of the current draft', () => { + const state = run( + opened(), + { type: 'add' }, + { + type: 'checked', + revision: 1, + diagnostics: [finding('stages[2].name')], + }, + ); + + expect(state.check).toEqual({ + revision: 1, + diagnostics: [finding('stages[2].name')], + problems: [ + { message: 'Wrong.', severity: 'error', stageId: 's3', field: 'name' }, + ], + }); + }); + + it('keeps the findings of an older draft on show until the new check answers', () => { + const state = run( + opened(), + { type: 'add' }, + { + type: 'checked', + revision: 1, + diagnostics: [finding('stages[2].name')], + }, + { type: 'rename', id: 's3', name: 'x' }, + ); + + expect(state.revision).toBe(2); + expect(state.check.revision).toBe(1); + expect(state.check.diagnostics).toEqual([finding('stages[2].name')]); + }); + + it('drops the findings of a draft that has changed since', () => { + const state = run( + opened(), + { type: 'add' }, + { type: 'add' }, + { + type: 'checked', + revision: 1, + diagnostics: [finding('stages[2].name')], + }, + ); + + expect(state.check.revision).toBe(0); + }); +}); + +describe('save', () => { + const dirty = () => + run(opened(), { type: 'set-agent', id: 's1', agent: 'k-tpm' }); + + it('starts only when there is something to save', () => { + expect(run(dirty(), { type: 'save' }).save).toEqual({ phase: 'saving' }); + const clean = opened(); + expect(reduce(clean, { type: 'save' })).toBe(clean); + }); + + it('does not start while the check of the draft has found errors', () => { + const bad = run(dirty(), { + type: 'checked', + revision: 1, + diagnostics: [finding('stages[0].agent')], + }); + + expect(reduce(bad, { type: 'save' })).toBe(bad); + }); + + it('does not start a second save while one is going', () => { + const saving = run(dirty(), { type: 'save' }); + + expect(reduce(saving, { type: 'save' })).toBe(saving); + }); + + it('makes the sent text the saved one and keeps the draft', () => { + const saved = run( + dirty(), + { type: 'save' }, + { + type: 'saved', + text: 'new text', + result: savedResult([ + { name: 'draft', agent: 'k-tpm' }, + HELLO_VIEW.stages[1]!, + ]), + }, + ); + + expect(saved.saved).toMatchObject({ text: 'new text', hash: SAVED_HASH }); + expect(saved.saved.view.stages[0]?.agent).toBe('k-tpm'); + expect(saved.save).toEqual({ phase: 'saved' }); + expect(stagesOf(saved.draft)[0]?.agent).toBe('k-tpm'); + }); + + it('replaces the findings of an older draft with those of the save', () => { + const saved = run( + opened(), + { type: 'set-agent', id: 's1', agent: 'nope' }, + { + type: 'checked', + revision: 1, + diagnostics: [finding('stages[0].agent')], + }, + { type: 'set-agent', id: 's1', agent: 'k-tpm' }, + { type: 'save' }, + { + type: 'saved', + text: 'x', + result: savedResult([ + { name: 'draft', agent: 'k-tpm' }, + HELLO_VIEW.stages[1]!, + ]), + }, + ); + + expect(saved.check).toEqual({ revision: 2, diagnostics: [], problems: [] }); + }); + + it('leaves the findings alone when the draft changed during the save', () => { + const saved = run( + opened(), + { type: 'set-agent', id: 's1', agent: 'nope' }, + { + type: 'checked', + revision: 1, + diagnostics: [finding('stages[0].agent')], + }, + { type: 'save' }, + { type: 'rename', id: 's2', name: 'qa' }, + { + type: 'saved', + text: 'x', + result: savedResult([ + { name: 'draft', agent: 'nope' }, + HELLO_VIEW.stages[1]!, + ]), + }, + ); + + expect(saved.check.revision).toBe(1); + }); + + it('keeps an edit made while the save was going', () => { + const saved = run( + dirty(), + { type: 'save' }, + { type: 'rename', id: 's2', name: 'qa' }, + { + type: 'saved', + text: 'sent', + result: savedResult([ + { name: 'draft', agent: 'k-tpm' }, + HELLO_VIEW.stages[1]!, + ]), + }, + ); + + expect(saved.draft.stages[1]?.name).toBe('qa'); + expect(saved.saved.view.stages[1]?.name).toBe('review'); + }); + + it('forgets that it was saved once the draft changes again', () => { + const state = run( + dirty(), + { type: 'save' }, + { type: 'saved', text: 'x', result: savedResult() }, + { type: 'add' }, + ); + + expect(state.save).toEqual({ phase: 'idle' }); + }); + + it('waits for a reload after a conflict, and edits do not clear it', () => { + const state = run( + dirty(), + { type: 'save' }, + { type: 'conflict' }, + { type: 'add' }, + ); + + expect(state.save).toEqual({ phase: 'conflict' }); + expect(reduce(state, { type: 'save' })).toBe(state); + }); + + it('shows the findings of a refused save for the draft that was sent', () => { + const state = run( + dirty(), + { type: 'save' }, + { + type: 'refused', + revision: 1, + diagnostics: [finding('stages[0].agent')], + }, + ); + + expect(state.save).toEqual({ phase: 'idle' }); + expect(state.check).toEqual({ + revision: 1, + diagnostics: [finding('stages[0].agent')], + problems: [ + { message: 'Wrong.', severity: 'error', stageId: 's1', field: 'agent' }, + ], + }); + }); + + it('ignores the findings of a refused save if the draft has changed since', () => { + const state = run( + dirty(), + { type: 'save' }, + { type: 'add' }, + { + type: 'refused', + revision: 1, + diagnostics: [finding('stages[0].agent')], + }, + ); + + expect(state.check.revision).toBe(0); + expect(state.save).toEqual({ phase: 'idle' }); + }); + + it('keeps the sentence of a save that failed, until the draft changes', () => { + const failed = run( + dirty(), + { type: 'save' }, + { type: 'failed', message: 'The server did not answer.' }, + ); + + expect(failed.save).toEqual({ + phase: 'failed', + message: 'The server did not answer.', + }); + expect(run(failed, { type: 'add' }).save).toEqual({ phase: 'idle' }); + }); +}); + +describe('reload', () => { + const NEW_DOCUMENT = { + ...HELLO_DOCUMENT, + hash: 'e'.repeat(64), + view: { ...HELLO_VIEW, stages: [{ name: 'only', agent: 'k-tpm' }] }, + }; + + it('starts a new draft from the document and drops the old one', () => { + const state = run( + opened(), + { type: 'add' }, + { type: 'conflict' }, + { type: 'reload' }, + ); + expect(state.save).toEqual({ phase: 'reloading' }); + + const next = run(state, { type: 'reloaded', document: NEW_DOCUMENT }); + + expect(stagesOf(next.draft)).toEqual([{ name: 'only', agent: 'k-tpm' }]); + expect(next.saved.hash).toBe(NEW_DOCUMENT.hash); + expect(next.revision).toBe(0); + expect(next.save).toEqual({ phase: 'reloaded' }); + }); + + it('stays in the conflict, with a reason, when the file can no longer be edited', () => { + const state = run( + opened(), + { type: 'conflict' }, + { type: 'reload' }, + { type: 'reloaded', document: { ...HELLO_DOCUMENT, view: null } }, + ); + + expect(state.save).toMatchObject({ phase: 'conflict' }); + expect(state.save).toHaveProperty('problem'); + }); + + it('stays in the conflict, with the sentence, when the load failed', () => { + const state = run( + opened(), + { type: 'conflict' }, + { type: 'reload' }, + { type: 'reload-failed', message: 'The server did not answer.' }, + ); + + expect(state.save).toEqual({ + phase: 'conflict', + problem: 'The server did not answer.', + }); + }); +}); diff --git a/workflow-canvas/src/designer/core/edit-reducer.ts b/workflow-canvas/src/designer/core/edit-reducer.ts new file mode 100644 index 00000000..af243997 --- /dev/null +++ b/workflow-canvas/src/designer/core/edit-reducer.ts @@ -0,0 +1,169 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { + DiagnosticView, + SaveResult, + WorkflowDocument, +} from '../../api/types'; +import { + addStage, + canAddStage, + canRemoveStage, + neighbourOf, + removeStage, + renameStage, + setAgent, + type Draft, +} from './draft'; +import { openDocument, type EditEvent, type EditState } from './edit-state'; +import { canEdit, canSave, isDirty } from './edit-view'; +import { checkOf } from './problems'; + +const NOT_EDITABLE = + 'The workflow on the server can no longer be edited here. Fix the file and reload the page.'; + +// A change of the draft: a new revision, and a finished save is old news. +// A read-only workflow takes no change. +function edited(state: EditState, draft: Draft, selectedId: string): EditState { + if (draft === state.draft || !canEdit(state)) return state; + const phase = state.save.phase; + const settled = + phase === 'saved' || phase === 'reloaded' || phase === 'failed'; + return { + ...state, + draft, + selectedId, + revision: state.revision + 1, + save: settled ? { phase: 'idle' } : state.save, + }; +} + +function removed(state: EditState, id: string): EditState { + if (!canRemoveStage(state.draft)) return state; + const draft = removeStage(state.draft, id); + const next = + id === state.selectedId ? neighbourOf(state.draft, id) : state.selectedId; + return edited(state, draft, next ?? state.selectedId); +} + +function added(state: EditState): EditState { + if (!canAddStage(state.draft)) return state; + const draft = addStage(state.draft); + return edited(state, draft, draft.stages.at(-1)?.id ?? state.selectedId); +} + +function renamed(state: EditState, id: string, name: string): EditState { + const stage = state.draft.stages.find((candidate) => candidate.id === id); + if (!stage || stage.name === name) return state; + return edited(state, renameStage(state.draft, id, name), state.selectedId); +} + +function agentSet(state: EditState, id: string, agent: string): EditState { + const stage = state.draft.stages.find((candidate) => candidate.id === id); + if (!stage || stage.agent === agent) return state; + return edited(state, setAgent(state.draft, id, agent), state.selectedId); +} + +function selected(state: EditState, id: string): EditState { + return state.draft.stages.some((stage) => stage.id === id) + ? { ...state, selectedId: id } + : state; +} + +function checked( + state: EditState, + revision: number, + diagnostics: readonly DiagnosticView[], +): EditState { + return revision === state.revision + ? { ...state, check: checkOf(revision, diagnostics, state.draft) } + : state; +} + +function savedAs( + state: EditState, + text: string, + result: SaveResult, +): EditState { + const next: EditState = { + ...state, + saved: { + ...state.saved, + text, + hash: result.hash, + view: result.view, + diagnostics: result.diagnostics, + }, + save: { phase: 'saved' }, + }; + // What was saved is what the findings of the save are about; if the draft + // changed meanwhile, the check of its new revision will speak. + return isDirty(next) + ? next + : { + ...next, + check: checkOf(state.revision, result.diagnostics, state.draft), + }; +} + +// Findings of a refused save are for the draft that was sent; if the draft +// has changed since, the next check will speak for the new one. +function refused( + state: EditState, + revision: number, + diagnostics: readonly DiagnosticView[], +): EditState { + const next: EditState = { ...state, save: { phase: 'idle' } }; + return revision === state.revision + ? { ...next, check: checkOf(revision, diagnostics, state.draft) } + : next; +} + +function reloaded(state: EditState, document: WorkflowDocument): EditState { + const next = openDocument(document); + return next + ? { ...next, save: { phase: 'reloaded' } } + : { ...state, save: { phase: 'conflict', problem: NOT_EDITABLE } }; +} + +/** + * The next state after an event, or `undefined` while there is nothing to + * edit. An event that does not fit, such as a save with nothing to save or a + * check for an older draft, changes nothing. + */ +export function reduce( + state: EditState | undefined, + event: EditEvent, +): EditState | undefined { + if (event.type === 'open') return openDocument(event.document); + if (!state) return undefined; + switch (event.type) { + case 'select': + return selected(state, event.id); + case 'add': + return added(state); + case 'rename': + return renamed(state, event.id, event.name); + case 'set-agent': + return agentSet(state, event.id, event.agent); + case 'remove': + return removed(state, event.id); + case 'checked': + return checked(state, event.revision, event.diagnostics); + case 'save': + return canSave(state) ? { ...state, save: { phase: 'saving' } } : state; + case 'saved': + return savedAs(state, event.text, event.result); + case 'conflict': + return { ...state, save: { phase: 'conflict' } }; + case 'refused': + return refused(state, event.revision, event.diagnostics); + case 'failed': + return { ...state, save: { phase: 'failed', message: event.message } }; + case 'reload': + return { ...state, save: { phase: 'reloading' } }; + case 'reloaded': + return reloaded(state, event.document); + case 'reload-failed': + return { ...state, save: { phase: 'conflict', problem: event.message } }; + } +} diff --git a/workflow-canvas/src/designer/core/edit-state.ts b/workflow-canvas/src/designer/core/edit-state.ts new file mode 100644 index 00000000..2f1795e5 --- /dev/null +++ b/workflow-canvas/src/designer/core/edit-state.ts @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { + DiagnosticView, + SaveResult, + WorkflowDocument, + WorkflowView, +} from '../../api/types'; +import { draftOf, type Draft } from './draft'; +import { checkOf, type Check } from './problems'; + +/** A workflow that has a file, so it can be edited and saved. */ +export type EditableDocument = WorkflowDocument & { + text: string; + hash: string; + view: WorkflowView; +}; + +/** Where saving is. `conflict` waits for a reload. */ +export type SaveState = + | { phase: 'idle' } + | { phase: 'saving' } + | { phase: 'saved' } + | { phase: 'reloading' } + | { phase: 'reloaded' } + | { phase: 'conflict'; problem?: string } + | { phase: 'failed'; message: string }; + +/** What the Designer edits: the saved workflow, and the draft beside it. */ +export interface EditState { + /** The workflow as the server has it. */ + saved: EditableDocument; + draft: Draft; + selectedId: string; + /** Counts the changes of the draft; a check answers for one revision. */ + revision: number; + /** + * The latest check. It stays on show while the draft changes, until the + * check of the new revision answers. + */ + check: Check; + save: SaveState; +} + +/** Something that happened to the editor. */ +export type EditEvent = + | { type: 'open'; document: WorkflowDocument } + | { type: 'select'; id: string } + | { type: 'add' } + | { type: 'rename'; id: string; name: string } + | { type: 'set-agent'; id: string; agent: string } + | { type: 'remove'; id: string } + | { type: 'checked'; revision: number; diagnostics: DiagnosticView[] } + | { type: 'save' } + | { type: 'saved'; text: string; result: SaveResult } + | { type: 'conflict' } + | { + type: 'refused'; + revision: number; + diagnostics: readonly DiagnosticView[]; + } + | { type: 'failed'; message: string } + | { type: 'reload' } + | { type: 'reloaded'; document: WorkflowDocument } + | { type: 'reload-failed'; message: string }; + +/** Whether the document has a file that parses, which is what can be edited. */ +export function isEditable( + document: WorkflowDocument, +): document is EditableDocument { + return ( + document.text !== null && document.hash !== null && document.view !== null + ); +} + +/** The state for a document just loaded, or `undefined` if it has no file. */ +export function openDocument( + document: WorkflowDocument, +): EditState | undefined { + if (!isEditable(document)) return undefined; + const draft = draftOf(document.view.stages); + return { + saved: document, + draft, + selectedId: draft.stages[0]?.id ?? '', + revision: 0, + check: checkOf(0, document.diagnostics, draft), + save: { phase: 'idle' }, + }; +} diff --git a/workflow-canvas/src/designer/core/edit-view.test.ts b/workflow-canvas/src/designer/core/edit-view.test.ts new file mode 100644 index 00000000..213e8175 --- /dev/null +++ b/workflow-canvas/src/designer/core/edit-view.test.ts @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, expect, it } from 'vitest'; +import type { DiagnosticView } from '../../api/types'; +import { HELLO_DOCUMENT, HELLO_TEXT } from '../../test-support/fixtures'; +import { reduce } from './edit-reducer'; +import { openDocument, type EditEvent, type EditState } from './edit-state'; +import { + canSave, + isDirty, + problemsOfDraft, + saveBlocker, + textOf, +} from './edit-view'; + +function run(...events: EditEvent[]): EditState { + let state = openDocument(HELLO_DOCUMENT); + for (const event of events) state = reduce(state, event); + if (!state) throw new Error('Nothing to edit.'); + return state; +} + +const AGENT_CHANGE: EditEvent = { type: 'set-agent', id: 's1', agent: 'k-tpm' }; + +function error(path: string): DiagnosticView { + return { message: 'Wrong.', path, line: 1, severity: 'error' }; +} + +describe('isDirty', () => { + it('is true while the draft differs from the saved stages', () => { + expect(isDirty(run())).toBe(false); + expect(isDirty(run(AGENT_CHANGE))).toBe(true); + }); + + it('is false again when the change is undone', () => { + const undone = run(AGENT_CHANGE, { + type: 'set-agent', + id: 's1', + agent: 'k-developer', + }); + + expect(isDirty(undone)).toBe(false); + }); +}); + +describe('textOf', () => { + it('is the saved text when nothing changed', () => { + expect(textOf(run())).toBe(HELLO_TEXT); + }); + + it('is the saved text with the stages of the draft', () => { + expect(textOf(run(AGENT_CHANGE))).toBe( + HELLO_TEXT.replace('k-developer', 'k-tpm'), + ); + }); +}); + +describe('saveBlocker', () => { + it('says there is nothing to save when nothing changed', () => { + expect(saveBlocker(run())).toBe('There are no changes to save.'); + }); + + it('is nothing for a change that was not found wrong', () => { + expect(saveBlocker(run(AGENT_CHANGE))).toBeUndefined(); + expect(canSave(run(AGENT_CHANGE))).toBe(true); + }); + + it('says to fix the problems when the check of the draft found errors', () => { + const state = run(AGENT_CHANGE, { + type: 'checked', + revision: 1, + diagnostics: [error('stages[0].agent')], + }); + + expect(saveBlocker(state)).toBe('Fix the problems in the workflow first.'); + }); + + it('does not count a finding of the draft before the change', () => { + const state = run( + { type: 'checked', revision: 0, diagnostics: [error('stages[0].agent')] }, + AGENT_CHANGE, + ); + + expect(canSave(state)).toBe(true); + }); + + it('does not count a warning or a note as a reason to stop', () => { + const state = run(AGENT_CHANGE, { + type: 'checked', + revision: 1, + diagnostics: [ + { ...error('stages[0].agent'), severity: 'warning' }, + { ...error('stages[0].name'), severity: 'info' }, + ], + }); + + expect(saveBlocker(state)).toBeUndefined(); + }); + + it('says the workflow is read-only when the server says so', () => { + const open = openDocument({ ...HELLO_DOCUMENT, readOnly: true }); + const state = reduce(open, AGENT_CHANGE); + + expect(state && saveBlocker(state)).toBe('The workflow is read-only.'); + }); + + it.each([ + ['saving', [{ type: 'save' }], 'The workflow is being saved.'], + [ + 'a conflict', + [{ type: 'conflict' }], + 'The file changed on disk. Reload it first.', + ], + ['reloading', [{ type: 'reload' }], 'The workflow is being loaded again.'], + ] as const)('says why not during %s', (_, events, reason) => { + expect(saveBlocker(run(AGENT_CHANGE, ...events))).toBe(reason); + }); +}); + +describe('problemsOfDraft', () => { + it('lists the findings of the check of the draft as it is now', () => { + const state = run(AGENT_CHANGE, { + type: 'checked', + revision: 1, + diagnostics: [error('stages[0].agent')], + }); + + expect(problemsOfDraft(state)).toEqual([ + { message: 'Wrong.', severity: 'error', stageId: 's1', field: 'agent' }, + ]); + }); + + it('keeps the last problems on show while the check of the draft is still to come', () => { + const state = run( + { type: 'checked', revision: 0, diagnostics: [error('stages[0].agent')] }, + AGENT_CHANGE, + ); + + expect(problemsOfDraft(state)).toEqual([ + { message: 'Wrong.', severity: 'error', stageId: 's1', field: 'agent' }, + ]); + }); + + it('keeps a problem on its stage when the stages move', () => { + const state = run( + { type: 'checked', revision: 0, diagnostics: [error('stages[1].name')] }, + { type: 'remove', id: 's1' }, + ); + + expect(problemsOfDraft(state)).toEqual([ + { message: 'Wrong.', severity: 'error', stageId: 's2', field: 'name' }, + ]); + }); + + it('drops a problem of a stage that was deleted, and keeps one of the whole text', () => { + const state = run( + { + type: 'checked', + revision: 0, + diagnostics: [error('stages[1].name'), error('intent')], + }, + { type: 'remove', id: 's2' }, + ); + + expect(problemsOfDraft(state)).toEqual([ + { message: 'Wrong.', severity: 'error' }, + ]); + }); +}); diff --git a/workflow-canvas/src/designer/core/edit-view.ts b/workflow-canvas/src/designer/core/edit-view.ts new file mode 100644 index 00000000..a5426faf --- /dev/null +++ b/workflow-canvas/src/designer/core/edit-view.ts @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { EditState } from './edit-state'; +import { sameStages, stagesOf } from './draft'; +import { hasErrors, type Problem } from './problems'; +import { withStages } from './stage-text'; + +/** The words that say why a read-only workflow cannot be changed. */ +export const READ_ONLY_REASON = 'The workflow is read-only.'; + +/** Whether the stages may be changed: not when the server says read-only. */ +export function canEdit(state: EditState): boolean { + return !state.saved.readOnly; +} + +/** Whether the draft differs from the saved workflow. */ +export function isDirty(state: EditState): boolean { + return !sameStages(stagesOf(state.draft), state.saved.view.stages); +} + +/** The text that a save would write: the saved text with the draft's stages. */ +export function textOf(state: EditState): string { + const { text, view } = state.saved; + return withStages(text, view, stagesOf(state.draft)); +} + +// The findings that speak for the draft as it is now. +function currentFindings(state: EditState) { + return state.check.revision === state.revision ? state.check.diagnostics : []; +} + +/** + * The problems to show: those of the latest check, which stay on show while + * the check of the new draft is still to come, so the page does not flicker + * at every key. A problem of a stage that was deleted is dropped. + */ +export function problemsOfDraft(state: EditState): Problem[] { + const ids = new Set(state.draft.stages.map((stage) => stage.id)); + return state.check.problems.filter( + (problem) => problem.stageId === undefined || ids.has(problem.stageId), + ); +} + +/** Why the draft cannot be saved now, or `undefined` when it can. */ +export function saveBlocker(state: EditState): string | undefined { + if (!canEdit(state)) return READ_ONLY_REASON; + switch (state.save.phase) { + case 'saving': + return 'The workflow is being saved.'; + case 'reloading': + return 'The workflow is being loaded again.'; + case 'conflict': + return 'The file changed on disk. Reload it first.'; + default: + break; + } + if (!isDirty(state)) return 'There are no changes to save.'; + return hasErrors(currentFindings(state)) + ? 'Fix the problems in the workflow first.' + : undefined; +} + +/** Whether the draft can be saved now. */ +export function canSave(state: EditState): boolean { + return saveBlocker(state) === undefined; +} diff --git a/workflow-canvas/src/designer/core/problems.test.ts b/workflow-canvas/src/designer/core/problems.test.ts new file mode 100644 index 00000000..932ca319 --- /dev/null +++ b/workflow-canvas/src/designer/core/problems.test.ts @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, expect, it } from 'vitest'; +import type { DiagnosticView } from '../../api/types'; +import { draftOf } from './draft'; +import { + errorsOf, + hasErrors, + joinMessages, + notesOf, + problemsFor, + problemsOf, +} from './problems'; + +const DRAFT = draftOf([ + { name: 'write', agent: 'k-developer' }, + { name: 'write', agent: 'k-tpm' }, +]); + +function finding(path: string, message = 'Wrong.'): DiagnosticView { + return { message, path, line: 1, severity: 'error' }; +} + +describe('problemsOf', () => { + it('puts a finding about a stage by its place in the list on that stage', () => { + const [problem] = problemsOf([finding('stages[1].name')], DRAFT); + + expect(problem).toEqual({ + message: 'Wrong.', + severity: 'error', + stageId: 's2', + field: 'name', + }); + }); + + it('puts a finding about a stage by its name on the first stage of that name', () => { + const [problem] = problemsOf([finding('stages.write.agent')], DRAFT); + + expect(problem).toMatchObject({ stageId: 's1', field: 'agent' }); + }); + + it.each([ + ['the whole text', ''], + ['the intent', 'intent'], + ['the list of stages', 'stages'], + ['a stage without a field', 'stages[0]'], + ['a stage that is not there', 'stages[7].name'], + ['a name that no stage has', 'stages.gone.name'], + ['a field the Designer does not edit', 'stages.write.mcp'], + ])('leaves a finding about %s without a stage', (_, path) => { + const [problem] = problemsOf([finding(path)], DRAFT); + + expect(problem).toEqual({ message: 'Wrong.', severity: 'error' }); + }); +}); + +describe('problemsFor', () => { + it('lists the messages about one field of one stage', () => { + const problems = problemsOf( + [ + finding('stages[0].name', 'One.'), + finding('stages[0].agent', 'Two.'), + finding('stages[1].name', 'Three.'), + finding('stages[0].name', 'Four.'), + ], + DRAFT, + ); + + expect(problemsFor(problems, 's1', 'name').map((p) => p.message)).toEqual([ + 'One.', + 'Four.', + ]); + expect(problemsFor(problems, 's2', 'agent')).toEqual([]); + }); +}); + +describe('severity', () => { + const warning: DiagnosticView = { + ...finding('stages[0].name', 'Careful.'), + severity: 'warning', + }; + const note: DiagnosticView = { + ...finding('stages[0].agent', 'FYI.'), + severity: 'info', + }; + const problems = problemsOf( + [finding('stages[0].name'), warning, note], + DRAFT, + ); + + it('stays on the problem of a finding', () => { + expect(problems.map((p) => p.severity)).toEqual([ + 'error', + 'warning', + 'info', + ]); + }); + + it('splits the errors, which stop a save, from the notes, which do not', () => { + expect(errorsOf(problems).map((p) => p.message)).toEqual(['Wrong.']); + expect(notesOf(problems).map((p) => p.message)).toEqual([ + 'Careful.', + 'FYI.', + ]); + }); + + it('joins messages into one sentence, or says there are none', () => { + expect(joinMessages(notesOf(problems))).toBe('Careful. FYI.'); + expect(joinMessages([])).toBeUndefined(); + }); +}); + +describe('hasErrors', () => { + it('is true only for a finding of severity error', () => { + const warning: DiagnosticView = { ...finding('x'), severity: 'warning' }; + + expect(hasErrors([warning])).toBe(false); + expect(hasErrors([warning, finding('x')])).toBe(true); + expect(hasErrors([])).toBe(false); + }); +}); diff --git a/workflow-canvas/src/designer/core/problems.ts b/workflow-canvas/src/designer/core/problems.ts new file mode 100644 index 00000000..5856da6e --- /dev/null +++ b/workflow-canvas/src/designer/core/problems.ts @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { DiagnosticView, Severity } from '../../api/types'; +import type { Draft, DraftStage } from './draft'; + +/** A field of a stage that the Designer edits. */ +export type StageField = 'name' | 'agent'; + +/** A finding of the check, with the stage and field it is about, if any. */ +export interface Problem { + message: string; + /** How serious it is; only an `error` stops a save. */ + severity: Severity; + /** The stage it is about; `undefined` for a finding about the whole text. */ + stageId?: string; + field?: StageField; +} + +const BY_INDEX = /^stages\[(\d+)\]\.(name|agent)$/; +const BY_NAME = /^stages\.([^.[\]]+)\.(name|agent)$/; + +function aim( + stage: DraftStage | undefined, + field: string | undefined, +): { stageId: string; field: StageField } | undefined { + return stage && (field === 'name' || field === 'agent') + ? { stageId: stage.id, field } + : undefined; +} + +// The stage and field that a path names, by place in the list or by name. +function targetOf( + path: string, + draft: Draft, +): { stageId: string; field: StageField } | undefined { + const index = BY_INDEX.exec(path); + if (index) return aim(draft.stages[Number(index[1])], index[2]); + const named = BY_NAME.exec(path); + if (!named) return undefined; + return aim( + draft.stages.find((stage) => stage.name === named[1]), + named[2], + ); +} + +/** Turns the findings of a check into problems of the stages of the draft. */ +export function problemsOf( + diagnostics: readonly DiagnosticView[], + draft: Draft, +): Problem[] { + return diagnostics.map(({ message, path, severity }) => ({ + message, + severity, + ...targetOf(path, draft), + })); +} + +/** The latest check: the revision it spoke for, and what it found. */ +export interface Check { + revision: number; + diagnostics: readonly DiagnosticView[]; + /** The findings placed on the stages of the draft that was checked. */ + problems: readonly Problem[]; +} + +/** A check of `draft` at `revision`, with its findings placed on the stages. */ +export function checkOf( + revision: number, + diagnostics: readonly DiagnosticView[], + draft: Draft, +): Check { + return { revision, diagnostics, problems: problemsOf(diagnostics, draft) }; +} + +/** The problems about one field of one stage. */ +export function problemsFor( + problems: readonly Problem[], + stageId: string, + field: StageField, +): Problem[] { + return problems.filter((p) => p.stageId === stageId && p.field === field); +} + +/** The problems that stop a save: the findings of severity `error`. */ +export function errorsOf(problems: readonly Problem[]): Problem[] { + return problems.filter((problem) => problem.severity === 'error'); +} + +/** The problems that do not stop a save: warnings and notes. */ +export function notesOf(problems: readonly Problem[]): Problem[] { + return problems.filter((problem) => problem.severity !== 'error'); +} + +/** The messages of `problems`, joined into one sentence, or `undefined`. */ +export function joinMessages(problems: readonly Problem[]): string | undefined { + return problems.map((problem) => problem.message).join(' ') || undefined; +} + +/** Whether any finding stops the workflow from being saved. */ +export function hasErrors(diagnostics: readonly DiagnosticView[]): boolean { + return diagnostics.some((diagnostic) => diagnostic.severity === 'error'); +} diff --git a/workflow-canvas/src/designer/core/stage-text.test.ts b/workflow-canvas/src/designer/core/stage-text.test.ts new file mode 100644 index 00000000..b763f5f3 --- /dev/null +++ b/workflow-canvas/src/designer/core/stage-text.test.ts @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, expect, it } from 'vitest'; +import { willRewriteWhole, withStages } from './stage-text'; + +const VIEW = { + name: 'hello', + intent: { outcome: 'Add a greeting to the README' }, + stages: [{ name: 'draft', agent: 'k-developer' }], +}; + +const TEXT = + 'name: hello\nintent: Add a greeting to the README\nstages:\n - name: draft\n agent: k-developer\n - name: review\n agent: k-quality-assurance\n'; + +const REVIEW = { name: 'review', agent: 'k-quality-assurance' }; + +describe('withStages', () => { + it('changes an agent and nothing else', () => { + const next = withStages(TEXT, VIEW, [ + { name: 'draft', agent: 'k-architect' }, + REVIEW, + ]); + + expect(next).toBe(TEXT.replace('k-developer', 'k-architect')); + }); + + it('writes the same text back when the stages are the same', () => { + expect( + withStages(TEXT, VIEW, [{ name: 'draft', agent: 'k-developer' }, REVIEW]), + ).toBe(TEXT); + }); + + it('keeps the keys, comments and blank lines around the stages', () => { + const text = + "# Greets\nversion: '1.1'\nname: hello\nintent: Say hi\n\nstages: # in order\n # first\n - name: a\n agent: k-developer\n\n# the end\n"; + + const next = withStages(text, VIEW, [ + { name: 'a', agent: 'k-developer' }, + { name: 'b', agent: 'k-tpm' }, + ]); + + expect(next).toBe( + "# Greets\nversion: '1.1'\nname: hello\nintent: Say hi\n\nstages: # in order\n - name: a\n agent: k-developer\n - name: b\n agent: k-tpm\n\n# the end\n", + ); + }); + + it('keeps a key that comes after the stages', () => { + const text = + 'name: hello\nstages:\n- name: a\n agent: k-developer\nintent: Say hi\n'; + + const next = withStages(text, VIEW, [{ name: 'b', agent: 'k-tpm' }]); + + expect(next).toBe( + 'name: hello\nstages:\n - name: b\n agent: k-tpm\nintent: Say hi\n', + ); + }); + + it('stops the block at a document marker', () => { + const text = + 'name: hello\nstages:\n - name: a\n agent: k-tpm\n---\nx: 1\n'; + + const next = withStages(text, VIEW, [{ name: 'b', agent: 'k-tpm' }]); + + expect(next).toBe( + 'name: hello\nstages:\n - name: b\n agent: k-tpm\n---\nx: 1\n', + ); + }); + + it('adds no line break at the end of a file that has none', () => { + const text = 'name: hello\nstages:\n - name: a\n agent: k-tpm'; + + const next = withStages(text, VIEW, [{ name: 'a', agent: 'k-architect' }]); + + expect(next).toBe( + 'name: hello\nstages:\n - name: a\n agent: k-architect', + ); + }); + + it('keeps a file in CRLF line endings', () => { + const text = TEXT.replaceAll('\n', '\r\n'); + + const next = withStages(text, VIEW, [REVIEW]); + + expect(next).toBe( + 'name: hello\r\nintent: Add a greeting to the README\r\nstages:\r\n - name: review\r\n agent: k-quality-assurance\r\n', + ); + }); + + it('keeps a comment and a key after the stages of a CRLF file', () => { + const text = + 'name: hello\r\nstages:\r\n - name: a\r\n agent: k-tpm\r\n# note\r\nversion: 1\r\n'; + + const next = withStages(text, VIEW, [REVIEW]); + + expect(next).toBe( + 'name: hello\r\nstages:\r\n - name: review\r\n agent: k-quality-assurance\r\n# note\r\nversion: 1\r\n', + ); + }); + + it('adds no stray carriage return to a CRLF file that ends without a line break', () => { + const text = 'name: hello\r\nstages:\r\n - name: a\r\n agent: k-tpm'; + + const next = withStages(text, VIEW, [REVIEW]); + + expect(next).toBe( + 'name: hello\r\nstages:\r\n - name: review\r\n agent: k-quality-assurance', + ); + }); + + it('writes a name that is not plain text in double quotes', () => { + const next = withStages(TEXT, VIEW, [ + { name: 'Bad: name #1', agent: 'true' }, + { name: '123', agent: '' }, + { name: 'trailing ', agent: 'say "hi"' }, + ]); + + expect(next).toContain(' - name: "Bad: name #1"\n agent: "true"\n'); + expect(next).toContain(' - name: "123"\n agent: ""\n'); + expect(next).toContain( + ' - name: "trailing "\n agent: "say \\"hi\\""\n', + ); + }); + + it('writes the whole file from the view when the stages are not a block', () => { + const text = + 'name: hello\nintent: Say hi\nstages: [{name: a, agent: k-tpm}]\n'; + + const next = withStages(text, VIEW, [{ name: 'b', agent: 'k-architect' }]); + + expect(next).toBe( + 'name: hello\nintent: Add a greeting to the README\nstages:\n - name: b\n agent: k-architect\n', + ); + }); + + it('quotes an intent that is not plain text when it writes the whole file', () => { + const view = { ...VIEW, intent: { outcome: 'Fix: the #1 bug' } }; + + const next = withStages('stages: []\n', view, [ + { name: 'a', agent: 'k-tpm' }, + ]); + + expect(next).toContain('intent: "Fix: the #1 bug"\n'); + }); +}); + +describe('willRewriteWhole', () => { + it('is false for stages in the block form', () => { + expect(willRewriteWhole(TEXT)).toBe(false); + }); + + it('is false when a comment follows the key', () => { + expect(willRewriteWhole('name: a\nstages: # run in order\n')).toBe(false); + }); + + it('is true for stages in the flow form', () => { + expect(willRewriteWhole('stages: [{name: a, agent: k-tpm}]\n')).toBe(true); + }); + + it('is true when the text has no stages key', () => { + expect(willRewriteWhole('name: a\n')).toBe(true); + }); + + it('agrees with what withStages does', () => { + const text = + 'name: hello\n# kept\nstages:\n # dropped\n - name: a\n agent: k-tpm\n'; + + const next = withStages(text, VIEW, [{ name: 'a', agent: 'k-tpm' }]); + + expect(willRewriteWhole(text)).toBe(false); + expect(next).toContain('# kept'); + expect(next).not.toContain('# dropped'); + }); +}); diff --git a/workflow-canvas/src/designer/core/stage-text.ts b/workflow-canvas/src/designer/core/stage-text.ts new file mode 100644 index 00000000..17ca5b55 --- /dev/null +++ b/workflow-canvas/src/designer/core/stage-text.ts @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { StageView, WorkflowView } from '../../api/types'; + +// A value that a plain YAML scalar reads back as the same text: it starts +// with a letter, has no character that YAML gives a meaning, and does not end +// in a space. `true`, `false` and `null` are left out, as they are not text. +const PLAIN = /^[A-Za-z](?:[A-Za-z0-9 _.,()/-]*[A-Za-z0-9_.,()/-])?$/; +const NOT_TEXT = /^(?:true|false|null)$/i; + +// The value as YAML text: plain when that is safe, double-quoted if not. +function scalar(value: string): string { + return PLAIN.test(value) && !NOT_TEXT.test(value) + ? value + : JSON.stringify(value); +} + +function stageLines(stages: readonly StageView[]): string[] { + return stages.flatMap((stage) => [ + ` - name: ${scalar(stage.name)}`, + ` agent: ${scalar(stage.agent)}`, + ]); +} + +// The `stages:` key of the top level, with an optional comment after it. +const STAGES_KEY = /^stages:[ \t]*(?:#.*)?\r?$/; +// A line that starts the next top-level key or a document marker, which +// ends the block of stages. A `-` at the start is an item of the block. +const NEXT_KEY = /^(?:[^\s#-]|---|\.\.\.)/; +// `.` stops at a `\r`, so a comment of a CRLF file is matched with `[^\n]`. +const BLANK_OR_COMMENT = /^(?:\s*|#[^\n]*)$/; + +// The file's own line ending, so a file kept in CRLF stays in CRLF. +function endingOf(text: string): string { + return text.includes('\r\n') ? '\r\n' : '\n'; +} + +// Swaps the block of stages of `text` for `stages`, and leaves every other +// byte alone. `undefined` when the text has no `stages:` key of the block form +// at the top level. +function replaceStageBlock( + text: string, + stages: readonly StageView[], +): string | undefined { + const lines = text.split('\n'); + const key = lines.findIndex((line) => STAGES_KEY.test(line)); + if (key < 0) return undefined; + const next = lines.findIndex( + (line, index) => index > key && NEXT_KEY.test(line), + ); + let end = next < 0 ? lines.length : next; + // Blank lines and comments after the last stage belong to what follows. + while (end > key + 1 && BLANK_OR_COMMENT.test(lines[end - 1] ?? '')) end -= 1; + const tail = lines.slice(end); + // A block that ends the file ends it without a line break, as it was. + const cr = endingOf(text) === '\r\n' ? '\r' : ''; + const block = stageLines(stages).map((line, index, all) => + tail.length === 0 && index === all.length - 1 ? line : `${line}${cr}`, + ); + return [...lines.slice(0, key + 1), ...block, ...tail].join('\n'); +} + +// A whole file written from the view: the layout that nothing else fits. +function writeWhole(view: WorkflowView, stages: readonly StageView[]): string { + return [ + `name: ${scalar(view.name)}`, + `intent: ${scalar(view.intent.outcome)}`, + 'stages:', + ...stageLines(stages), + '', + ].join('\n'); +} + +/** + * The text of the workflow with `stages` in place of its own. Only the block + * of stages changes: the name, the intent, `version` and comments outside the + * block stay as they are, and comments inside the block go with its old + * stages. A file whose `stages` are not a block at the top level is written + * whole from `view`, in the plain layout, which drops its comments and + * `version`. + */ +export function withStages( + text: string, + view: WorkflowView, + stages: readonly StageView[], +): string { + return replaceStageBlock(text, stages) ?? writeWhole(view, stages); +} + +/** + * Whether a save of `text` would write the file whole, in the plain layout, + * and so drop its comments and `version`: its `stages` are not a block at the + * top level. + */ +export function willRewriteWhole(text: string): boolean { + return replaceStageBlock(text, []) === undefined; +} diff --git a/workflow-canvas/src/test-support/fixtures.ts b/workflow-canvas/src/test-support/fixtures.ts index 5a3d64cd..05655ca4 100644 --- a/workflow-canvas/src/test-support/fixtures.ts +++ b/workflow-canvas/src/test-support/fixtures.ts @@ -26,10 +26,19 @@ export const HELLO_SESSION: Session = { apiVersion: 1, }; +/** The text of the file behind the workflow above. */ +export const HELLO_TEXT = + 'name: hello\nintent: Add a greeting to the README\nstages:\n - name: draft\n agent: k-developer\n - name: review\n agent: k-quality-assurance\n'; + +/** The hash that the server gives that text; a test only needs it to differ. */ +export const HELLO_HASH = 'a'.repeat(64); + /** The document that `GET /api/workflow` returns for the workflow above. */ export const HELLO_DOCUMENT: WorkflowDocument = { name: 'hello', - text: 'name: hello\n', + text: HELLO_TEXT, + hash: HELLO_HASH, + readOnly: false, view: HELLO_VIEW, diagnostics: [], }; diff --git a/workflow-canvas/stub-api/http/ui-wire.test.ts b/workflow-canvas/stub-api/http/ui-wire.test.ts index 4700039d..f8bc9bdf 100644 --- a/workflow-canvas/stub-api/http/ui-wire.test.ts +++ b/workflow-canvas/stub-api/http/ui-wire.test.ts @@ -2,6 +2,8 @@ /** Reads the real replies of the stub with the parsers of the page. */ import { describe, expect, it } from 'vitest'; import { parseSession, parseWorkflowDocument } from '../../src/api/parse.ts'; +import { sha256Hex } from '../core/hash.ts'; +import { SAMPLE_FILES } from '../core/sample.ts'; import { LAUNCH_CODE, useStub } from '../test-support/harness.ts'; import { oneStageWorkflow } from '../test-support/workflows.ts'; @@ -31,6 +33,8 @@ describe('the wire types of the page read GET /api/workflow', () => { expect(parseWorkflowDocument(reply.body)).toEqual({ name: 'hello', text: expect.stringContaining('name: hello'), + hash: sha256Hex(SAMPLE_FILES.hello ?? ''), + readOnly: false, view: { name: 'hello', intent: { outcome: 'Add a greeting to the README' }, diff --git a/workflow-canvas/test-support/designer-purity.test.ts b/workflow-canvas/test-support/designer-purity.test.ts new file mode 100644 index 00000000..c93ae9eb --- /dev/null +++ b/workflow-canvas/test-support/designer-purity.test.ts @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: Apache-2.0 +/** + * Checks that the core of the Designer's editing does no IO and imports no + * UI: it may import its own modules and the wire types, and nothing else. + */ +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { + problemsIn, + problemsUnder, + sourcesUnder, + type PurityRules, +} from './purity.ts'; + +const RULES: PurityRules = { + dir: fileURLToPath(new URL('../src/designer/core/', import.meta.url)), + allowedPackages: new Set(['../../api/types']), + allowedNames: {}, +}; + +describe('the editing core', () => { + it('has sources to check', () => { + expect(sourcesUnder(RULES.dir).length).toBeGreaterThanOrEqual(5); + }); + + it('imports only its own modules and the wire types, and reads no clock, randomness, environment or network', () => { + expect(problemsUnder(RULES)).toEqual([]); + }); + + it.each([ + ['react', "import { useState } from 'react';"], + ['the HTTP adapter', "import { createHttpApi } from '../../api/http-api';"], + [ + 'the timer adapter', + "import { timerScheduler } from '../../execution/timer-scheduler';", + ], + ['a Node module', "import { readFileSync } from 'node:fs';"], + ])('rejects an import of %s', (_, line) => { + expect(problemsIn('x.ts', line, RULES)).not.toEqual([]); + }); + + it.each([ + ['a timer', 'setTimeout(() => {}, 1000);'], + ['the clock', 'const now = Date.now();'], + ['a network call', "await fetch('/api/validate');"], + ])('rejects %s', (_, line) => { + expect(problemsIn('x.ts', line, RULES)).not.toEqual([]); + }); +}); From ec70358747d9d29ecbea9125d9acecb72c5d8b69 Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Tue, 27 Oct 2026 14:45:00 +0000 Subject: [PATCH 2/5] Mark the earlier stage too when two stages share a name The server reports a repeated stage name on the later stage only. A person who renames the first stage to a name the second one has saw no mark at the field they were editing. The problems of the draft now hold a copy of such a name problem for the earlier stages with that name. A list of problems leaves the copies out, and a message that several stages repeat is joined once. --- .../src/designer/core/edit-view.test.ts | 32 +++++++++ .../src/designer/core/edit-view.ts | 8 ++- .../src/designer/core/problems.test.ts | 67 +++++++++++++++++++ workflow-canvas/src/designer/core/problems.ts | 44 +++++++++++- 4 files changed, 146 insertions(+), 5 deletions(-) diff --git a/workflow-canvas/src/designer/core/edit-view.test.ts b/workflow-canvas/src/designer/core/edit-view.test.ts index 213e8175..955f6b6e 100644 --- a/workflow-canvas/src/designer/core/edit-view.test.ts +++ b/workflow-canvas/src/designer/core/edit-view.test.ts @@ -4,6 +4,7 @@ import type { DiagnosticView } from '../../api/types'; import { HELLO_DOCUMENT, HELLO_TEXT } from '../../test-support/fixtures'; import { reduce } from './edit-reducer'; import { openDocument, type EditEvent, type EditState } from './edit-state'; +import { listedOf, problemsFor } from './problems'; import { canSave, isDirty, @@ -151,6 +152,37 @@ describe('problemsOfDraft', () => { ]); }); + it('marks the field of the earlier stage when a name is repeated, as the server names the later one', () => { + const state = run( + { type: 'rename', id: 's1', name: 'review' }, + { + type: 'checked', + revision: 1, + diagnostics: [error('stages[1].name')], + }, + ); + + const problems = problemsOfDraft(state); + + expect(problemsFor(problems, 's1', 'name')).toHaveLength(1); + expect(problemsFor(problems, 's2', 'name')).toHaveLength(1); + expect(listedOf(problems)).toHaveLength(1); + }); + + it('stops marking the earlier stage as soon as its name is changed again', () => { + const state = run( + { type: 'rename', id: 's1', name: 'review' }, + { + type: 'checked', + revision: 1, + diagnostics: [error('stages[1].name')], + }, + { type: 'rename', id: 's1', name: 'draft' }, + ); + + expect(problemsFor(problemsOfDraft(state), 's1', 'name')).toEqual([]); + }); + it('drops a problem of a stage that was deleted, and keeps one of the whole text', () => { const state = run( { diff --git a/workflow-canvas/src/designer/core/edit-view.ts b/workflow-canvas/src/designer/core/edit-view.ts index a5426faf..5939fb8d 100644 --- a/workflow-canvas/src/designer/core/edit-view.ts +++ b/workflow-canvas/src/designer/core/edit-view.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 import type { EditState } from './edit-state'; import { sameStages, stagesOf } from './draft'; -import { hasErrors, type Problem } from './problems'; +import { hasErrors, withEchoes, type Problem } from './problems'; import { withStages } from './stage-text'; /** The words that say why a read-only workflow cannot be changed. */ @@ -31,13 +31,15 @@ function currentFindings(state: EditState) { /** * The problems to show: those of the latest check, which stay on show while * the check of the new draft is still to come, so the page does not flicker - * at every key. A problem of a stage that was deleted is dropped. + * at every key. A problem of a stage that was deleted is dropped, and a + * repeated name is marked on the earlier stage as well (see `withEchoes`). */ export function problemsOfDraft(state: EditState): Problem[] { const ids = new Set(state.draft.stages.map((stage) => stage.id)); - return state.check.problems.filter( + const present = state.check.problems.filter( (problem) => problem.stageId === undefined || ids.has(problem.stageId), ); + return withEchoes(present, state.draft); } /** Why the draft cannot be saved now, or `undefined` when it can. */ diff --git a/workflow-canvas/src/designer/core/problems.test.ts b/workflow-canvas/src/designer/core/problems.test.ts index 932ca319..342a6c48 100644 --- a/workflow-canvas/src/designer/core/problems.test.ts +++ b/workflow-canvas/src/designer/core/problems.test.ts @@ -6,9 +6,11 @@ import { errorsOf, hasErrors, joinMessages, + listedOf, notesOf, problemsFor, problemsOf, + withEchoes, } from './problems'; const DRAFT = draftOf([ @@ -53,6 +55,71 @@ describe('problemsOf', () => { }); }); +describe('a stage name that two stages share', () => { + const draft = draftOf([ + { name: 'review', agent: 'k-developer' }, + { name: 'review', agent: 'k-tpm' }, + ]); + const duplicate = finding( + 'stages[1].name', + 'The stage name `review` is already used.', + ); + const echoed = (diagnostics: DiagnosticView[], of = draft) => + withEchoes(problemsOf(diagnostics, of), of); + + it('marks the name field of the earlier stage too, not only that of the later one the server names', () => { + const problems = echoed([duplicate]); + + expect(problemsFor(problems, 's2', 'name')).toHaveLength(1); + expect(problemsFor(problems, 's1', 'name').map((p) => p.message)).toEqual([ + duplicate.message, + ]); + }); + + it('marks no other field of the earlier stage', () => { + expect(problemsFor(echoed([duplicate]), 's1', 'agent')).toEqual([]); + }); + + it('is still one problem in a list, so the list says it once', () => { + expect(listedOf(echoed([duplicate])).map((p) => p.stageId)).toEqual(['s2']); + }); + + it('copies nothing to a later stage that has the name', () => { + expect( + withEchoes(problemsOf([finding('stages[0].name')], draft), draft), + ).toHaveLength(1); + }); + + it('does not repeat a message on a stage that has a finding of its own', () => { + const problems = echoed([ + finding('stages[0].name', 'Wrong.'), + finding('stages[1].name', 'Wrong.'), + ]); + + expect(problemsFor(problems, 's1', 'name')).toHaveLength(1); + expect(problemsFor(problems, 's2', 'name')).toHaveLength(1); + }); + + it('joins the same message once when several stages repeat a name', () => { + const three = draftOf([ + { name: 'review', agent: 'k-developer' }, + { name: 'review', agent: 'k-developer' }, + { name: 'review', agent: 'k-developer' }, + ]); + const problems = echoed( + [ + duplicate, + finding('stages[2].name', 'The stage name `review` is already used.'), + ], + three, + ); + + expect(joinMessages(problemsFor(problems, 's1', 'name'))).toBe( + duplicate.message, + ); + }); +}); + describe('problemsFor', () => { it('lists the messages about one field of one stage', () => { const problems = problemsOf( diff --git a/workflow-canvas/src/designer/core/problems.ts b/workflow-canvas/src/designer/core/problems.ts index 5856da6e..c7f9f890 100644 --- a/workflow-canvas/src/designer/core/problems.ts +++ b/workflow-canvas/src/designer/core/problems.ts @@ -13,6 +13,13 @@ export interface Problem { /** The stage it is about; `undefined` for a finding about the whole text. */ stageId?: string; field?: StageField; + /** + * Set on a copy of the problem of a later stage with the same name. The + * server reports a name that two stages share on the later one only, so the + * copy marks the field of the earlier one too. A list of problems leaves + * the copies out; see `listedOf`. + */ + echo?: true; } const BY_INDEX = /^stages\[(\d+)\]\.(name|agent)$/; @@ -71,6 +78,35 @@ export function checkOf( return { revision, diagnostics, problems: problemsOf(diagnostics, draft) }; } +/** + * `problems` and, for each name problem of a stage, a copy for the earlier + * stages of the draft that have the same name and no name problem of their + * own (see `Problem.echo`). + */ +export function withEchoes( + problems: readonly Problem[], + draft: Draft, +): Problem[] { + const named = problems.filter((problem) => problem.field === 'name'); + const marked = new Set(named.map((problem) => problem.stageId)); + const echoes = named.flatMap((problem) => { + const at = draft.stages.findIndex(({ id }) => id === problem.stageId); + const name = draft.stages[at]?.name; + const earlier = draft.stages + .slice(0, Math.max(at, 0)) + .find((stage) => stage.name === name && !marked.has(stage.id)); + return earlier + ? [{ ...problem, stageId: earlier.id, echo: true as const }] + : []; + }); + return [...problems, ...echoes]; +} + +/** The problems without the copies, for a list that says each problem once. */ +export function listedOf(problems: readonly Problem[]): Problem[] { + return problems.filter((problem) => !problem.echo); +} + /** The problems about one field of one stage. */ export function problemsFor( problems: readonly Problem[], @@ -90,9 +126,13 @@ export function notesOf(problems: readonly Problem[]): Problem[] { return problems.filter((problem) => problem.severity !== 'error'); } -/** The messages of `problems`, joined into one sentence, or `undefined`. */ +/** + * The messages of `problems`, each once, joined into one sentence, or + * `undefined`. + */ export function joinMessages(problems: readonly Problem[]): string | undefined { - return problems.map((problem) => problem.message).join(' ') || undefined; + const messages = new Set(problems.map((problem) => problem.message)); + return [...messages].join(' ') || undefined; } /** Whether any finding stops the workflow from being saved. */ From 939609f5a270ff1a0ad7b10c3b8639ccad76c1ea Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Wed, 28 Oct 2026 17:17:57 +0000 Subject: [PATCH 3/5] Keep the comments and layout of unchanged stages, and make the editor ignore late answers - A save keeps the lines of every stage that is unchanged, with the comments in front of it and the comments after the last stage. Line endings come from the stages line, a lone CR, a BOM and a quoted key are handled, and names that YAML 1.1 reads as booleans are quoted. - The revision of the draft only grows, also across open and reload, and an answer to a save or a reload counts only while that save or reload is on. - Deleting a stage that is not in the draft is not a change, and a name used three times marks the first stage once. - A failed save can say that it may have been written. - A test ties the stage limit of the draft to the contract. --- workflow-canvas/src/designer/core/draft.ts | 9 +- .../src/designer/core/edit-reducer.test.ts | 124 ++++++++++++- .../src/designer/core/edit-reducer.ts | 52 +++++- .../src/designer/core/edit-state.ts | 25 ++- .../src/designer/core/edit-view.test.ts | 2 +- .../src/designer/core/problems.test.ts | 24 +++ workflow-canvas/src/designer/core/problems.ts | 10 +- .../src/designer/core/stage-text.test.ts | 175 +++++++++++++++++- .../src/designer/core/stage-text.ts | 150 +++++++++++---- .../test-support/draft-contract.test.ts | 18 ++ 10 files changed, 518 insertions(+), 71 deletions(-) create mode 100644 workflow-canvas/test-support/draft-contract.test.ts diff --git a/workflow-canvas/src/designer/core/draft.ts b/workflow-canvas/src/designer/core/draft.ts index 3a4cc7e6..183143db 100644 --- a/workflow-canvas/src/designer/core/draft.ts +++ b/workflow-canvas/src/designer/core/draft.ts @@ -104,15 +104,16 @@ export function setAgent(draft: Draft, id: string, agent: string): Draft { return change(draft, id, { agent }); } -/** Deletes the stage; the last one stays. */ +/** Deletes the stage; the last one stays, and an unknown id changes nothing. */ export function removeStage(draft: Draft, id: string): Draft { if (!canRemoveStage(draft)) return draft; - return { ...draft, stages: draft.stages.filter((stage) => stage.id !== id) }; + const stages = draft.stages.filter((stage) => stage.id !== id); + return stages.length === draft.stages.length ? draft : { ...draft, stages }; } /** - * The stage to select once `id` is gone: the one before it, or the first - * when it was the first. `undefined` when the draft has no such stage. + * The stage to select once `id` is gone: the one before it, or the one after + * it when it was the first. `undefined` when the draft has no such stage. */ export function neighbourOf(draft: Draft, id: string): string | undefined { const index = draft.stages.findIndex((stage) => stage.id === id); diff --git a/workflow-canvas/src/designer/core/edit-reducer.test.ts b/workflow-canvas/src/designer/core/edit-reducer.test.ts index 47b2972e..0c831796 100644 --- a/workflow-canvas/src/designer/core/edit-reducer.test.ts +++ b/workflow-canvas/src/designer/core/edit-reducer.test.ts @@ -24,6 +24,12 @@ function finding(path: string, message = 'Wrong.'): DiagnosticView { return { message, path, line: 2, severity: 'error' }; } +const NEW_DOCUMENT = { + ...HELLO_DOCUMENT, + hash: 'e'.repeat(64), + view: { ...HELLO_VIEW, stages: [{ name: 'only', agent: 'k-tpm' }] }, +}; + const SAVED_HASH = 'b'.repeat(64); function savedResult(stages = HELLO_VIEW.stages): SaveResult { @@ -399,16 +405,11 @@ describe('save', () => { }); describe('reload', () => { - const NEW_DOCUMENT = { - ...HELLO_DOCUMENT, - hash: 'e'.repeat(64), - view: { ...HELLO_VIEW, stages: [{ name: 'only', agent: 'k-tpm' }] }, - }; - it('starts a new draft from the document and drops the old one', () => { const state = run( opened(), { type: 'add' }, + { type: 'save' }, { type: 'conflict' }, { type: 'reload' }, ); @@ -418,7 +419,7 @@ describe('reload', () => { expect(stagesOf(next.draft)).toEqual([{ name: 'only', agent: 'k-tpm' }]); expect(next.saved.hash).toBe(NEW_DOCUMENT.hash); - expect(next.revision).toBe(0); + expect(next.revision).toBeGreaterThan(state.revision); expect(next.save).toEqual({ phase: 'reloaded' }); }); @@ -448,3 +449,112 @@ describe('reload', () => { }); }); }); + +describe('an answer that is meant for an earlier document or an earlier save', () => { + const OLD_ERROR = finding('stages[0].name'); + + function reloadedAfterEdits(): { before: EditState; after: EditState } { + const before = run( + opened(), + { type: 'rename', id: 's1', name: 'a' }, + { type: 'rename', id: 's1', name: 'b' }, + { type: 'save' }, + { type: 'conflict' }, + { type: 'reload' }, + ); + const after = run( + before, + { type: 'reloaded', document: NEW_DOCUMENT }, + { type: 'rename', id: 's1', name: 'c' }, + { type: 'rename', id: 's1', name: 'd' }, + ); + return { before, after }; + } + + it('does not count a revision twice, also across a reload', () => { + const { before, after } = reloadedAfterEdits(); + + expect(after.revision).toBeGreaterThan(before.revision); + }); + + it('ignores the check of a draft from before a reload', () => { + const { before, after } = reloadedAfterEdits(); + + const next = run(after, { + type: 'checked', + revision: before.revision, + diagnostics: [OLD_ERROR], + }); + + expect(next.check).toBe(after.check); + }); + + it('ignores the check of a draft from before a new document was opened', () => { + const before = run(opened(), { type: 'rename', id: 's1', name: 'a' }); + const after = reduce(before, { type: 'open', document: NEW_DOCUMENT })!; + + expect(after.revision).toBeGreaterThan(before.revision); + expect( + run(after, { + type: 'checked', + revision: before.revision, + diagnostics: [OLD_ERROR], + }).check, + ).toBe(after.check); + }); + + it.each([ + ['saved', { type: 'saved', text: 'x', result: savedResult() }], + ['conflict', { type: 'conflict' }], + ['refused', { type: 'refused', revision: 0, diagnostics: [] }], + ['failed', { type: 'failed', message: 'Late.' }], + ] as const)('ignores a late %s when no save is going on', (_, event) => { + const state = run(opened(), { type: 'rename', id: 's1', name: 'a' }); + + expect(reduce(state, event)).toBe(state); + }); + + it.each([ + ['reloaded', { type: 'reloaded', document: NEW_DOCUMENT }], + ['reload-failed', { type: 'reload-failed', message: 'Late.' }], + ] as const)('ignores a late %s when no reload is going on', (_, event) => { + const state = run(opened(), { type: 'rename', id: 's1', name: 'a' }); + + expect(reduce(state, event)).toBe(state); + }); + + it('does not let a late save answer replace the file of a reload', () => { + const { after } = reloadedAfterEdits(); + + const next = reduce(after, { + type: 'saved', + text: 'old', + result: savedResult(), + }); + + expect(next?.saved.hash).toBe(NEW_DOCUMENT.hash); + }); + + it('keeps that a failed save may have been written', () => { + const state = run( + opened(), + { type: 'rename', id: 's1', name: 'a' }, + { type: 'save' }, + { type: 'failed', message: 'No answer.', uncertain: true }, + ); + + expect(state.save).toEqual({ + phase: 'failed', + message: 'No answer.', + uncertain: true, + }); + }); +}); + +describe('removing a stage that is not in the draft', () => { + it('is not a change', () => { + const state = opened(); + + expect(reduce(state, { type: 'remove', id: 'nope' })).toBe(state); + }); +}); diff --git a/workflow-canvas/src/designer/core/edit-reducer.ts b/workflow-canvas/src/designer/core/edit-reducer.ts index af243997..44294721 100644 --- a/workflow-canvas/src/designer/core/edit-reducer.ts +++ b/workflow-canvas/src/designer/core/edit-reducer.ts @@ -14,7 +14,12 @@ import { setAgent, type Draft, } from './draft'; -import { openDocument, type EditEvent, type EditState } from './edit-state'; +import { + openDocument, + type EditEvent, + type EditState, + type SaveState, +} from './edit-state'; import { canEdit, canSave, isDirty } from './edit-view'; import { checkOf } from './problems'; @@ -119,12 +124,28 @@ function refused( } function reloaded(state: EditState, document: WorkflowDocument): EditState { - const next = openDocument(document); + const next = openDocument(document, state.revision + 1); return next ? { ...next, save: { phase: 'reloaded' } } : { ...state, save: { phase: 'conflict', problem: NOT_EDITABLE } }; } +// The answer to a save counts only while that save is going on; a late answer +// to an earlier one, such as one that comes after a reload, changes nothing. +function whileSaving(state: EditState, apply: () => EditState): EditState { + return state.save.phase === 'saving' ? apply() : state; +} + +function whileReloading(state: EditState, apply: () => EditState): EditState { + return state.save.phase === 'reloading' ? apply() : state; +} + +function failure(message: string, uncertain: true | undefined): SaveState { + return uncertain + ? { phase: 'failed', message, uncertain } + : { phase: 'failed', message }; +} + /** * The next state after an event, or `undefined` while there is nothing to * edit. An event that does not fit, such as a save with nothing to save or a @@ -134,7 +155,9 @@ export function reduce( state: EditState | undefined, event: EditEvent, ): EditState | undefined { - if (event.type === 'open') return openDocument(event.document); + if (event.type === 'open') { + return openDocument(event.document, state ? state.revision + 1 : 0); + } if (!state) return undefined; switch (event.type) { case 'select': @@ -152,18 +175,29 @@ export function reduce( case 'save': return canSave(state) ? { ...state, save: { phase: 'saving' } } : state; case 'saved': - return savedAs(state, event.text, event.result); + return whileSaving(state, () => savedAs(state, event.text, event.result)); case 'conflict': - return { ...state, save: { phase: 'conflict' } }; + return whileSaving(state, () => ({ + ...state, + save: { phase: 'conflict' }, + })); case 'refused': - return refused(state, event.revision, event.diagnostics); + return whileSaving(state, () => + refused(state, event.revision, event.diagnostics), + ); case 'failed': - return { ...state, save: { phase: 'failed', message: event.message } }; + return whileSaving(state, () => ({ + ...state, + save: failure(event.message, event.uncertain), + })); case 'reload': return { ...state, save: { phase: 'reloading' } }; case 'reloaded': - return reloaded(state, event.document); + return whileReloading(state, () => reloaded(state, event.document)); case 'reload-failed': - return { ...state, save: { phase: 'conflict', problem: event.message } }; + return whileReloading(state, () => ({ + ...state, + save: { phase: 'conflict', problem: event.message }, + })); } } diff --git a/workflow-canvas/src/designer/core/edit-state.ts b/workflow-canvas/src/designer/core/edit-state.ts index 2f1795e5..b948a909 100644 --- a/workflow-canvas/src/designer/core/edit-state.ts +++ b/workflow-canvas/src/designer/core/edit-state.ts @@ -23,7 +23,12 @@ export type SaveState = | { phase: 'reloading' } | { phase: 'reloaded' } | { phase: 'conflict'; problem?: string } - | { phase: 'failed'; message: string }; + | { + phase: 'failed'; + message: string; + /** The save may or may not have been written: the answer was lost. */ + uncertain?: true; + }; /** What the Designer edits: the saved workflow, and the draft beside it. */ export interface EditState { @@ -31,7 +36,11 @@ export interface EditState { saved: EditableDocument; draft: Draft; selectedId: string; - /** Counts the changes of the draft; a check answers for one revision. */ + /** + * Counts the changes of the draft; a check answers for one revision. It + * only grows, also across `open` and `reloaded`, so an answer meant for an + * earlier document never fits a later one. + */ revision: number; /** * The latest check. It stays on show while the draft changes, until the @@ -58,7 +67,7 @@ export type EditEvent = revision: number; diagnostics: readonly DiagnosticView[]; } - | { type: 'failed'; message: string } + | { type: 'failed'; message: string; uncertain?: true } | { type: 'reload' } | { type: 'reloaded'; document: WorkflowDocument } | { type: 'reload-failed'; message: string }; @@ -72,9 +81,13 @@ export function isEditable( ); } -/** The state for a document just loaded, or `undefined` if it has no file. */ +/** + * The state for a document just loaded, or `undefined` if it has no file. It + * starts at `revision`, which must be above any revision used before. + */ export function openDocument( document: WorkflowDocument, + revision = 0, ): EditState | undefined { if (!isEditable(document)) return undefined; const draft = draftOf(document.view.stages); @@ -82,8 +95,8 @@ export function openDocument( saved: document, draft, selectedId: draft.stages[0]?.id ?? '', - revision: 0, - check: checkOf(0, document.diagnostics, draft), + revision, + check: checkOf(revision, document.diagnostics, draft), save: { phase: 'idle' }, }; } diff --git a/workflow-canvas/src/designer/core/edit-view.test.ts b/workflow-canvas/src/designer/core/edit-view.test.ts index 955f6b6e..4d8078e6 100644 --- a/workflow-canvas/src/designer/core/edit-view.test.ts +++ b/workflow-canvas/src/designer/core/edit-view.test.ts @@ -108,7 +108,7 @@ describe('saveBlocker', () => { ['saving', [{ type: 'save' }], 'The workflow is being saved.'], [ 'a conflict', - [{ type: 'conflict' }], + [{ type: 'save' }, { type: 'conflict' }], 'The file changed on disk. Reload it first.', ], ['reloading', [{ type: 'reload' }], 'The workflow is being loaded again.'], diff --git a/workflow-canvas/src/designer/core/problems.test.ts b/workflow-canvas/src/designer/core/problems.test.ts index 342a6c48..adbc09bb 100644 --- a/workflow-canvas/src/designer/core/problems.test.ts +++ b/workflow-canvas/src/designer/core/problems.test.ts @@ -11,6 +11,7 @@ import { problemsFor, problemsOf, withEchoes, + type Problem, } from './problems'; const DRAFT = draftOf([ @@ -185,3 +186,26 @@ describe('hasErrors', () => { expect(hasErrors([])).toBe(false); }); }); + +describe('withEchoes, for three stages of one name', () => { + it('marks the first stage once, though two repeats are reported', () => { + const draft = draftOf([ + { name: 'a', agent: 'k' }, + { name: 'a', agent: 'k' }, + { name: 'a', agent: 'k' }, + ]); + const repeat = (stageId: string): Problem => ({ + message: 'Duplicate stage name: a', + severity: 'error', + stageId, + field: 'name', + }); + + const result = withEchoes([repeat('s2'), repeat('s3')], draft); + + expect(result.filter((problem) => problem.echo)).toHaveLength(1); + expect(result.filter((problem) => problem.stageId === 's1')).toHaveLength( + 1, + ); + }); +}); diff --git a/workflow-canvas/src/designer/core/problems.ts b/workflow-canvas/src/designer/core/problems.ts index c7f9f890..d5b20456 100644 --- a/workflow-canvas/src/designer/core/problems.ts +++ b/workflow-canvas/src/designer/core/problems.ts @@ -89,15 +89,17 @@ export function withEchoes( ): Problem[] { const named = problems.filter((problem) => problem.field === 'name'); const marked = new Set(named.map((problem) => problem.stageId)); + const echoed = new Set(); const echoes = named.flatMap((problem) => { const at = draft.stages.findIndex(({ id }) => id === problem.stageId); + if (at < 0) return []; const name = draft.stages[at]?.name; const earlier = draft.stages - .slice(0, Math.max(at, 0)) + .slice(0, at) .find((stage) => stage.name === name && !marked.has(stage.id)); - return earlier - ? [{ ...problem, stageId: earlier.id, echo: true as const }] - : []; + if (!earlier || echoed.has(earlier.id)) return []; + echoed.add(earlier.id); + return [{ ...problem, stageId: earlier.id, echo: true as const }]; }); return [...problems, ...echoes]; } diff --git a/workflow-canvas/src/designer/core/stage-text.test.ts b/workflow-canvas/src/designer/core/stage-text.test.ts index b763f5f3..357e72df 100644 --- a/workflow-canvas/src/designer/core/stage-text.test.ts +++ b/workflow-canvas/src/designer/core/stage-text.test.ts @@ -8,6 +8,33 @@ const VIEW = { stages: [{ name: 'draft', agent: 'k-developer' }], }; +const COMMENTED = [ + '# about the workflow', + 'name: wf', + 'intent: Say hi', + 'stages:', + ' # the first one', + ' - {name: a, agent: x} # flow style', + '', + ' # the second one', + ' - agent: y', + " name: 'b'", + ' - name: c', + ' agent: z', + ' # about the end', + '# footer', + '', +].join('\n'); +const COMMENTED_VIEW = { + name: 'wf', + intent: { outcome: 'Say hi' }, + stages: [ + { name: 'a', agent: 'x' }, + { name: 'b', agent: 'y' }, + { name: 'c', agent: 'z' }, + ], +}; + const TEXT = 'name: hello\nintent: Add a greeting to the README\nstages:\n - name: draft\n agent: k-developer\n - name: review\n agent: k-quality-assurance\n'; @@ -39,7 +66,7 @@ describe('withStages', () => { ]); expect(next).toBe( - "# Greets\nversion: '1.1'\nname: hello\nintent: Say hi\n\nstages: # in order\n - name: a\n agent: k-developer\n - name: b\n agent: k-tpm\n\n# the end\n", + "# Greets\nversion: '1.1'\nname: hello\nintent: Say hi\n\nstages: # in order\n # first\n - name: a\n agent: k-developer\n - name: b\n agent: k-tpm\n\n# the end\n", ); }); @@ -161,12 +188,152 @@ describe('willRewriteWhole', () => { it('agrees with what withStages does', () => { const text = - 'name: hello\n# kept\nstages:\n # dropped\n - name: a\n agent: k-tpm\n'; + 'name: hello\n# kept\nstages:\n - name: a\n agent: k-tpm\n'; - const next = withStages(text, VIEW, [{ name: 'a', agent: 'k-tpm' }]); + const next = withStages(text, VIEW, [{ name: 'b', agent: 'k-tpm' }]); expect(willRewriteWhole(text)).toBe(false); expect(next).toContain('# kept'); - expect(next).not.toContain('# dropped'); }); + + it.each([ + [ + 'a lone CR as the line break', + 'name: a\rstages:\r - name: a\r agent: x\r', + ], + ['a BOM before stages', '\uFEFFstages:\n - name: a\n agent: x\n'], + ['a double-quoted key', 'name: a\n"stages":\n - name: a\n agent: x\n'], + ['a single-quoted key', "name: a\n'stages':\n - name: a\n agent: x\n"], + ])('does not rewrite the whole file for %s', (_, text) => { + expect(willRewriteWhole(text)).toBe(false); + }); +}); + +describe('withStages, on a file with comments in the block', () => { + const [A, B, C] = COMMENTED_VIEW.stages; + + it('keeps the lines and comments of the stages that are unchanged', () => { + const next = withStages(COMMENTED, COMMENTED_VIEW, [ + A!, + B!, + { name: 'c', agent: 'w' }, + ]); + + expect(next).toBe( + COMMENTED.replace( + ' - name: c\n agent: z\n', + ' - name: c\n agent: w\n', + ), + ); + }); + + it('takes the comments in front of a deleted stage along with it', () => { + const next = withStages(COMMENTED, COMMENTED_VIEW, [A!, C!]); + + expect(next).not.toContain('the second one'); + expect(next).toContain('the first one'); + expect(next).toContain('flow style'); + expect(next).toContain(' # about the end\n# footer\n'); + }); + + it('keeps every comment and blank line after the last stage, indented or not', () => { + const next = withStages(COMMENTED, COMMENTED_VIEW, [A!]); + + expect(next.endsWith(' # about the end\n# footer\n')).toBe(true); + }); + + it('moves the lines of a stage that moved, with its comments', () => { + const next = withStages(COMMENTED, COMMENTED_VIEW, [B!, A!, C!]); + + expect(next.indexOf('the second one')).toBeLessThan( + next.indexOf("name: 'b'"), + ); + expect(next.indexOf("name: 'b'")).toBeLessThan(next.indexOf('flow style')); + }); + + it('writes a new stage in the plain layout after the others', () => { + const next = withStages(COMMENTED, COMMENTED_VIEW, [ + A!, + B!, + C!, + { name: 'd', agent: 'v' }, + ]); + + expect(next).toContain( + ' - name: c\n agent: z\n - name: d\n agent: v\n', + ); + }); + + it('writes every stage afresh when the items do not match the view', () => { + const next = withStages(COMMENTED, { ...COMMENTED_VIEW, stages: [A!] }, [ + A!, + B!, + ]); + + expect(next).toContain( + ' - name: a\n agent: x\n - name: b\n agent: "y"\n', + ); + }); +}); + +describe('withStages, on line endings', () => { + const ONE = { + name: 'wf', + intent: { outcome: 'x' }, + stages: [{ name: 'a', agent: 'x' }], + }; + + it('uses the line ending of the stages line in a file with mixed endings', () => { + const text = + 'name: wf\nintent: x\n# c\r\nstages:\n - name: a\n agent: x\n'; + + const next = withStages(text, ONE, [ + { name: 'a', agent: 'x' }, + { name: 'b', agent: 'k-tpm' }, + ]); + + expect(next).toBe(`${text} - name: b\n agent: k-tpm\n`); + }); + + it('keeps a file of lone CR as it is, and writes new lines with CR', () => { + const text = 'name: wf\rintent: x\rstages:\r - name: a\r agent: x\r'; + + const next = withStages(text, ONE, [ + { name: 'a', agent: 'x' }, + { name: 'b', agent: 'k-tpm' }, + ]); + + expect(next).toBe(`${text} - name: b\r agent: k-tpm\r`); + }); + + it('keeps a BOM in front of the stages key', () => { + const text = '\uFEFFstages:\n - name: a\n agent: x\n'; + + const next = withStages(text, ONE, [{ name: 'a', agent: 'k-tpm' }]); + + expect(next).toBe('\uFEFFstages:\n - name: a\n agent: k-tpm\n'); + }); + + it('writes into a block under a quoted key', () => { + const text = 'name: wf\n"stages":\n - name: a\n agent: x\n'; + + const next = withStages(text, ONE, [{ name: 'a', agent: 'k-tpm' }]); + + expect(next).toBe('name: wf\n"stages":\n - name: a\n agent: k-tpm\n'); + }); +}); + +describe('withStages, on words that YAML 1.1 reads as booleans', () => { + it.each(['y', 'n', 'yes', 'no', 'on', 'off', 'Yes', 'OFF'])( + 'quotes the name %s', + (name) => { + const next = withStages( + 'name: wf\nstages:\n - name: a\n agent: x\n', + VIEW, + [{ name, agent: 'x' }], + ); + + expect(next).toContain(`- name: "${name}"`); + }, + ); }); diff --git a/workflow-canvas/src/designer/core/stage-text.ts b/workflow-canvas/src/designer/core/stage-text.ts index 17ca5b55..d75fe5a6 100644 --- a/workflow-canvas/src/designer/core/stage-text.ts +++ b/workflow-canvas/src/designer/core/stage-text.ts @@ -3,9 +3,10 @@ import type { StageView, WorkflowView } from '../../api/types'; // A value that a plain YAML scalar reads back as the same text: it starts // with a letter, has no character that YAML gives a meaning, and does not end -// in a space. `true`, `false` and `null` are left out, as they are not text. +// in a space. Words that YAML 1.1 reads as booleans or null are left out, so +// an editor on that version reads the same text. const PLAIN = /^[A-Za-z](?:[A-Za-z0-9 _.,()/-]*[A-Za-z0-9_.,()/-])?$/; -const NOT_TEXT = /^(?:true|false|null)$/i; +const NOT_TEXT = /^(?:true|false|null|y|n|yes|no|on|off)$/i; // The value as YAML text: plain when that is safe, double-quoted if not. function scalar(value: string): string { @@ -14,24 +15,91 @@ function scalar(value: string): string { : JSON.stringify(value); } -function stageLines(stages: readonly StageView[]): string[] { - return stages.flatMap((stage) => [ - ` - name: ${scalar(stage.name)}`, - ` agent: ${scalar(stage.agent)}`, - ]); +/** One line of a file and the line break that ends it (empty at the end). */ +interface Line { + text: string; + eol: string; } -// The `stages:` key of the top level, with an optional comment after it. -const STAGES_KEY = /^stages:[ \t]*(?:#.*)?\r?$/; +// The contract takes `\r\n`, `\n` and a lone `\r` as line breaks. +function linesOf(text: string): Line[] { + const lines: Line[] = []; + let end = 0; + for (const match of text.matchAll(/([^\r\n]*)(\r\n|\r|\n)/g)) { + lines.push({ text: match[1] ?? '', eol: match[2] ?? '' }); + end = match.index + match[0].length; + } + if (end < text.length) lines.push({ text: text.slice(end), eol: '' }); + return lines; +} + +const BOM = '\uFEFF'; +// The `stages:` key of the top level, quoted or not, with an optional comment. +const STAGES_KEY = /^(?:stages|"stages"|'stages'):[ \t]*(?:#.*)?$/; // A line that starts the next top-level key or a document marker, which // ends the block of stages. A `-` at the start is an item of the block. const NEXT_KEY = /^(?:[^\s#-]|---|\.\.\.)/; -// `.` stops at a `\r`, so a comment of a CRLF file is matched with `[^\n]`. -const BLANK_OR_COMMENT = /^(?:\s*|#[^\n]*)$/; +const BLANK_OR_COMMENT = /^\s*(?:#.*)?$/; +const ITEM_START = /^[ \t]*-(?:[ \t]|$)/; + +const isFiller = (line: Line) => BLANK_OR_COMMENT.test(line.text); + +// A stage as the file wrote it: the lines of its item, with the comments and +// blank lines in front of it. +interface Item { + stage: StageView; + lines: Line[]; +} + +// Splits the lines of the block at the start of each item. Comments and blank +// lines between two items go with the one after them. +function itemsOf(block: Line[], stages: readonly StageView[]): Item[] { + const starts = block.flatMap((line, i) => + ITEM_START.test(line.text) ? i : [], + ); + if (starts.length !== stages.length) return []; + let lead: Line[] = []; + return starts.map((start, n) => { + const body = block.slice(start, starts[n + 1] ?? block.length); + let cut = body.length; + while (cut > 1 && isFiller(body[cut - 1]!)) cut -= 1; + const item = { stage: stages[n]!, lines: [...lead, ...body.slice(0, cut)] }; + lead = body.slice(cut); + return item; + }); +} + +function generated(stage: StageView, eol: string): Line[] { + return [ + { text: ` - name: ${scalar(stage.name)}`, eol }, + { text: ` agent: ${scalar(stage.agent)}`, eol }, + ]; +} + +const sameStage = (a: StageView, b: StageView) => + a.name === b.name && a.agent === b.agent; -// The file's own line ending, so a file kept in CRLF stays in CRLF. -function endingOf(text: string): string { - return text.includes('\r\n') ? '\r\n' : '\n'; +// The lines of `stage`: the file's own for a stage that is unchanged, so its +// comments and layout stay, and new ones for a stage that is new or changed. +function linesOfStage(stage: StageView, unused: Item[], eol: string): Line[] { + const at = unused.findIndex((item) => sameStage(item.stage, stage)); + return at < 0 ? generated(stage, eol) : unused.splice(at, 1)[0]!.lines; +} + +// Where the block of stages is in `lines`, or `undefined` when the file has no +// `stages:` key of the block form at the top level. `end` stops before the +// comments and blank lines that trail the last stage. +function blockIn(lines: Line[]): { key: number; end: number } | undefined { + const key = lines.findIndex((line, i) => + STAGES_KEY.test(i === 0 ? line.text.replace(BOM, '') : line.text), + ); + if (key < 0) return undefined; + const next = lines.findIndex( + (line, i) => i > key && NEXT_KEY.test(line.text), + ); + let end = next < 0 ? lines.length : next; + while (end > key + 1 && isFiller(lines[end - 1]!)) end -= 1; + return { key, end }; } // Swaps the block of stages of `text` for `stages`, and leaves every other @@ -39,51 +107,61 @@ function endingOf(text: string): string { // at the top level. function replaceStageBlock( text: string, + view: WorkflowView, stages: readonly StageView[], ): string | undefined { - const lines = text.split('\n'); - const key = lines.findIndex((line) => STAGES_KEY.test(line)); - if (key < 0) return undefined; - const next = lines.findIndex( - (line, index) => index > key && NEXT_KEY.test(line), - ); - let end = next < 0 ? lines.length : next; - // Blank lines and comments after the last stage belong to what follows. - while (end > key + 1 && BLANK_OR_COMMENT.test(lines[end - 1] ?? '')) end -= 1; + const lines = linesOf(text); + const found = blockIn(lines); + if (!found) return undefined; + const { key, end } = found; + const block = lines.slice(key + 1, end); + const firstItem = block.findIndex((line) => ITEM_START.test(line.text)); + const head = firstItem < 0 ? block : block.slice(0, firstItem); + const unused = itemsOf(block.slice(head.length), view.stages); + const eol = lines[key]!.eol || '\n'; + const body = stages.flatMap((stage) => linesOfStage(stage, unused, eol)); const tail = lines.slice(end); - // A block that ends the file ends it without a line break, as it was. - const cr = endingOf(text) === '\r\n' ? '\r' : ''; - const block = stageLines(stages).map((line, index, all) => - tail.length === 0 && index === all.length - 1 ? line : `${line}${cr}`, + // A block that ends the file ends it as the file did; any other line break + // is the one of the `stages:` line. + const lastEol = tail.length === 0 ? (lines[end - 1]?.eol ?? '') : eol; + const written = [...head, ...body].map((line, i, all) => + i === all.length - 1 + ? { ...line, eol: lastEol } + : { ...line, eol: line.eol || eol }, ); - return [...lines.slice(0, key + 1), ...block, ...tail].join('\n'); + return [...lines.slice(0, key + 1), ...written, ...tail] + .map((line) => line.text + line.eol) + .join(''); } // A whole file written from the view: the layout that nothing else fits. function writeWhole(view: WorkflowView, stages: readonly StageView[]): string { + const lines = stages.flatMap((stage) => generated(stage, '\n')); return [ `name: ${scalar(view.name)}`, `intent: ${scalar(view.intent.outcome)}`, 'stages:', - ...stageLines(stages), + ...lines.map((line) => line.text), '', ].join('\n'); } /** * The text of the workflow with `stages` in place of its own. Only the block - * of stages changes: the name, the intent, `version` and comments outside the - * block stay as they are, and comments inside the block go with its old - * stages. A file whose `stages` are not a block at the top level is written - * whole from `view`, in the plain layout, which drops its comments and - * `version`. + * of stages changes: the name, the intent, `version`, the line endings and the + * comments outside the block stay as they are. A stage that is unchanged keeps + * its own lines, with the comments in front of it; a stage that is new or + * changed is written in the plain layout, and the comments written inside a + * stage that is changed or deleted go with it. A file whose `stages` are not a + * block at the top level is written whole from `view`, in the plain layout, + * which drops its comments and `version`. */ export function withStages( text: string, view: WorkflowView, stages: readonly StageView[], ): string { - return replaceStageBlock(text, stages) ?? writeWhole(view, stages); + return replaceStageBlock(text, view, stages) ?? writeWhole(view, stages); } /** @@ -92,5 +170,5 @@ export function withStages( * top level. */ export function willRewriteWhole(text: string): boolean { - return replaceStageBlock(text, []) === undefined; + return blockIn(linesOf(text)) === undefined; } diff --git a/workflow-canvas/test-support/draft-contract.test.ts b/workflow-canvas/test-support/draft-contract.test.ts new file mode 100644 index 00000000..99d6a76e --- /dev/null +++ b/workflow-canvas/test-support/draft-contract.test.ts @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: Apache-2.0 +/** Checks that the limits of the draft are those of api/openapi.yaml. */ +import { describe, expect, it } from 'vitest'; +import { MAX_STAGES } from '../src/designer/core/draft.ts'; +import { schemaOf } from '../stub-api/test-support/contract.ts'; + +type Json = Record; + +describe('the draft and api/openapi.yaml', () => { + it('allow the same number of stages', () => { + const properties = schemaOf('WorkflowView').properties as Record< + string, + Json + >; + + expect(properties.stages?.maxItems).toBe(MAX_STAGES); + }); +}); From 9f560cd47540a2c01fa476c3d923d6a5dee771d2 Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Wed, 28 Oct 2026 19:44:18 +0000 Subject: [PATCH 4/5] Write new stages in the layout of the file's list, and test every layout with the validator A stage that is new or changed is written with its dashes where the file's own items have them, so a list with four spaces, three, or none stays one list. A list that cannot take items, such as a flow list on the line below stages, is written whole, which the notice already warns about. A kept last stage keeps its own line ending in a file with mixed endings. --- .../src/designer/core/edit-view.ts | 4 +- .../src/designer/core/stage-text.test.ts | 22 +- .../src/designer/core/stage-text.ts | 134 ++++++++--- .../test-support/stage-text-layouts.test.ts | 225 ++++++++++++++++++ 4 files changed, 347 insertions(+), 38 deletions(-) create mode 100644 workflow-canvas/test-support/stage-text-layouts.test.ts diff --git a/workflow-canvas/src/designer/core/edit-view.ts b/workflow-canvas/src/designer/core/edit-view.ts index 5939fb8d..7f49ce4c 100644 --- a/workflow-canvas/src/designer/core/edit-view.ts +++ b/workflow-canvas/src/designer/core/edit-view.ts @@ -23,7 +23,9 @@ export function textOf(state: EditState): string { return withStages(text, view, stagesOf(state.draft)); } -// The findings that speak for the draft as it is now. +// The findings that speak for the draft as it is now. After a change they are +// gone until the check of the new draft answers; a check that failed has no +// event, so Save is open again then and the server checks the text on save. function currentFindings(state: EditState) { return state.check.revision === state.revision ? state.check.diagnostics : []; } diff --git a/workflow-canvas/src/designer/core/stage-text.test.ts b/workflow-canvas/src/designer/core/stage-text.test.ts index 357e72df..425a8cc2 100644 --- a/workflow-canvas/src/designer/core/stage-text.test.ts +++ b/workflow-canvas/src/designer/core/stage-text.test.ts @@ -77,7 +77,7 @@ describe('withStages', () => { const next = withStages(text, VIEW, [{ name: 'b', agent: 'k-tpm' }]); expect(next).toBe( - 'name: hello\nstages:\n - name: b\n agent: k-tpm\nintent: Say hi\n', + 'name: hello\nstages:\n- name: b\n agent: k-tpm\nintent: Say hi\n', ); }); @@ -295,6 +295,26 @@ describe('withStages, on line endings', () => { expect(next).toBe(`${text} - name: b\n agent: k-tpm\n`); }); + it('keeps the line ending of a kept last stage in a file with mixed endings', () => { + const text = + 'name: wf\nintent: x\r\nstages:\n - name: a\n agent: x\n - name: b\r\n agent: y\r\n# end\n'; + const view = { + ...ONE, + stages: [ + { name: 'a', agent: 'x' }, + { name: 'b', agent: 'y' }, + ], + }; + + const next = withStages(text, view, [ + { name: 'a', agent: 'z' }, + { name: 'b', agent: 'y' }, + ]); + + expect(next).toBe(text.replace('agent: x', 'agent: z')); + expect(withStages(text, view, view.stages)).toBe(text); + }); + it('keeps a file of lone CR as it is, and writes new lines with CR', () => { const text = 'name: wf\rintent: x\rstages:\r - name: a\r agent: x\r'; diff --git a/workflow-canvas/src/designer/core/stage-text.ts b/workflow-canvas/src/designer/core/stage-text.ts index d75fe5a6..69956302 100644 --- a/workflow-canvas/src/designer/core/stage-text.ts +++ b/workflow-canvas/src/designer/core/stage-text.ts @@ -19,6 +19,8 @@ function scalar(value: string): string { interface Line { text: string; eol: string; + /** Set on a line that the writer made, as opposed to one the file had. */ + made?: true; } // The contract takes `\r\n`, `\n` and a lone `\r` as line breaks. @@ -41,8 +43,11 @@ const STAGES_KEY = /^(?:stages|"stages"|'stages'):[ \t]*(?:#.*)?$/; const NEXT_KEY = /^(?:[^\s#-]|---|\.\.\.)/; const BLANK_OR_COMMENT = /^\s*(?:#.*)?$/; const ITEM_START = /^[ \t]*-(?:[ \t]|$)/; +// Where the dashes go in a list that has no item yet. +const DEFAULT_DASH = ' '; const isFiller = (line: Line) => BLANK_OR_COMMENT.test(line.text); +const indentOf = (line: Line) => /^[ \t]*/.exec(line.text)![0]; // A stage as the file wrote it: the lines of its item, with the comments and // blank lines in front of it. @@ -51,12 +56,40 @@ interface Item { lines: Line[]; } -// Splits the lines of the block at the start of each item. Comments and blank -// lines between two items go with the one after them. -function itemsOf(block: Line[], stages: readonly StageView[]): Item[] { - const starts = block.flatMap((line, i) => - ITEM_START.test(line.text) ? i : [], +// How the file lays out its list: the comments in front of the first item, +// the lines where the items start, and the indent of their dashes. An item +// is a line at the shortest dash indent; a dash further in belongs to the +// item above it, as in a list of tags. +interface Layout { + head: Line[]; + starts: number[]; + dash: string; +} + +// The layout of `block`, or `undefined` when something other than comments +// stands in front of the first item, such as a list in the flow form on the +// line below `stages:`. Such a list cannot take items next to it. +function layoutOf(block: Line[]): Layout | undefined { + const dashes = block.flatMap((line, i) => + ITEM_START.test(line.text) ? [{ i, indent: indentOf(line) }] : [], + ); + const dash = dashes.reduce( + (least, { indent }) => (indent.length < least.length ? indent : least), + dashes[0]?.indent ?? DEFAULT_DASH, ); + const starts = dashes.filter((d) => d.indent === dash).map((d) => d.i); + const head = block.slice(0, starts[0] ?? block.length); + return head.every(isFiller) ? { head, starts, dash } : undefined; +} + +// Splits the lines after the head at the start of each item. Comments and +// blank lines between two items go with the one after them. Empty when the +// items cannot be matched one to one with `stages`. +function itemsOf( + block: Line[], + { starts }: Layout, + stages: readonly StageView[], +): Item[] { if (starts.length !== stages.length) return []; let lead: Line[] = []; return starts.map((start, n) => { @@ -69,10 +102,12 @@ function itemsOf(block: Line[], stages: readonly StageView[]): Item[] { }); } -function generated(stage: StageView, eol: string): Line[] { +// A new item in the layout of the list: its dashes where the others have +// them, and the agent in line with the name. +function generated(stage: StageView, eol: string, dash: string): Line[] { return [ - { text: ` - name: ${scalar(stage.name)}`, eol }, - { text: ` agent: ${scalar(stage.agent)}`, eol }, + { text: `${dash}- name: ${scalar(stage.name)}`, eol, made: true }, + { text: `${dash} agent: ${scalar(stage.agent)}`, eol, made: true }, ]; } @@ -81,9 +116,14 @@ const sameStage = (a: StageView, b: StageView) => // The lines of `stage`: the file's own for a stage that is unchanged, so its // comments and layout stay, and new ones for a stage that is new or changed. -function linesOfStage(stage: StageView, unused: Item[], eol: string): Line[] { +function linesOfStage( + stage: StageView, + unused: Item[], + eol: string, + dash: string, +): Line[] { const at = unused.findIndex((item) => sameStage(item.stage, stage)); - return at < 0 ? generated(stage, eol) : unused.splice(at, 1)[0]!.lines; + return at < 0 ? generated(stage, eol, dash) : unused.splice(at, 1)[0]!.lines; } // Where the block of stages is in `lines`, or `undefined` when the file has no @@ -102,33 +142,54 @@ function blockIn(lines: Line[]): { key: number; end: number } | undefined { return { key, end }; } +// The block of stages of a file, as far as the writer can edit it in place. +interface Plan { + lines: Line[]; + key: number; + end: number; + block: Line[]; + layout: Layout; +} + +function planFor(text: string): Plan | undefined { + const lines = linesOf(text); + const found = blockIn(lines); + if (!found) return undefined; + const block = lines.slice(found.key + 1, found.end); + const layout = layoutOf(block); + return layout && { lines, ...found, block, layout }; +} + +// The line break a line ends with. A new line takes the one of the `stages:` +// line. The last line of a block that ends the file ends it as the file did. +function breakOf(line: Line, last: boolean, eol: string, fileEnd?: string) { + if (last && fileEnd !== undefined) return fileEnd; + return line.made ? eol : line.eol || eol; +} + // Swaps the block of stages of `text` for `stages`, and leaves every other -// byte alone. `undefined` when the text has no `stages:` key of the block form -// at the top level. +// byte alone. `undefined` when the stages are not a block that new items can +// join: no `stages:` key of the block form at the top level, or a list of +// another form. function replaceStageBlock( text: string, view: WorkflowView, stages: readonly StageView[], ): string | undefined { - const lines = linesOf(text); - const found = blockIn(lines); - if (!found) return undefined; - const { key, end } = found; - const block = lines.slice(key + 1, end); - const firstItem = block.findIndex((line) => ITEM_START.test(line.text)); - const head = firstItem < 0 ? block : block.slice(0, firstItem); - const unused = itemsOf(block.slice(head.length), view.stages); + const plan = planFor(text); + if (!plan) return undefined; + const { lines, key, end, block, layout } = plan; + const unused = itemsOf(block, layout, view.stages); const eol = lines[key]!.eol || '\n'; - const body = stages.flatMap((stage) => linesOfStage(stage, unused, eol)); - const tail = lines.slice(end); - // A block that ends the file ends it as the file did; any other line break - // is the one of the `stages:` line. - const lastEol = tail.length === 0 ? (lines[end - 1]?.eol ?? '') : eol; - const written = [...head, ...body].map((line, i, all) => - i === all.length - 1 - ? { ...line, eol: lastEol } - : { ...line, eol: line.eol || eol }, + const body = stages.flatMap((stage) => + linesOfStage(stage, unused, eol, layout.dash), ); + const tail = lines.slice(end); + const fileEnd = tail.length === 0 ? (lines[end - 1]?.eol ?? '') : undefined; + const written = [...layout.head, ...body].map((line, i, all) => ({ + ...line, + eol: breakOf(line, i === all.length - 1, eol, fileEnd), + })); return [...lines.slice(0, key + 1), ...written, ...tail] .map((line) => line.text + line.eol) .join(''); @@ -136,7 +197,7 @@ function replaceStageBlock( // A whole file written from the view: the layout that nothing else fits. function writeWhole(view: WorkflowView, stages: readonly StageView[]): string { - const lines = stages.flatMap((stage) => generated(stage, '\n')); + const lines = stages.flatMap((stage) => generated(stage, '\n', DEFAULT_DASH)); return [ `name: ${scalar(view.name)}`, `intent: ${scalar(view.intent.outcome)}`, @@ -151,9 +212,10 @@ function writeWhole(view: WorkflowView, stages: readonly StageView[]): string { * of stages changes: the name, the intent, `version`, the line endings and the * comments outside the block stay as they are. A stage that is unchanged keeps * its own lines, with the comments in front of it; a stage that is new or - * changed is written in the plain layout, and the comments written inside a - * stage that is changed or deleted go with it. A file whose `stages` are not a - * block at the top level is written whole from `view`, in the plain layout, + * changed is written with its dashes where the file's own have them, and the + * comments written inside a stage that is changed or deleted go with it. A + * file whose `stages` are not a list of items under a top-level key, such as + * a list in the flow form, is written whole from `view`, in the plain layout, * which drops its comments and `version`. */ export function withStages( @@ -166,9 +228,9 @@ export function withStages( /** * Whether a save of `text` would write the file whole, in the plain layout, - * and so drop its comments and `version`: its `stages` are not a block at the - * top level. + * and so drop its comments and `version`: its `stages` are not a list of items + * under a top-level key. */ export function willRewriteWhole(text: string): boolean { - return blockIn(linesOf(text)) === undefined; + return planFor(text) === undefined; } diff --git a/workflow-canvas/test-support/stage-text-layouts.test.ts b/workflow-canvas/test-support/stage-text-layouts.test.ts new file mode 100644 index 00000000..7d41c657 --- /dev/null +++ b/workflow-canvas/test-support/stage-text-layouts.test.ts @@ -0,0 +1,225 @@ +// SPDX-License-Identifier: Apache-2.0 +/** + * Writes edits into files of many list layouts and checks every result with + * the validator of the stub, so a layout that the writer mixes up fails. + */ +import { describe, expect, it } from 'vitest'; +import type { StageView, WorkflowView } from '../src/api/types.ts'; +import { + willRewriteWhole, + withStages, +} from '../src/designer/core/stage-text.ts'; +import { checkWorkflowText } from '../stub-api/core/validator.ts'; + +/** Writes the item of a stage, its dashes at `dash`; `n` picks a variation. */ +type Item = (stage: StageView, dash: string, n: number) => string[]; + +const plain: Item = (s, d) => [ + `${d}- name: ${s.name}`, + `${d} agent: ${s.agent}`, +]; +const agentFirst: Item = (s, d) => [ + `${d}- agent: ${s.agent}`, + `${d} name: ${s.name}`, +]; +const flow: Item = (s, d) => [ + `${d}- {name: ${s.name}, agent: ${s.agent}} # flow`, +]; +const dashAlone: Item = (s, d) => [ + `${d}-`, + `${d} name: ${s.name}`, + `${d} agent: ${s.agent}`, +]; +const quoted: Item = (s, d) => [ + `${d}- name: "${s.name}"`, + `${d} agent: '${s.agent}'`, +]; +const commented: Item = (s, d) => [ + `${d}# about ${s.name}`, + ...plain(s, d, 0), + `${d} # inside`, +]; +const mixed: Item = (s, d, n) => + [plain, agentFirst, flow, dashAlone, quoted][n % 5]!(s, d, n); + +const BLOCK_LAYOUTS: Array<[string, string, Item]> = [ + ['two spaces', ' ', plain], + ['four spaces', ' ', plain], + ['three spaces', ' ', plain], + ['no indent', '', plain], + ['agent first', ' ', agentFirst], + ['flow items', '', flow], + ['dash on its own line', ' ', dashAlone], + ['dash on its own line, no indent', '', dashAlone], + ['quoted values', ' ', quoted], + ['comments around items', ' ', commented], + ['items of different forms', ' ', mixed], + ['items of different forms, no indent', '', mixed], + ['items of different forms, four spaces', ' ', mixed], +]; + +const FLOW_LAYOUTS: Array<[string, (stages: StageView[]) => string[]]> = [ + [ + 'a flow list on the line below stages', + (s) => [ + ` [${s.map((x) => `{name: ${x.name}, agent: ${x.agent}}`).join(', ')}]`, + ], + ], + [ + 'a flow list on several lines', + (s) => [ + ' [', + ...s.map((x) => ` {name: ${x.name}, agent: ${x.agent}},`), + ' ]', + ], + ], +]; + +const ENDINGS: Array<[string, string]> = [ + ['LF', '\n'], + ['CRLF', '\r\n'], + ['CR', '\r'], +]; + +// A small generator with a fixed seed, so a failure repeats. +function random(seed: number): () => number { + let a = seed; + return () => { + a = (a + 0x6d2b79f5) | 0; + let t = Math.imul(a ^ (a >>> 15), 1 | a); + t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t; + return ((t ^ (t >>> 14)) >>> 0) / 4294967296; + }; +} + +const HEAD = ['# a workflow', 'name: wf', 'intent: Say hi', '']; +const FOOT = ['# footer', '']; + +function stagesOf(count: number, agents: string[]): StageView[] { + return Array.from({ length: count }, (_, i) => ({ + name: `s${i + 1}`, + agent: agents[i % agents.length]!, + })); +} + +function viewOf(stages: StageView[]): WorkflowView { + return { name: 'wf', intent: { outcome: 'Say hi' }, stages }; +} + +function fileOf(eol: string, lines: string[]): string { + return [...HEAD, 'stages:', ...lines, ...FOOT].join(eol); +} + +/** The edits of a list: each is a new list of stages. */ +function editsOf(stages: StageView[], pick: () => number): StageView[][] { + const fresh = { name: 'extra', agent: 'k-tpm' }; + const at = Math.floor(pick() * (stages.length + 1)); + return [ + [...stages, fresh], + [fresh, ...stages], + [...stages.slice(0, at), fresh, ...stages.slice(at)], + ...stages.map((_, i) => + stages.map((t, j) => (i === j ? { ...t, name: `${t.name}-b` } : t)), + ), + ...stages.map((_, i) => + stages.map((t, j) => (i === j ? { ...t, agent: 'k-architect' } : t)), + ), + ...stages + .map((_, i) => stages.filter((__, j) => i !== j)) + .filter((x) => x.length > 0), + [...stages].reverse(), + [...stages, fresh, { name: 'extra-2', agent: 'k-tpm' }], + ]; +} + +function expectValid(text: string, stages: StageView[], label: string) { + const result = checkWorkflowText(text, 'wf'); + expect( + result.diagnostics.map((d) => d.code), + label, + ).toEqual([]); + expect(result.view?.stages, label).toEqual(stages); +} + +describe('withStages in every layout of the list', () => { + it.each(BLOCK_LAYOUTS)( + 'writes valid text in a list with %s', + (name, dash, item) => { + const pick = random(name.length); + for (const [, eol] of ENDINGS) { + for (let count = 1; count <= 5; count += 1) { + const stages = stagesOf(count, ['k-developer', 'k-tpm', 'x']); + const text = fileOf( + eol, + stages.flatMap((s, n) => item(s, dash, n)), + ); + expectValid(text, stages, `${name} as it is`); + + for (const next of editsOf(stages, pick)) { + const written = withStages(text, viewOf(stages), next); + + expect(willRewriteWhole(text), name).toBe(false); + expectValid(written, next, `${name} ${JSON.stringify(next)}`); + expect( + written.startsWith(`${HEAD.join(eol)}${eol}stages:${eol}`), + ).toBe(true); + expect(written.endsWith(FOOT.join(eol))).toBe(true); + } + } + } + }, + ); + + it.each(BLOCK_LAYOUTS)( + 'writes the same text back for no edit in a list with %s', + (_name, dash, item) => { + const stages = stagesOf(4, ['k-developer', 'k-tpm']); + const text = fileOf( + '\n', + stages.flatMap((s, n) => item(s, dash, n)), + ); + + expect(withStages(text, viewOf(stages), stages)).toBe(text); + }, + ); + + it.each(BLOCK_LAYOUTS)( + 'puts a new stage under the dashes of the others in a list with %s', + (_name, dash, item) => { + const stages = stagesOf(2, ['k-developer']); + const text = fileOf( + '\n', + stages.flatMap((s, n) => item(s, dash, n)), + ); + + const written = withStages(text, viewOf(stages), [ + ...stages, + { name: 'extra', agent: 'k-tpm' }, + ]); + + expect(written).toContain( + `\n${dash}- name: extra\n${dash} agent: k-tpm\n`, + ); + }, + ); +}); + +describe('withStages when the list cannot take new items', () => { + it.each(FLOW_LAYOUTS)( + 'writes the whole file, and valid text, for %s', + (_name, lines) => { + const stages = stagesOf(2, ['k-developer']); + const text = fileOf('\n', lines(stages)); + expectValid(text, stages, 'as it is'); + + for (const next of editsOf(stages, random(7))) { + expect(willRewriteWhole(text)).toBe(true); + expectValid( + withStages(text, viewOf(stages), next), + next, + JSON.stringify(next), + ); + } + }, + ); +}); From e4dbaa117dd6f10decee9e8ef8d95f1061730fac Mon Sep 17 00:00:00 2001 From: Nihit <24500178+knihit@users.noreply.github.com> Date: Wed, 28 Oct 2026 21:15:03 +0000 Subject: [PATCH 5/5] Give the diagnostic in the backtick test its path and severity --- workflow-canvas/src/designer/Designer.test.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/workflow-canvas/src/designer/Designer.test.tsx b/workflow-canvas/src/designer/Designer.test.tsx index 1e20ce14..734372a9 100644 --- a/workflow-canvas/src/designer/Designer.test.tsx +++ b/workflow-canvas/src/designer/Designer.test.tsx @@ -90,7 +90,12 @@ describe('Designer', () => { ...HELLO_DOCUMENT, view: null, diagnostics: [ - { message: 'The mapping needs the key `intent`.', line: 2 }, + { + message: 'The mapping needs the key `intent`.', + line: 2, + path: '', + severity: 'error' as const, + }, ], };