diff --git a/workflow-canvas/README.md b/workflow-canvas/README.md index e26ff0bb..ff392820 100644 --- a/workflow-canvas/README.md +++ b/workflow-canvas/README.md @@ -7,7 +7,7 @@ of the agent package or the CLI. The page has Designer and Execution tabs. The Designer tab loads the workflow from the API and draws it: the intent, each stage in list order (name, agent and harness) and the end, joined by run-order edges. You can pick the agent of -a stage, rename it, add a stage and delete one. It shows a +a stage, rename it, add a stage, delete one and save the file. It shows a loading message, an error message or an empty message when there is nothing to draw. The Execution tab draws the saved workflow and runs it: a Run button starts a run, and each node shows its live state until the run ends. The @@ -236,10 +236,17 @@ modules, the graph core and the wire types. The rest wraps the core, so the rules can be tested without a browser: - `api/`: `WorkflowApi` is the port with the calls the page makes (open a - session, load the workflow, start a run, read a run); `createHttpApi` is its + session, load, save and check the workflow, start a run, read a run); + `createHttpApi` is its adapter over `fetch`. It ignores fields it does not know, refuses a reply that breaks the contract, and turns a failed call into an `ApiError` with a message fit to show. + `messageOf` (`api/message-of.ts`) is how every alert words a failure, and it + adds what to do next: for no answer, or a 5xx that is not the API's (the + answer of a proxy whose server is down), "If it stopped, start it again and + get a fresh launch address from the server."; for the API's 401 (the session + is gone, as after the server started again), "Get a fresh launch address from + the server." - `launch/launch-code.ts` and `workflow/` take the launch code from the address and load the workflow. `loadWorkflow` never rejects: it returns `{ ok: true, workflow }` or `{ ok: false, message }`. `createWorkflowSource` @@ -292,11 +299,12 @@ The rest wraps the core, so the rules can be tested without a browser: ## The Designer tab -The tab has the graph on the left, with **Add stage** above it, and -**Properties** on the right. Properties shows the selected stage (the first -one when the page opens): its `name` and its `agent`, and **Delete stage**. A -stage is only a `name` and an `agent`, and the stages run in the order they are -listed, so there is no other field and no way to link or reorder them yet. +The tab has the graph on the left, with **Add stage** above it and the save +line and **Save** in its header, and **Properties** on the right. Properties +shows the selected stage (the first one when the page opens): its `name` and +its `agent`, and **Delete stage**. A stage is only a `name` and an `agent`, and +the stages run in the order they are listed, so there is no other field and no +way to link or reorder them yet. - **Add stage** adds a stage at the end, named `agent-1` (the first free `agent-N`) with the agent `k-developer`, selects it and moves focus to its @@ -319,37 +327,94 @@ listed, so there is no other field and no way to link or reorder them yet. moves focus to its name; a click on it changes nothing. Escape on a node leaves focus on it. A polite live region says what was added, deleted or selected. + +Nothing is written until **Save**. The line beside it says **No changes**, +**Unsaved changes**, **Saving** or **Saved** in a polite live region. Save +sends the whole text with `PUT /api/workflow` and the `hash` of the file as it +was loaded (`base_hash`), and takes the hash of the answer for the next save. +Only the block of stages changes: the `name`, `intent`, `version` and the +comments outside the block stay as they were. A stage that is unchanged keeps +its own lines and the comments in front of it; the comments written inside a +stage that is changed or deleted go with it. If the stages are not a block at the top level of the file (for +example `stages: [...]`), the whole file is written from the view, in the plain +layout, which drops its comments and `version`; an info note above the graph +says so before you save. + +- **Checking.** A moment (400 ms) after the last change, the text is checked + with `POST /api/validate`. What it finds shows under the field it is about, + and all of it in an alert above the graph, with the name of the stage in + front. What the last check found stays on show while you type, until the + check of the new text answers, so the page does not jump at every key. The + server has the last word: Save is off while the check of the current draft + has errors, with the reason in a tooltip, and a save that goes out in the + pause is checked again by the server. A check that cannot reach the server + says nothing; the save checks again and reports it. Only an error stops a + save: a warning or a note is shown as a note (a warning or info alert, and + the text under the field in the warning style) and Save stays on. +- **A refused save** (422) shows the findings the same way. A refusal with no + error among its findings shows the sentence of the server as a failure, so + it is never silent. +- **A conflict** (409) means the file changed after the page loaded. Nothing + is written, and an alert says so. If the file already holds the text that was + sent (the 409 names its hash, and the page reads the file), the earlier write + landed and the save counts as done. Otherwise the alert offers **Copy my + version**, which puts the draft text on the clipboard (or shows it in a box, + if the page may not), and **Load the file again**, which loads the file from + the server and replaces the draft. The stages cannot be changed while the file + loads. After the load, a note keeps **Copy my version** for the text that was + replaced. It stays while the person goes on to edit, until the file is loaded + again or a save works, so the draft is never lost without a way back to it. - **A read-only workflow** (the server says `read_only`) can be looked at, and a stage can be selected, but the name is read-only and Add stage, Delete - stage and the agent list are off. A note above the graph says why, and the - buttons carry the reason in a tooltip. + stage, the agent list and Save are off. A note above the graph says why, and + the buttons carry the reason in a tooltip. +- **A save whose answer is lost** (no answer, a server error, or a 200 that the + page cannot read) may still have written the file. The page reads the file + again: if it holds the text that was sent, the save counts as done. If it + does not, the failure is shown in an alert, and Save can be tried again. If + the file cannot be read either, the alert says "The save did not finish" and + that it is not known whether the file was written. +- **Any other failure** of a save shows its sentence in an alert, and Save can + be tried again. When the session is gone (the API's 401, or a 401 from a + proxy), the sentence adds that the changes on the page are lost when the + launch address is opened again. A check that meets a lost session says so at + once, under "The session is lost", as no save can work then. - **Focus and keys.** Everything above works with the keyboard: Tab to **Add stage** and press Enter; the new stage's name has focus. A button that is off - (Add stage, Delete stage) stays focusable and gives the reason in a tooltip, - so focus stays on the button that was pressed. + (Save, Add stage, Delete stage) stays focusable and gives the reason in a + tooltip, so focus stays on the button that was pressed, for instance Save + while it saves. - **The two tabs.** The draft lives in `App`, so it is kept when you switch tabs. The Execution tab draws the saved workflow, because a run uses the saved file, and says so when the Designer holds changes that are not saved. -The page does not write the file in this version: the draft is only kept in the -page. So while the draft differs from the file, closing or reloading the page +While the draft differs from the saved file, closing or reloading the page makes the browser ask whether to leave (`useLeaveGuard`, a `beforeunload` -handler that is on only then). A change that is put back, so the draft equals -the file again, does not ask. +handler that is on only then), because the draft lives in the page. A change +that is put back, so the draft equals the file again, does not ask, and nor +does a draft that has been saved. The code follows the split of the Execution tab: -- `src/designer/core/` is the core and does no IO: the draft of the stages - (`draft.ts`), the text a save would write (`stage-text.ts`), the findings put - on the stage and field they are about (`problems.ts`), and the state of the - editor with its reducer. `test-support/designer-purity.test.ts` fails if the - core imports anything but its own modules and the wire types, or reads a +- `src/designer/core/` is the core and does no IO: the draft of the stages, the + text that a save writes (`stage-text.ts`), the findings put on the stage and + field they are about (`problems.ts`), the state of the editor and its + reducer, and the two loops that check (after a pause) and save. Its ports + are `EditPort` (save, check and reload) and the `Scheduler` of the run core, + so a test moves time by hand and nothing sleeps. + `test-support/designer-purity.test.ts` fails if the core imports anything + but its own modules, the wire types, `messageOf` and the scheduler port, or reads a clock, randomness, the environment or the network. -- `useEditor` keeps the state in `App`; `DesignerEditor` and `PropertiesPane` - draw it. `useLeaveGuard` asks the browser to confirm leaving while the draft - has changes. A stage keeps an id of its own while it is renamed, and the node is - named from that id, so two stages with one name for a moment still have two - nodes. +- `createHttpApi` has `saveWorkflow` and `checkWorkflow`; `loadWorkflow` ties + them to the session token as the `EditPort`, and turns a 409 `conflict` and + a 422 `invalid_workflow` (with its `diagnostics`) into the core's own errors. +- `useEditor` keeps the state in `App`; `DesignerEditor`, `PropertiesPane`, + `SaveControls` and `EditAlerts` draw it. `useLeaveGuard` asks the browser to + confirm leaving while the draft has changes. A stage keeps an id of its own + while it is renamed, and the node is named from that id, so two stages with + one name for a moment still have two nodes. +- The API contract is unchanged. `test-support/editing-contract.test.ts` + checks the limits and calls that the Designer assumes against it. ## The Execution tab @@ -413,8 +478,9 @@ intent, because the API needs one (1 to 2048 characters) to start a run. alert. The goal is one line: line breaks in the outcome of the intent become spaces, and Enter that ends an input method composition starts no run. - A workflow can be saved while a run goes on, so the run may have other stages - than the graph. The graph is read once, when the page loads, while the run - works from the saved file. Node states attach by stage name, so a warning + than the graph. The graph is the saved workflow, and it is drawn again when + the Designer saves, while a run works from the file as it was saved when the + run started. Node states attach by stage name, so a warning above the graph names the stages that are not drawn and those that are not in the run. - A run goes on while the Designer tab is open, and the tab shows it again when @@ -547,8 +613,11 @@ the theme button is pressed). The Execution tab is saved before a run (`execution-tab-running.png`) and after the run passed (`execution-tab-finished.png`), all in the light theme, and while the first stage runs in the dark theme (`execution-tab-dark.png`). The Designer tab is -also saved with edits in the draft (`designer-editing.png`, and -`designer-editing-dark.png` in the dark theme). Pull request +also saved with unsaved edits (`designer-editing.png`, and +`designer-editing-dark.png` in the dark theme), after a save +(`designer-saved.png`), with a problem the server found +(`designer-problems.png`) and when the file changed on disk +(`designer-conflict.png`). Pull request descriptions embed these files, so regenerate and commit them when the UI changes. diff --git a/workflow-canvas/e2e/capture/screenshots.spec.ts b/workflow-canvas/e2e/capture/screenshots.spec.ts index 24f3bf69..314d6fcc 100644 --- a/workflow-canvas/e2e/capture/screenshots.spec.ts +++ b/workflow-canvas/e2e/capture/screenshots.spec.ts @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 import { expect, test } from '../support/editing-fixtures'; import { answerRunsWith, runReply } from '../support/run-routes'; +import { overwriteWorkflow, SAMPLE_TEXT } from '../support/stub-workflow'; import { saveFullPageScreenshot } from '../support/screenshots'; import type { WorkflowCanvasPage } from '../support/workflow-canvas-page'; @@ -88,7 +89,8 @@ test('capture the Execution tab in the dark mode during a run', async ({ await saveFullPageScreenshot(canvas.page, 'execution-tab-dark'); }); -// Editing. The stub's workflow is not touched: these only change the draft. +// Editing. Each test puts the stub's workflow back afterwards (the fixture +// does), so the captures above and below it see the sample. // These two change what a stage says but add none: a longer graph is fitted at // a fractional zoom, and the edges of such a view differ by a level of grey // between machines, which the byte-exact check cannot take. @@ -96,11 +98,43 @@ test('capture the Designer tab with unsaved edits', async ({ designer }) => { await designer.chooseAgent('k-architect'); await designer.stage('review').click(); await designer.rename('ship'); - await designer.stage('ship').waitFor(); + await designer.expectStatus('Unsaved changes'); await saveFullPageScreenshot(designer.page, 'designer-editing'); }); +test('capture the Designer tab after a save', async ({ designer }) => { + await designer.rename('write'); + await designer.save.click(); + await designer.expectStatus('Saved'); + await saveFullPageScreenshot(designer.page, 'designer-saved'); +}); + +test('capture the Designer tab with a problem the server found', async ({ + designer, +}) => { + await designer.stage('review').click(); + await designer.rename('draft'); + await expect(designer.problems).toBeVisible(); + + await saveFullPageScreenshot(designer.page, 'designer-problems'); +}); + +test('capture the Designer tab when the file changed on disk', async ({ + designer, + request, +}) => { + await designer.rename('write'); + await overwriteWorkflow( + request, + SAMPLE_TEXT.replace('k-quality-assurance', 'k-tpm'), + ); + await designer.save.click(); + await expect(designer.conflict).toBeVisible(); + + await saveFullPageScreenshot(designer.page, 'designer-conflict'); +}); + test('capture the Designer tab with unsaved edits in the dark mode', async ({ canvas, designer, @@ -109,7 +143,7 @@ test('capture the Designer tab with unsaved edits in the dark mode', async ({ await designer.chooseAgent('k-architect'); await designer.stage('review').click(); await designer.rename('ship'); - await designer.stage('ship').waitFor(); + await designer.expectStatus('Unsaved changes'); await saveFullPageScreenshot(designer.page, 'designer-editing-dark'); }); diff --git a/workflow-canvas/e2e/screenshots/designer-conflict.png b/workflow-canvas/e2e/screenshots/designer-conflict.png new file mode 100644 index 00000000..c279f092 Binary files /dev/null and b/workflow-canvas/e2e/screenshots/designer-conflict.png differ diff --git a/workflow-canvas/e2e/screenshots/designer-editing-dark.png b/workflow-canvas/e2e/screenshots/designer-editing-dark.png index d200628d..a56409aa 100644 Binary files a/workflow-canvas/e2e/screenshots/designer-editing-dark.png and b/workflow-canvas/e2e/screenshots/designer-editing-dark.png differ diff --git a/workflow-canvas/e2e/screenshots/designer-editing.png b/workflow-canvas/e2e/screenshots/designer-editing.png index 7d3abca0..ad807c39 100644 Binary files a/workflow-canvas/e2e/screenshots/designer-editing.png and b/workflow-canvas/e2e/screenshots/designer-editing.png differ diff --git a/workflow-canvas/e2e/screenshots/designer-problems.png b/workflow-canvas/e2e/screenshots/designer-problems.png new file mode 100644 index 00000000..ba77c0fa Binary files /dev/null and b/workflow-canvas/e2e/screenshots/designer-problems.png differ diff --git a/workflow-canvas/e2e/screenshots/designer-saved.png b/workflow-canvas/e2e/screenshots/designer-saved.png new file mode 100644 index 00000000..8ed342d9 Binary files /dev/null and b/workflow-canvas/e2e/screenshots/designer-saved.png differ diff --git a/workflow-canvas/e2e/screenshots/designer-tab-dark.png b/workflow-canvas/e2e/screenshots/designer-tab-dark.png index 812bb83e..2970250e 100644 Binary files a/workflow-canvas/e2e/screenshots/designer-tab-dark.png and b/workflow-canvas/e2e/screenshots/designer-tab-dark.png differ diff --git a/workflow-canvas/e2e/screenshots/designer-tab.png b/workflow-canvas/e2e/screenshots/designer-tab.png index c8acc3da..b98359db 100644 Binary files a/workflow-canvas/e2e/screenshots/designer-tab.png and b/workflow-canvas/e2e/screenshots/designer-tab.png differ diff --git a/workflow-canvas/e2e/screenshots/execution-tab-finished.png b/workflow-canvas/e2e/screenshots/execution-tab-finished.png index 54fe55db..d7464313 100644 Binary files a/workflow-canvas/e2e/screenshots/execution-tab-finished.png and b/workflow-canvas/e2e/screenshots/execution-tab-finished.png differ diff --git a/workflow-canvas/e2e/support/designer-page.ts b/workflow-canvas/e2e/support/designer-page.ts index e01d9847..db405e83 100644 --- a/workflow-canvas/e2e/support/designer-page.ts +++ b/workflow-canvas/e2e/support/designer-page.ts @@ -1,5 +1,13 @@ // SPDX-License-Identifier: Apache-2.0 -import type { Locator, Page } from '@playwright/test'; +import { expect, type Locator, type Page } from '@playwright/test'; + +/** What the save line says. */ +export type SaveStatus = + | 'No changes' + | 'Unsaved changes' + | 'Saving' + | 'Saved' + | 'Loaded again from the server'; /** Page object for the editing parts of the Designer tab. */ export class DesignerPage { @@ -7,6 +15,10 @@ export class DesignerPage { readonly agent: Locator; readonly add: Locator; readonly remove: Locator; + readonly save: Locator; + readonly saveStatus: Locator; + readonly conflict: Locator; + readonly problems: Locator; readonly selected: Locator; constructor(readonly page: Page) { @@ -14,6 +26,10 @@ export class DesignerPage { this.agent = page.getByRole('button', { name: /^agent / }); this.add = page.getByRole('button', { name: 'Add stage' }); this.remove = page.getByRole('button', { name: 'Delete stage' }); + this.save = page.getByRole('button', { name: 'Save', exact: true }); + this.saveStatus = page.getByTestId('save-status'); + this.conflict = page.getByTestId('conflict'); + this.problems = page.getByTestId('problems'); this.selected = page.getByText('Selected:'); } @@ -34,4 +50,14 @@ export class DesignerPage { .getByRole('option', { name: new RegExp(`^${id}\\b`) }) .click(); } + + async expectStatus(status: SaveStatus): Promise { + await expect(this.saveStatus).toHaveText(status); + } + + /** Asserts the save button can be pressed, or cannot. */ + async expectSaveEnabled(enabled: boolean): Promise { + if (enabled) await expect(this.save).toBeEnabled(); + else await expect(this.save).toBeDisabled(); + } } diff --git a/workflow-canvas/e2e/support/editing-fixtures.ts b/workflow-canvas/e2e/support/editing-fixtures.ts index 053b34b4..7dee130b 100644 --- a/workflow-canvas/e2e/support/editing-fixtures.ts +++ b/workflow-canvas/e2e/support/editing-fixtures.ts @@ -1,11 +1,24 @@ // SPDX-License-Identifier: Apache-2.0 import { DesignerPage } from './designer-page'; import { test as base } from './fixtures'; +import { restoreSample } from './stub-workflow'; export { expect } from './fixtures'; -/** `test` for specs that edit on the Designer tab. */ -export const test = base.extend<{ designer: DesignerPage }>({ +/** + * `test` for specs that save to the stub. The stub keeps what is saved until + * it stops, so an automatic fixture puts the sample back before each test + * opens the page, and after it. + */ +export const test = base.extend<{ sample: void; designer: DesignerPage }>({ + sample: [ + async ({ request }, use) => { + await restoreSample(request); + await use(); + await restoreSample(request); + }, + { auto: true }, + ], designer: async ({ canvas }, use) => { await canvas.expectWorkflowDrawn(); await use(new DesignerPage(canvas.page)); diff --git a/workflow-canvas/e2e/support/screenshots.ts b/workflow-canvas/e2e/support/screenshots.ts index bcc0e2c2..74a87092 100644 --- a/workflow-canvas/e2e/support/screenshots.ts +++ b/workflow-canvas/e2e/support/screenshots.ts @@ -29,13 +29,17 @@ async function waitForFonts(page: Page): Promise { /** * Saves a full-page PNG as `e2e/screenshots/.png`, rendered with the - * bundled fonts, with animations and the caret frozen so repeated runs match. + * bundled fonts, with animations and the caret frozen and the pointer moved off + * the page, so repeated runs match. */ export async function saveFullPageScreenshot( page: Page, name: string, ): Promise { await waitForFonts(page); + // A button that was just clicked and is now off shows its reason in a tooltip + // while the pointer is over it, which comes up a moment later or not. + await page.mouse.move(0, 0); await page.screenshot({ path: path.join(SCREENSHOT_DIR, `${name}.png`), fullPage: true, diff --git a/workflow-canvas/e2e/support/stub-workflow.ts b/workflow-canvas/e2e/support/stub-workflow.ts new file mode 100644 index 00000000..1162587c --- /dev/null +++ b/workflow-canvas/e2e/support/stub-workflow.ts @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: Apache-2.0 +import type { APIRequestContext } from '@playwright/test'; +import { SAMPLE_FILES } from '../../stub-api/core/sample.ts'; +import { API_URL } from '../../playwright.shared'; +import { sharedSession } from './session'; + +/** The text the stub starts with. */ +export const SAMPLE_TEXT = SAMPLE_FILES.hello ?? ''; + +function headers(extra: Record = {}) { + return { + Authorization: `Bearer ${sharedSession().token}`, + Origin: API_URL, + ...extra, + }; +} + +/** The workflow as the API holds it now. */ +export async function currentWorkflow( + request: APIRequestContext, +): Promise<{ text: string; hash: string }> { + const reply = await request.get(`${API_URL}/api/workflow`, { + headers: headers(), + }); + if (!reply.ok()) { + throw new Error(`The stub did not give the workflow (${reply.status()}).`); + } + return (await reply.json()) as { text: string; hash: string }; +} + +/** + * Writes `text` over the workflow as another program would, in the middle of + * a test, so a save from the page finds the file changed. + */ +export async function overwriteWorkflow( + request: APIRequestContext, + text: string, +): Promise { + const { hash } = await currentWorkflow(request); + const reply = await request.put(`${API_URL}/api/workflow`, { + headers: headers({ 'Content-Type': 'application/json' }), + data: { text, base_hash: hash }, + }); + if (!reply.ok()) { + throw new Error( + `The stub refused to write the workflow (${reply.status()}).`, + ); + } +} + +/** + * Puts the stub's workflow back as it started. The editing tests save for real + * and the stub keeps what is saved until it stops, so each one starts and ends + * from the sample. + */ +export async function restoreSample(request: APIRequestContext): Promise { + const { text } = await currentWorkflow(request); + if (text !== SAMPLE_TEXT) await overwriteWorkflow(request, SAMPLE_TEXT); +} diff --git a/workflow-canvas/e2e/tests/designer.spec.ts b/workflow-canvas/e2e/tests/designer.spec.ts index 0d9e915d..02112e8f 100644 --- a/workflow-canvas/e2e/tests/designer.spec.ts +++ b/workflow-canvas/e2e/tests/designer.spec.ts @@ -1,4 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 +import type { Locator } from '@playwright/test'; import { STUB_LAUNCH_CODE } from '../../playwright.shared'; import { test as editing, expect } from '../support/editing-fixtures'; import { test as base } from '../support/fixtures'; @@ -57,6 +58,25 @@ editing( }, ); +// A problem appears while the person types. It must not push the field they +// type in, or the graph, down the page. Layout is not drawn in jsdom, so this +// needs a browser. +editing( + 'keeps the name field and the graph where they are when a problem appears', + async ({ designer }) => { + const graph = designer.page.getByRole('region', { name: 'Workflow graph' }); + const top = async () => (await designer.name.boundingBox())?.y; + const before = { name: await top(), graph: await graph.boundingBox() }; + + await designer.rename('Bad'); + await expect(designer.page.getByTestId('problems')).toBeVisible(); + + expect({ name: await top(), graph: await graph.boundingBox() }).toEqual( + before, + ); + }, +); + // The Designer is a grid of the graph and Properties; a note above them takes // the whole row, so the graph keeps the wide column. Layout is not drawn in // jsdom, so this needs a browser. @@ -75,23 +95,28 @@ base( await canvas.open(STUB_LAUNCH_CODE); await canvas.expectWorkflowDrawn(); - const note = await page.getByTestId('read-only-note').boundingBox(); - const [, , card, properties] = await page - .locator('.wf-designer > *') - .evaluateAll((items) => - items.map((item) => { - const { x, y, width, height } = item.getBoundingClientRect(); - return { x, y, width, height }; - }), - ); - - expect(note).not.toBeNull(); - expect(card!.width).toBeGreaterThan(properties!.width); - expect(card!.x).toBeLessThan(properties!.x); - expect(card!.y).toBeGreaterThanOrEqual(note!.y + note!.height); - expect(Math.abs(card!.y - properties!.y)).toBeLessThan(1); - expect(note!.x + note!.width).toBeGreaterThanOrEqual( - properties!.x + properties!.width - 1, + const boxOf = (locator: Locator) => + locator.evaluate((element) => { + const { x, y, width, height } = element.getBoundingClientRect(); + return { x, y, width, height }; + }); + const items = page.locator('.wf-designer > *'); + const note = await boxOf(page.getByTestId('read-only-note')); + const card = await boxOf( + items.filter({ + has: page.getByRole('region', { name: 'Workflow graph' }), + }), + ); + const properties = await boxOf( + items.filter({ has: page.getByRole('heading', { name: 'Properties' }) }), + ); + + expect(card.width).toBeGreaterThan(properties.width); + expect(card.x).toBeLessThan(properties.x); + expect(card.y).toBeGreaterThanOrEqual(note.y + note.height); + expect(Math.abs(card.y - properties.y)).toBeLessThan(1); + expect(note.x + note.width).toBeGreaterThanOrEqual( + properties.x + properties.width - 1, ); }, ); diff --git a/workflow-canvas/e2e/tests/server-gone.spec.ts b/workflow-canvas/e2e/tests/server-gone.spec.ts new file mode 100644 index 00000000..2bbb19d3 --- /dev/null +++ b/workflow-canvas/e2e/tests/server-gone.spec.ts @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 +import { test as bare } from '@playwright/test'; +import { expect, test } from '../support/editing-fixtures'; +import { WorkflowCanvasPage } from '../support/workflow-canvas-page'; + +// What the page says when the server is gone or has forgotten the session. The +// stub is shared by every test, so it is not stopped here: the calls are +// answered the way the dev server answers when the stub is down (a 502 with a +// plain-text body that is not the API's) and the way the stub answers after it +// started again (a 401 `unauthorized`). Each message must say what to do next. + +const NEXT_AFTER_STOP = + 'If it stopped, start it again and get a fresh launch address from the server.'; +const NEXT_AFTER_RESTART = 'Get a fresh launch address from the server.'; + +const badGateway = { + status: 502, + contentType: 'text/plain', + body: 'Bad Gateway', +}; +const sessionGone = { + status: 401, + json: { + error: 'The session token is missing or wrong.', + code: 'unauthorized', + }, +}; + +bare( + 'says what to do when the server is down while the page loads', + async ({ page }) => { + await page.route('**/api/session', (route) => route.fulfill(badGateway)); + + await new WorkflowCanvasPage(page).open('any-launch-code'); + + await expect(page.getByText('The workflow did not load')).toBeVisible(); + await expect(page.getByText('The server answered 502.')).toBeVisible(); + await expect(page.getByText(NEXT_AFTER_STOP)).toBeVisible(); + }, +); + +test('says what to do when the server is down as a run starts', async ({ + canvas, +}) => { + await canvas.page.route('**/api/run', (route) => route.fulfill(badGateway)); + await canvas.selectTab('Execution'); + + await canvas.runButton.click(); + + await expect(canvas.runError).toContainText('The server answered 502.'); + await expect(canvas.runError).toContainText(NEXT_AFTER_STOP); +}); + +test('says what to do when the server forgot the session as a run starts', async ({ + canvas, +}) => { + await canvas.page.route('**/api/run', (route) => route.fulfill(sessionGone)); + await canvas.selectTab('Execution'); + + await canvas.runButton.click(); + + await expect(canvas.runError).toContainText( + 'The server no longer knows this session.', + ); + await expect(canvas.runError).toContainText(NEXT_AFTER_RESTART); + expect(await canvas.page.evaluate(() => window.sessionStorage.length)).toBe( + 0, + ); +}); + +test('says what to do when the server is down as the changes are saved', async ({ + designer, +}) => { + const { page } = designer; + await page.route('**/api/workflow', (route) => + route.request().method() === 'PUT' + ? route.fulfill(badGateway) + : route.continue(), + ); + await designer.stage('draft').click(); + await designer.rename('drafting'); + const save = page.getByRole('button', { name: 'Save', exact: true }); + await expect(save).toBeEnabled(); + + await save.click(); + + const alert = page.getByText('The workflow was not saved').locator('..'); + await expect(alert).toContainText('The server answered 502.'); + await expect(alert).toContainText(NEXT_AFTER_STOP); + await expect(designer.name).toHaveValue('drafting'); +}); diff --git a/workflow-canvas/src/App.tsx b/workflow-canvas/src/App.tsx index 7b7522bf..81d63818 100644 --- a/workflow-canvas/src/App.tsx +++ b/workflow-canvas/src/App.tsx @@ -3,7 +3,7 @@ import AppLayout from '@cloudscape-design/components/app-layout'; import ContentLayout from '@cloudscape-design/components/content-layout'; import Header from '@cloudscape-design/components/header'; import Tabs from '@cloudscape-design/components/tabs'; -import { useEffect, type ReactNode } from 'react'; +import { useEffect, useMemo, type ReactNode } from 'react'; import { Breadcrumbs } from './chrome/Breadcrumbs'; import './chrome/chrome.css'; import { TOP_BAR_ID, TopBar } from './chrome/TopBar'; @@ -21,7 +21,11 @@ import { useGoal } from './execution/use-goal'; import type { SwapRisk } from './launch/swap-guard'; import { TAB_STRIP_STYLE } from './theme/tokens'; import { useThemeMode } from './theme/use-theme-mode'; -import { openWorkflowName, type LoadResult } from './workflow/load-workflow'; +import { + openWorkflowName, + withDocument, + type LoadResult, +} from './workflow/load-workflow'; /** * Root component: the top bar, then the page header (breadcrumbs and title) @@ -58,10 +62,17 @@ export function App({ const [mode, toggleMode] = useThemeMode(); // The Designer edits a draft; the run and the Execution tab use the file // as it is saved. - const editor = useEditor(result?.ok ? result.workflow : undefined); + const editor = useEditor(result?.ok ? result.workflow : undefined, scheduler); const unsaved = editor ? isDirty(editor.state) : false; // The draft lives in the page, so leaving the page loses the changes. useLeaveGuard(unsaved, window); + // One object until the saved file changes, as the tab draws a new canvas + // for each new result. + const document = editor?.state.saved; + const saved = useMemo( + () => (document ? withDocument(result, document) : result), + [result, document], + ); const running = isInFlight(execution.state); useEffect( () => swapRisk?.set({ draft: unsaved, run: running }), @@ -80,9 +91,7 @@ export function App({ - } + breadcrumbs={} header={
Workflow canvas
} > {notice} @@ -102,7 +111,7 @@ export function App({ label: 'Execution', content: ( ; /** Loads the root workflow of the session. */ getWorkflow(token: string): Promise; + /** + * Writes the text of the root workflow if the file still has `baseHash`. + * A file that changed fails with code `conflict`; a text with errors fails + * with code `invalid_workflow` and its `diagnostics`. + */ + saveWorkflow( + token: string, + text: string, + baseHash: string | null, + ): Promise; + /** Checks workflow text without writing it. */ + checkWorkflow(token: string, text: string): Promise; /** * Starts a run of the root workflow with the goal; resolves to its id. * Aborting the signal ends the call. @@ -27,13 +46,26 @@ export class ApiError extends Error { readonly code?: string; /** The `run_id` of the error body, when there was one. */ readonly runId?: string; + /** The `diagnostics` of the error body; empty when there were none. */ + readonly diagnostics: readonly DiagnosticView[]; + /** The `current_hash` of a 409 body: the hash of the file as it is now. */ + readonly currentHash?: string; - constructor(message: string, status: number, code?: string, runId?: string) { + constructor( + message: string, + status: number, + code?: string, + runId?: string, + diagnostics: readonly DiagnosticView[] = [], + currentHash?: string, + ) { super(message); this.name = 'ApiError'; this.status = status; this.code = code; this.runId = runId; + this.diagnostics = diagnostics; + this.currentHash = currentHash; } } diff --git a/workflow-canvas/src/api/http-api.test.ts b/workflow-canvas/src/api/http-api.test.ts index 19a1325f..410f313a 100644 --- a/workflow-canvas/src/api/http-api.test.ts +++ b/workflow-canvas/src/api/http-api.test.ts @@ -24,6 +24,29 @@ const DOCUMENT_BODY = { diagnostics: [], }; +const SAVE_BODY = { + hash: 'c'.repeat(64), + view: DOCUMENT_BODY.view, + diagnostics: [], +}; + +const CHECK_BODY = { + errors: 1, + warnings: 0, + infos: 0, + view: null, + diagnostics: [ + { + code: 'DUPLICATE_STAGE', + severity: 'error', + path: 'stages[1].name', + line: 6, + message: 'The stage name `write` is already used.', + }, + ], + summary: { valid: false, counts: { stages: 0 } }, +}; + const RUN_STARTED_BODY = { run_id: '20261024-0412-a7b3c' }; const RUN_BODY = { @@ -365,6 +388,182 @@ describe('createHttpApi', () => { expect((error as ApiError).runId).toBeUndefined(); }); + it('puts the text and base hash as JSON to /api/workflow with the token', async () => { + const { api, fetchFn } = apiFor(reply(SAVE_BODY)); + + await api.saveWorkflow('t-1', 'name: hello\n', 'b'.repeat(64)); + + const [path, init] = fetchFn.mock.calls[0] as unknown as [ + string, + RequestInit, + ]; + expect(path).toBe('/api/workflow'); + expect(init.method).toBe('PUT'); + expect(init.headers).toEqual({ + Authorization: 'Bearer t-1', + 'Content-Type': 'application/json', + }); + expect(JSON.parse(init.body as string)).toEqual({ + text: 'name: hello\n', + base_hash: 'b'.repeat(64), + }); + }); + + it('reads the hash and view of a saved workflow', async () => { + const { api } = apiFor(reply({ ...SAVE_BODY, future_field: true })); + + expect(await api.saveWorkflow('t', 'x', 'b'.repeat(64))).toEqual({ + hash: 'c'.repeat(64), + view: DOCUMENT_BODY.view, + diagnostics: [], + }); + }); + + it('fails when a save is answered without a view', async () => { + const { api } = apiFor(reply({ hash: 'c'.repeat(64), diagnostics: [] })); + + await expect(api.saveWorkflow('t', 'x', null)).rejects.toBeInstanceOf( + ApiError, + ); + }); + + it('fails a conflicting save with the code conflict', async () => { + const { api } = apiFor( + reply({ error: 'The file changed on disk.', code: 'conflict' }, 409), + ); + + await expect( + api.saveWorkflow('t', 'x', 'b'.repeat(64)), + ).rejects.toMatchObject({ + status: 409, + code: 'conflict', + message: 'The file changed on disk.', + }); + }); + + it('carries the hash of the file in a conflict', async () => { + const { api } = apiFor( + reply( + { + error: 'The file changed on disk.', + code: 'conflict', + current_hash: 'c'.repeat(64), + }, + 409, + ), + ); + + const error = await api + .saveWorkflow('t', 'x', 'b'.repeat(64)) + .catch((caught: unknown) => caught); + + expect((error as ApiError).currentHash).toBe('c'.repeat(64)); + }); + + it('carries no hash in a conflict about a file that is gone', async () => { + const { api } = apiFor( + reply({ error: 'The file is gone.', code: 'conflict' }, 409), + ); + + const error = await api + .saveWorkflow('t', 'x', 'b'.repeat(64)) + .catch((caught: unknown) => caught); + + expect((error as ApiError).currentHash).toBeUndefined(); + }); + + it('carries the findings of a refused save', async () => { + const { api } = apiFor( + reply( + { + error: 'The workflow has errors.', + code: 'invalid_workflow', + diagnostics: [ + { + code: 'DUPLICATE_STAGE', + severity: 'error', + path: 'stages[1].name', + line: 6, + message: 'The stage name `write` is already used.', + }, + ], + }, + 422, + ), + ); + + const error = await api + .saveWorkflow('t', 'x', 'b'.repeat(64)) + .catch((caught: unknown) => caught); + + expect(error).toMatchObject({ status: 422, code: 'invalid_workflow' }); + expect((error as ApiError).diagnostics).toEqual([ + { + message: 'The stage name `write` is already used.', + line: 6, + path: 'stages[1].name', + severity: 'error', + }, + ]); + }); + + it('carries no findings when the error body has none or cannot be read', async () => { + const { api } = apiFor( + reply({ error: 'Bad.', code: 'invalid_workflow', diagnostics: [7] }, 422), + ); + + const error = await api + .saveWorkflow('t', 'x', null) + .catch((caught: unknown) => caught); + + expect((error as ApiError).diagnostics).toEqual([]); + }); + + it('posts the text as JSON to /api/validate with the token', async () => { + const { api, fetchFn } = apiFor(reply(CHECK_BODY)); + + await api.checkWorkflow('t-1', 'name: hello\n'); + + const [path, init] = fetchFn.mock.calls[0] as unknown as [ + string, + RequestInit, + ]; + expect(path).toBe('/api/validate'); + expect(init.method).toBe('POST'); + expect(init.headers).toEqual({ + Authorization: 'Bearer t-1', + 'Content-Type': 'application/json', + }); + expect(JSON.parse(init.body as string)).toEqual({ text: 'name: hello\n' }); + }); + + it('reads the view and findings of a check', async () => { + const { api } = apiFor(reply(CHECK_BODY)); + + expect(await api.checkWorkflow('t', 'x')).toEqual({ + view: null, + diagnostics: [ + { + message: 'The stage name `write` is already used.', + line: 6, + path: 'stages[1].name', + severity: 'error', + }, + ], + }); + }); + + it('reads a check with no problems as a view', async () => { + const { api } = apiFor( + reply({ ...CHECK_BODY, view: DOCUMENT_BODY.view, diagnostics: [] }), + ); + + expect(await api.checkWorkflow('t', 'x')).toEqual({ + view: DOCUMENT_BODY.view, + diagnostics: [], + }); + }); + it('gets /api/runs/{id} with the bearer token', async () => { const { api, fetchFn } = apiFor(reply(RUN_BODY)); diff --git a/workflow-canvas/src/api/http-api.ts b/workflow-canvas/src/api/http-api.ts index 2b0a53f0..3a424a0f 100644 --- a/workflow-canvas/src/api/http-api.ts +++ b/workflow-canvas/src/api/http-api.ts @@ -2,10 +2,14 @@ import { ApiError, type WorkflowApi } from './api'; import { errorCodeOf, + errorCurrentHashOf, + errorDiagnosticsOf, errorMessageOf, errorRunIdOf, + parseCheckResult, parseRunStarted, parseRunStatus, + parseSaveResult, parseSession, parseWorkflowDocument, } from './parse'; @@ -89,6 +93,8 @@ async function send( response.status, errorCodeOf(body), errorRunIdOf(body), + errorDiagnosticsOf(body), + errorCurrentHashOf(body), ); } @@ -106,6 +112,13 @@ function bearer(token: string): Record { return { Authorization: `Bearer ${token}` }; } +function jsonHeaders(token?: string): Record { + return { + ...(token === undefined ? {} : bearer(token)), + 'Content-Type': 'application/json', + }; +} + /** * The adapter of the workflow API over HTTP. It calls the relative `/api`, so * the page and the server share one origin, and keeps no token itself. @@ -122,7 +135,7 @@ export function createHttpApi( // after the server has traded it would leave the page with no way in. const body = await send(fetchFn, undefined, '/api/session', { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: jsonHeaders(), body: JSON.stringify({ code }), }); return parseSession(body); @@ -133,6 +146,22 @@ export function createHttpApi( }); return parseWorkflowDocument(body); }, + async saveWorkflow(token, text, baseHash) { + const body = await call('/api/workflow', { + method: 'PUT', + headers: jsonHeaders(token), + body: JSON.stringify({ text, base_hash: baseHash }), + }); + return parseSaveResult(body); + }, + async checkWorkflow(token, text) { + const body = await call('/api/validate', { + method: 'POST', + headers: jsonHeaders(token), + body: JSON.stringify({ text }), + }); + return parseCheckResult(body); + }, async startRun(token, goal, signal) { const body = await call( '/api/run', diff --git a/workflow-canvas/src/api/message-of.test.ts b/workflow-canvas/src/api/message-of.test.ts new file mode 100644 index 00000000..e2fb619d --- /dev/null +++ b/workflow-canvas/src/api/message-of.test.ts @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: Apache-2.0 +import { describe, expect, it } from 'vitest'; +import { ApiError } from './api'; +import { isSessionLost, lostSessionMessage, messageOf } from './message-of'; + +describe('messageOf', () => { + it('gives the message of an Error', () => { + expect(messageOf(new Error('Down.'), 'Fallback.')).toBe('Down.'); + }); + + it('gives the fallback for a value that is not an Error', () => { + expect(messageOf('boom', 'Fallback.')).toBe('Fallback.'); + }); + + it.each([ + ['no answer', new ApiError('The server did not answer.', 0)], + ['a 502 from a proxy', new ApiError('The server answered 502.', 502)], + ['a 504 from a proxy', new ApiError('The server answered 504.', 504)], + ])('tells what to do when the server may have stopped: %s', (_, error) => { + expect(messageOf(error, 'Fallback.')).toBe( + `${error.message} If it stopped, start it again and get a fresh launch address from the server.`, + ); + }); + + it.each(['unauthorized', 'invalid_code'])( + 'tells to get a fresh launch address for the code %s', + (code) => { + const error = new ApiError('The session is gone.', 401, code); + + expect(messageOf(error, 'Fallback.')).toBe( + 'The session is gone. Get a fresh launch address from the server.', + ); + }, + ); + + it('takes a 401 with no body of the API for a lost session', () => { + const error = new ApiError('The server answered 401.', 401); + + expect(isSessionLost(error)).toBe(true); + expect(lostSessionMessage(error)).toBe( + 'The server answered 401. Get a fresh launch address from the server.', + ); + }); + + it.each([ + ['its own 401', new ApiError('Gone.', 401, 'unauthorized')], + ['a bad launch code', new ApiError('Bad.', 400, 'invalid_code')], + ])('takes %s for a lost session', (_, error) => { + expect(isSessionLost(error)).toBe(true); + }); + + it.each([403, 404, 500])( + 'does not take a %i for a lost session', + (status) => { + expect(isSessionLost(new ApiError('No.', status))).toBe(false); + }, + ); + + it.each([ + ['a refusal of the API', new ApiError('Not allowed.', 403, 'forbidden')], + ['a failure of its storage', new ApiError('Disk.', 500, 'io_error')], + ['a 404 with no code', new ApiError('Gone.', 404)], + ])('adds nothing to %s', (_, error) => { + expect(messageOf(error, 'Fallback.')).toBe(error.message); + }); +}); diff --git a/workflow-canvas/src/api/message-of.ts b/workflow-canvas/src/api/message-of.ts new file mode 100644 index 00000000..92286b0e --- /dev/null +++ b/workflow-canvas/src/api/message-of.ts @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: Apache-2.0 +import { ApiError, canRetryStatus } from './api'; + +const IF_STOPPED = + 'If it stopped, start it again and get a fresh launch address from the server.'; +const NEW_SESSION = 'Get a fresh launch address from the server.'; + +/** Whether the server no longer knows the session: its own 401, or a proxy's. */ +export function isSessionLost(error: ApiError): boolean { + return ( + error.status === 401 || + error.code === 'unauthorized' || + error.code === 'invalid_code' + ); +} + +// What a person can do about a failure that they can fix, or `undefined`. +// No answer, or a server error with no body of the API (the answer of a proxy +// whose server is down), means the server may have stopped. The API's own +// 401 means the session is gone, as after the server started again. +function nextStepOf(error: ApiError): string | undefined { + if (error.code === 'unauthorized' || error.code === 'invalid_code') { + return NEW_SESSION; + } + return error.code === undefined && canRetryStatus(error.status) + ? IF_STOPPED + : undefined; +} + +/** The message of a lost session, with the way to get a new one. */ +export function lostSessionMessage(error: ApiError): string { + return error.message.includes(NEW_SESSION) + ? error.message + : `${error.message} ${NEW_SESSION}`; +} + +/** + * The message to show for a failure: that of the `Error`, with what to do + * next when the failure is the server not answering or the session being + * gone. `fallback` is for anything that is not an `Error`. + */ +export function messageOf(error: unknown, fallback: string): string { + if (!(error instanceof Error)) return fallback; + const next = error instanceof ApiError ? nextStepOf(error) : undefined; + return next === undefined ? error.message : `${error.message} ${next}`; +} diff --git a/workflow-canvas/src/api/parse.ts b/workflow-canvas/src/api/parse.ts index 81b273f3..0527f959 100644 --- a/workflow-canvas/src/api/parse.ts +++ b/workflow-canvas/src/api/parse.ts @@ -1,10 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 import { ApiError } from './api'; import type { + CheckResult, DiagnosticView, RunStageView, RunStatus, RunView, + SaveResult, Session, Severity, StageRunStatus, @@ -205,6 +207,26 @@ export function parseWorkflowDocument(body: unknown): WorkflowDocument { }; } +/** Reads a `SaveResponse`; unknown fields are ignored. */ +export function parseSaveResult(body: unknown): SaveResult { + const saved = objectOf(body); + const view = parseView(saved.view); + return { + hash: stringOf(saved.hash), + view: view ?? fail(), + diagnostics: parseDiagnostics(saved.diagnostics), + }; +} + +/** Reads a `CheckResponse`; unknown fields are ignored. */ +export function parseCheckResult(body: unknown): CheckResult { + const check = objectOf(body); + return { + view: parseView(check.view), + diagnostics: parseDiagnostics(check.diagnostics), + }; +} + /** * The sentence of an `Error` body, followed by its `fix` (what to do about it), * or `undefined` when it has no sentence. @@ -224,9 +246,31 @@ export function errorRunIdOf(body: unknown): string | undefined { : undefined; } +/** The `current_hash` of a 409 `Error` body, or `undefined` when it has none. */ +export function errorCurrentHashOf(body: unknown): string | undefined { + return isObject(body) && + typeof body.current_hash === 'string' && + body.current_hash !== '' + ? body.current_hash + : undefined; +} + /** The `code` of an `Error` body, or `undefined` when it has none. */ export function errorCodeOf(body: unknown): string | undefined { return isObject(body) && typeof body.code === 'string' ? body.code : undefined; } + +/** + * The `diagnostics` of an `Error` body, or none when it has none or they + * cannot be read: the sentence of the error is still shown. + */ +export function errorDiagnosticsOf(body: unknown): DiagnosticView[] { + if (!isObject(body) || !Array.isArray(body.diagnostics)) return []; + try { + return parseDiagnostics(body.diagnostics); + } catch { + return []; + } +} diff --git a/workflow-canvas/src/api/types.ts b/workflow-canvas/src/api/types.ts index aebbc9a2..ebcc18a0 100644 --- a/workflow-canvas/src/api/types.ts +++ b/workflow-canvas/src/api/types.ts @@ -61,6 +61,13 @@ export interface SaveResult { diagnostics: DiagnosticView[]; } +/** What `POST /api/validate` returns. */ +export interface CheckResult { + /** `null` when the text has errors. */ + view: WorkflowView | null; + 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/canvas/WorkflowGraphCard.tsx b/workflow-canvas/src/canvas/WorkflowGraphCard.tsx index 1d357aae..e00974e6 100644 --- a/workflow-canvas/src/canvas/WorkflowGraphCard.tsx +++ b/workflow-canvas/src/canvas/WorkflowGraphCard.tsx @@ -44,7 +44,8 @@ const EDIT_NOTE = /** * The card both tabs draw the workflow in: its name, then the graph. The * graph is read-only unless `selection` is given, and `statuses` puts a live - * state on the nodes it names, by node id. `toolbar` sits above the graph. + * state on the nodes it names, by node id. `actions` sit at the right of the + * header, and `toolbar` above the graph. */ export function WorkflowGraphCard({ view, @@ -52,6 +53,7 @@ export function WorkflowGraphCard({ statuses = NO_STATUSES, selection, readOnly = false, + actions, toolbar, }: { view: WorkflowSpec; @@ -60,6 +62,7 @@ export function WorkflowGraphCard({ selection?: CanvasSelection; /** The workflow cannot be changed, so the note says it is read-only. */ readOnly?: boolean; + actions?: ReactNode; toolbar?: ReactNode; }) { const graph = useMemo( @@ -72,6 +75,7 @@ export function WorkflowGraphCard({
Workflow: {view.name}
diff --git a/workflow-canvas/src/chrome/chrome.css b/workflow-canvas/src/chrome/chrome.css index f990720d..70a4f7aa 100644 --- a/workflow-canvas/src/chrome/chrome.css +++ b/workflow-canvas/src/chrome/chrome.css @@ -25,3 +25,7 @@ padding: 0; list-style: none; } + +code { + font-family: var(--wf-font-code); +} diff --git a/workflow-canvas/src/designer/DesignerEditor.test.tsx b/workflow-canvas/src/designer/DesignerEditor.test.tsx index 2903b329..4856f06a 100644 --- a/workflow-canvas/src/designer/DesignerEditor.test.tsx +++ b/workflow-canvas/src/designer/DesignerEditor.test.tsx @@ -1,6 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 import createWrapper from '@cloudscape-design/components/test-utils/dom'; import { + act, fireEvent, render, screen, @@ -9,13 +10,27 @@ import { } from '@testing-library/react'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { App } from '../App'; +import { SaveConflict, SaveLost, SaveRefused, SessionLost } from './core'; import { FakeScheduler } from '../test-support/fake-scheduler'; -import { HELLO_DOCUMENT, HELLO_VIEW, loaded } from '../test-support/fixtures'; +import { + acceptingEdits, + finding, + SAVED_HASH, + viewOfText, + type FakeEdits, +} from '../test-support/fake-edits'; +import { + HELLO_DOCUMENT, + HELLO_HASH, + HELLO_TEXT, + HELLO_VIEW, + loaded, +} from '../test-support/fixtures'; import type { WorkflowDocument } from '../api/types'; -// The Cloudscape test utils wrap their updates in `act`, which React only -// accepts when the environment says so. Testing Library does that on its own -// only with test globals, so it is set for this file. +// The tests here wrap their updates in `act`, which React only accepts when the +// environment says so. Testing Library does that on its own only with test +// globals, and the other files do not wrap theirs, so it is set for this file. const actFlag = globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }; const actBefore = actFlag.IS_REACT_ACT_ENVIRONMENT; beforeAll(() => { @@ -25,20 +40,27 @@ afterAll(() => { actFlag.IS_REACT_ACT_ENVIRONMENT = actBefore; }); -async function openDesigner(document: WorkflowDocument = HELLO_DOCUMENT) { +async function openDesigner( + edits: FakeEdits = acceptingEdits(), + document: WorkflowDocument = HELLO_DOCUMENT, +) { + const scheduler = new FakeScheduler(); render( , ); await screen.findByTestId('node-end'); + return { edits, scheduler }; } const wrapper = () => createWrapper(document.body); const nameInput = () => screen.getByRole('textbox', { name: 'name' }) as HTMLInputElement; const agentSelect = () => wrapper().findSelect()!; +const nameError = () => + wrapper().findFormField()?.findError()?.getElement().textContent; // The reason of a button that is off, which the button carries after its label. function reasonOf(label: string): string | undefined { const text = wrapper() @@ -47,8 +69,10 @@ function reasonOf(label: string): string | undefined { .find((content) => content.startsWith(label)); return text?.slice(label.length) || undefined; } +const saveButton = () => screen.getByRole('button', { name: 'Save' }); const addButton = () => screen.getByRole('button', { name: 'Add stage' }); const deleteButton = () => screen.getByRole('button', { name: 'Delete stage' }); +const status = () => screen.getByTestId('save-status').textContent; const isOff = (button: HTMLElement) => button.hasAttribute('disabled') || button.getAttribute('aria-disabled') === 'true'; @@ -66,6 +90,13 @@ function chooseAgent(id: string) { select.selectOption(index + 1); } +// Runs the pause of the check and lets its answer reach the page. +async function checkNow(scheduler: FakeScheduler) { + await act(async () => { + scheduler.tick(); + }); +} + const stageLabels = () => screen .getAllByRole('group') @@ -174,7 +205,7 @@ describe('the Properties of the selected stage', () => { }); describe('renaming a stage', () => { - it('shows the new name on the node at once', async () => { + it('shows the new name on the node at once, and marks the draft unsaved', async () => { await openDesigner(); type('write'); @@ -182,6 +213,8 @@ describe('renaming a stage', () => { expect(screen.getByTestId('node-stage-write').textContent).toContain( 'write', ); + expect(status()).toBe('Unsaved changes'); + expect(isOff(saveButton())).toBe(false); }); it('keeps focus in the field while it is typed in', async () => { @@ -203,14 +236,13 @@ describe('renaming a stage', () => { expect(screen.getAllByTestId('node-stage-review')).toHaveLength(2); }); - it('draws the old name again when the name is put back', async () => { + it('is unsaved no more when the name is put back', async () => { await openDesigner(); type('write'); type('draft'); - expect(screen.queryByTestId('node-stage-write')).toBeNull(); - expect(screen.getByTestId('node-stage-draft')).toBeTruthy(); + expect(status()).toBe('No changes'); }); }); @@ -229,7 +261,7 @@ describe('picking the agent of a stage', () => { expect(options[2]).toContain('Backend and frontend code'); }); - it('shows the chosen agent on the node', async () => { + it('shows the chosen agent on the node and marks the draft unsaved', async () => { await openDesigner(); chooseAgent('k-architect'); @@ -237,6 +269,7 @@ describe('picking the agent of a stage', () => { expect(screen.getByTestId('node-stage-draft').textContent).toContain( 'k-architect ยท kiro-cli', ); + expect(status()).toBe('Unsaved changes'); }); it('keeps an agent that Konductor does not ship, and offers it first', async () => { @@ -247,7 +280,7 @@ describe('picking the agent of a stage', () => { stages: [{ name: 'draft', agent: 'my-agent' }], }, }; - await openDesigner(odd); + await openDesigner(acceptingEdits(), odd); expect(agentSelect().findTrigger().getElement().textContent).toContain( 'my-agent', @@ -271,6 +304,7 @@ describe('adding a stage', () => { ); expect(nameInput().value).toBe('agent-1'); await waitFor(() => expect(document.activeElement).toBe(nameInput())); + expect(status()).toBe('Unsaved changes'); expect(stageLabels()).toHaveLength(3); }); @@ -309,7 +343,7 @@ describe('adding a stage', () => { name: `stage-${index}`, agent: 'k-developer', })); - await openDesigner({ + await openDesigner(acceptingEdits(), { ...HELLO_DOCUMENT, view: { ...HELLO_VIEW, stages }, }); @@ -333,6 +367,7 @@ describe('deleting a stage', () => { expect(screen.queryByTestId('node-stage-review')).toBeNull(); expect(nameInput().value).toBe('draft'); await waitFor(() => expect(document.activeElement).toBe(nameInput())); + expect(status()).toBe('Unsaved changes'); }); it('is off, and says why, for the last stage', async () => { @@ -349,6 +384,600 @@ describe('deleting a stage', () => { }); }); +describe('saving', () => { + it('has nothing to save at first, and says why', async () => { + await openDesigner(); + + expect(isOff(saveButton())).toBe(true); + expect(status()).toBe('No changes'); + }); + + it('writes the saved text with the new stages on the hash it loaded', async () => { + const { edits } = await openDesigner(); + chooseAgent('k-architect'); + + fireEvent.click(saveButton()); + + expect(edits.save).toHaveBeenCalledOnce(); + expect(edits.save).toHaveBeenCalledWith( + HELLO_TEXT.replace('k-developer', 'k-architect'), + HELLO_HASH, + ); + await waitFor(() => expect(status()).toBe('Saved')); + }); + + it('says Saved, turns Save off and keeps focus on it', async () => { + await openDesigner(); + chooseAgent('k-architect'); + saveButton().focus(); + + fireEvent.click(saveButton()); + + await waitFor(() => expect(status()).toBe('Saved')); + expect(isOff(saveButton())).toBe(true); + expect(document.activeElement).toBe(saveButton()); + }); + + it('writes the next save on the hash of the last one', async () => { + const { edits } = await openDesigner(); + type('write'); + fireEvent.click(saveButton()); + await waitFor(() => expect(status()).toBe('Saved')); + + type('write-it'); + fireEvent.click(saveButton()); + + await waitFor(() => expect(edits.save).toHaveBeenCalledTimes(2)); + expect(edits.save.mock.calls[1]?.[1]).toBe(SAVED_HASH); + }); + + it('does not save twice when Save is pressed twice at once', async () => { + const { edits } = await openDesigner(); + type('write'); + + fireEvent.click(saveButton()); + fireEvent.click(saveButton()); + + await waitFor(() => expect(status()).toBe('Saved')); + expect(edits.save).toHaveBeenCalledOnce(); + }); + + it('says Saving while the call is under way', async () => { + const edits = acceptingEdits(); + let finish: () => void = () => {}; + edits.save.mockImplementation( + () => + new Promise((resolve) => { + finish = () => + resolve({ hash: SAVED_HASH, view: HELLO_VIEW, diagnostics: [] }); + }), + ); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + expect(status()).toBe('Saving'); + await act(async () => finish()); + }); +}); + +describe('checking while typing', () => { + it('does not ask the server until the text has been still for the pause', async () => { + const { edits, scheduler } = await openDesigner(); + + type('write'); + + expect(scheduler.pendingCount).toBe(1); + expect(edits.check).not.toHaveBeenCalled(); + }); + + it('asks once for a name typed in several keys', async () => { + const { edits, scheduler } = await openDesigner(); + + type('w'); + type('wr'); + type('write'); + await checkNow(scheduler); + + expect(edits.check).toHaveBeenCalledOnce(); + expect(edits.check).toHaveBeenCalledWith( + HELLO_TEXT.replace('name: draft', 'name: write'), + ); + }); + + it('shows what the server found under the field and in the list of problems', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValue({ + view: null, + diagnostics: [ + finding('stages[1].name', 'The stage name `draft` is already used.'), + ], + }); + const { scheduler } = await openDesigner(edits); + fireEvent.click(screen.getByTestId('node-stage-review')); + await waitFor(() => expect(nameInput().value).toBe('review')); + + type('draft'); + await checkNow(scheduler); + + const problems = await screen.findByTestId('problems'); + expect(problems.textContent).toContain( + 'draft: The stage name draft is already used.', + ); + expect(nameInput().getAttribute('aria-invalid')).toBe('true'); + expect(nameError()).toBe('The stage name draft is already used.'); + expect(screen.getByText('Needs a fix')).toBeTruthy(); + }); + + it('marks the field of the stage being renamed to a name that another stage has, though the server names the later stage', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValue({ + view: null, + diagnostics: [ + finding('stages[1].name', 'The stage name `review` is already used.'), + ], + }); + const { scheduler } = await openDesigner(edits); + expect(nameInput().value).toBe('draft'); + + type('review'); + await checkNow(scheduler); + + await waitFor(() => + expect(nameInput().getAttribute('aria-invalid')).toBe('true'), + ); + expect(nameError()).toBe('The stage name review is already used.'); + expect(screen.getByText('Needs a fix')).toBeTruthy(); + const items = within(screen.getByTestId('problems')).getAllByRole( + 'listitem', + ); + expect(items.map((item) => item.textContent)).toEqual([ + 'review: The stage name review is already used.', + ]); + }); + + it('names a stage with no name by its place in the list, not by an empty word', async () => { + const edits = acceptingEdits(); + const message = + '`name` must be lower-case letters, digits and hyphens, start with a letter or digit, and have at most 64 characters.'; + edits.check.mockResolvedValue({ + view: null, + diagnostics: [finding('stages[0].name', message)], + }); + const { scheduler } = await openDesigner(edits); + + type(''); + await checkNow(scheduler); + + const items = await within( + await screen.findByTestId('problems'), + ).findAllByRole('listitem'); + expect(items.map((item) => item.textContent)).toEqual([ + `Stage 1: ${message.replaceAll('`', '')}`, + ]); + }); + + it('shows a warning as a note, not as an error, and leaves Save on', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValue({ + view: null, + diagnostics: [ + { + ...finding('stages[0].name', 'The name is long.'), + severity: 'warning', + }, + ], + }); + const { scheduler } = await openDesigner(edits); + + type('write'); + await checkNow(scheduler); + + const notes = await screen.findByTestId('notes'); + expect(within(notes).getByText('write: The name is long.')).toBeTruthy(); + expect(screen.queryByTestId('problems')).toBeNull(); + expect(nameInput().getAttribute('aria-invalid')).not.toBe('true'); + expect(screen.queryByText('Needs a fix')).toBeNull(); + expect(screen.getByText('Has a note')).toBeTruthy(); + expect(isOff(saveButton())).toBe(false); + }); + + it('turns Save off, with the reason, while the check has found an error', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValue({ + view: null, + diagnostics: [finding('stages[0].name', 'Not a name.')], + }); + const { scheduler } = await openDesigner(edits); + saveButton(); + + type('Not A Name'); + await checkNow(scheduler); + + await screen.findByTestId('problems'); + expect(isOff(saveButton())).toBe(true); + fireEvent.click(saveButton()); + expect(edits.save).not.toHaveBeenCalled(); + }); + + it('keeps the problems on show while the next check is still to come', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValueOnce({ + view: null, + diagnostics: [finding('stages[0].name', 'Not a name.')], + }); + const { scheduler } = await openDesigner(edits); + type('Bad'); + await checkNow(scheduler); + const problems = await screen.findByTestId('problems'); + + type('Bad2'); + + expect(screen.getByTestId('problems')).toBe(problems); + expect(screen.getByText('Not a name.')).toBeTruthy(); + await checkNow(scheduler); + await waitFor(() => expect(screen.queryByTestId('problems')).toBeNull()); + }); + + it('clears the problems when the next check finds none', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValueOnce({ + view: null, + diagnostics: [finding('stages[0].name', 'Not a name.')], + }); + const { scheduler } = await openDesigner(edits); + type('Bad'); + await checkNow(scheduler); + await screen.findByTestId('problems'); + + type('good'); + await checkNow(scheduler); + + await waitFor(() => expect(screen.queryByTestId('problems')).toBeNull()); + expect(isOff(saveButton())).toBe(false); + }); + + it('shows a finding about the whole text without a stage in front of it', async () => { + const edits = acceptingEdits(); + edits.check.mockResolvedValue({ + view: null, + diagnostics: [finding('stages', 'A workflow needs a stage.')], + }); + const { scheduler } = await openDesigner(edits); + + type('write'); + await checkNow(scheduler); + + expect( + within(await screen.findByTestId('problems')).getByText( + 'A workflow needs a stage.', + ), + ).toBeTruthy(); + }); + + it('says nothing when the check cannot reach the server', async () => { + const edits = acceptingEdits(); + edits.check.mockRejectedValue(new Error('No answer.')); + const { scheduler } = await openDesigner(edits); + + type('write'); + await checkNow(scheduler); + + expect(screen.queryByTestId('problems')).toBeNull(); + expect(isOff(saveButton())).toBe(false); + }); +}); + +describe('a save that is refused', () => { + it('shows the findings of the server under the field', async () => { + const edits = acceptingEdits(); + edits.save.mockRejectedValue( + new SaveRefused('The workflow has errors.', [ + finding('stages.write.agent', 'The agent id is not valid.'), + ]), + ); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + const problems = await screen.findByTestId('problems'); + expect( + within(problems).getByText('write: The agent id is not valid.'), + ).toBeTruthy(); + expect(status()).toBe('Unsaved changes'); + }); +}); + +describe('a save that finds the file changed', () => { + async function conflict() { + const edits = acceptingEdits(); + edits.save.mockRejectedValue(new SaveConflict('The file changed.')); + const opened = await openDesigner(edits); + type('write'); + saveButton().focus(); + fireEvent.click(saveButton()); + await screen.findByTestId('conflict'); + return opened; + } + + it('says so, and offers to load the file again', async () => { + await conflict(); + + const alert = screen.getByTestId('conflict'); + expect(alert.textContent).toContain('The file changed on disk'); + expect(alert.textContent).toContain('nothing was saved'); + expect( + within(alert).getByRole('button', { name: 'Load the file again' }), + ).toBeTruthy(); + expect(isOff(saveButton())).toBe(true); + }); + + it('is announced, and leaves focus where it was', async () => { + await conflict(); + + expect( + screen.getByTestId('conflict').closest('[role="alert"]'), + ).toBeTruthy(); + expect(document.activeElement).toBe(saveButton()); + }); + + it('loads the file again, drops the draft and moves focus to the name', async () => { + const { edits } = await conflict(); + edits.reload.mockResolvedValue({ + ...HELLO_DOCUMENT, + hash: 'e'.repeat(64), + view: { ...HELLO_VIEW, stages: [{ name: 'only', agent: 'k-tpm' }] }, + }); + const reload = within(screen.getByTestId('conflict')).getByRole('button', { + name: 'Load the file again', + }); + reload.focus(); + + fireEvent.click(reload); + + await waitFor(() => expect(screen.queryByTestId('conflict')).toBeNull()); + expect(nameInput().value).toBe('only'); + expect(stageLabels()).toHaveLength(1); + expect(status()).toBe('Loaded again from the server'); + await waitFor(() => expect(document.activeElement).toBe(nameInput())); + }); + + it('lets the person take the draft away before it is replaced', async () => { + await conflict(); + + fireEvent.click(screen.getByRole('button', { name: 'Copy my version' })); + + const mine = (await screen.findByRole('textbox', { + name: 'My version of the workflow file', + })) as HTMLTextAreaElement; + expect(mine.value).toContain('- name: write'); + }); + + it('keeps the text of the draft in view after the file was loaded again', async () => { + const { edits } = await conflict(); + edits.reload.mockResolvedValue({ + ...HELLO_DOCUMENT, + hash: 'e'.repeat(64), + view: { ...HELLO_VIEW, stages: [{ name: 'only', agent: 'k-tpm' }] }, + }); + + fireEvent.click( + within(screen.getByTestId('conflict')).getByRole('button', { + name: 'Load the file again', + }), + ); + + const note = await screen.findByTestId('discarded'); + expect(note.textContent).toContain('Your changes were replaced'); + fireEvent.click( + within(note).getByRole('button', { name: 'Copy my version' }), + ); + const mine = (await screen.findByRole('textbox', { + name: 'My version of the workflow file', + })) as HTMLTextAreaElement; + expect(mine.value).toContain('- name: write'); + }); + + it('keeps the draft in view, with Copy my version, when the person types after the reload', async () => { + const { edits } = await conflict(); + edits.reload.mockResolvedValue({ + ...HELLO_DOCUMENT, + hash: 'e'.repeat(64), + view: { ...HELLO_VIEW, stages: [{ name: 'only', agent: 'k-tpm' }] }, + }); + fireEvent.click( + within(screen.getByTestId('conflict')).getByRole('button', { + name: 'Load the file again', + }), + ); + await screen.findByTestId('discarded'); + + type('typed'); + + const note = screen.getByTestId('discarded'); + fireEvent.click( + within(note).getByRole('button', { name: 'Copy my version' }), + ); + const mine = (await screen.findByRole('textbox', { + name: 'My version of the workflow file', + })) as HTMLTextAreaElement; + expect(mine.value).toContain('- name: write'); + }); + + it('does not let the stages change while the file loads', async () => { + const { edits } = await conflict(); + edits.reload.mockReturnValue(new Promise(() => {})); + + fireEvent.click( + within(screen.getByTestId('conflict')).getByRole('button', { + name: 'Load the file again', + }), + ); + + await waitFor(() => expect(nameInput().readOnly).toBe(true)); + expect(isOff(addButton())).toBe(true); + expect(isOff(deleteButton())).toBe(true); + }); + + it('keeps the conflict, with the sentence, when the load fails', async () => { + const { edits } = await conflict(); + edits.reload.mockRejectedValue(new Error('The server did not answer.')); + + fireEvent.click( + within(screen.getByTestId('conflict')).getByRole('button', { + name: 'Load the file again', + }), + ); + + await waitFor(() => + expect(screen.getByTestId('conflict').textContent).toContain( + 'The server did not answer.', + ), + ); + expect(nameInput().value).toBe('write'); + }); +}); + +describe('a check that the server refuses for a lost session', () => { + it('says so, and that the changes are lost with the page', async () => { + const edits = acceptingEdits(); + edits.check.mockRejectedValue( + new SessionLost( + 'The session is not valid. Get a fresh launch address from the server. The changes you made on this page are lost when you do; copy them first.', + ), + ); + const { scheduler } = await openDesigner(edits); + type('write'); + + await checkNow(scheduler); + + expect(await screen.findByText('The session is lost')).toBeTruthy(); + expect(screen.getByText(/copy them first/)).toBeTruthy(); + expect(screen.queryByText('The workflow was not saved')).toBeNull(); + }); +}); + +describe('a save that fails', () => { + it('shows the sentence of the failure and lets the save be tried again', async () => { + const edits = acceptingEdits(); + edits.save.mockRejectedValueOnce(new Error('The server did not answer.')); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + expect(await screen.findByText('The server did not answer.')).toBeTruthy(); + expect(status()).toBe('Unsaved changes'); + fireEvent.click(saveButton()); + await waitFor(() => expect(status()).toBe('Saved')); + expect(screen.queryByText('The server did not answer.')).toBeNull(); + }); +}); + +describe('a save whose answer is lost', () => { + it('counts the save as done when the file holds the text that was sent', async () => { + const edits = acceptingEdits(); + let sent = ''; + edits.save.mockImplementationOnce(async (text) => { + sent = text; + throw new SaveLost('The server did not answer.'); + }); + edits.reload.mockImplementationOnce(async () => ({ + ...HELLO_DOCUMENT, + text: sent, + hash: SAVED_HASH, + view: viewOfText(sent), + })); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + await waitFor(() => expect(status()).toBe('Saved')); + expect(screen.queryByText('The server did not answer.')).toBeNull(); + expect(nameInput().value).toBe('write'); + }); + + it('shows the failure when the file does not hold the text', async () => { + const edits = acceptingEdits(); + edits.save.mockRejectedValueOnce( + new SaveLost('The server did not answer.'), + ); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + expect(await screen.findByText('The server did not answer.')).toBeTruthy(); + expect(status()).toBe('Unsaved changes'); + }); +}); + +describe('a save whose answer is lost and whose file cannot be read', () => { + it('says it is not known whether the file was written', async () => { + const edits = acceptingEdits(); + edits.save.mockRejectedValueOnce( + new SaveLost('The server did not answer.'), + ); + edits.reload.mockRejectedValueOnce(new Error('Still down.')); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + expect(await screen.findByText('The save did not finish')).toBeTruthy(); + expect(screen.getByText(/may or may not have gone through/)).toBeTruthy(); + expect(screen.queryByText('The workflow was not saved')).toBeNull(); + }); +}); + +describe('a save that is refused with nothing to fix', () => { + it('shows the sentence of the server', async () => { + const edits = acceptingEdits(); + edits.save.mockRejectedValueOnce( + new SaveRefused('The workflow has errors.', []), + ); + await openDesigner(edits); + type('write'); + + fireEvent.click(saveButton()); + + expect(await screen.findByText('The workflow has errors.')).toBeTruthy(); + expect(screen.getByText('The workflow was not saved')).toBeTruthy(); + }); +}); + +describe('a file whose stages are not a block', () => { + const FLOW = { + ...HELLO_DOCUMENT, + text: 'name: hello\nintent: Add a greeting to the README\nstages: [{name: draft, agent: k-developer}]\n', + }; + + it('says before a save that the file is written whole', async () => { + await openDesigner(acceptingEdits(), FLOW); + + const note = screen.getByTestId('rewrite-notice'); + + expect(note.textContent).toContain('Saving rewrites this file'); + expect(note.textContent).toContain('comments'); + }); + + it('says nothing for a file with a block of stages', async () => { + await openDesigner(); + + expect(screen.queryByTestId('rewrite-notice')).toBeNull(); + }); + + it('says nothing when the workflow is read-only, as nothing will be saved', async () => { + await openDesigner(acceptingEdits(), { ...FLOW, readOnly: true }); + + expect(screen.getByTestId('read-only-note')).toBeTruthy(); + expect(screen.queryByTestId('rewrite-notice')).toBeNull(); + }); +}); + describe('the two tabs', () => { it('keeps the draft when the tab is switched away and back', async () => { await openDesigner(); @@ -372,11 +1001,24 @@ describe('the two tabs', () => { expect(within(panel).queryByTestId('node-stage-write')).toBeNull(); expect(within(panel).getByTestId('unsaved-note')).toBeTruthy(); }); + + it('draws what was saved on Execution, with no note', async () => { + await openDesigner(); + type('write'); + fireEvent.click(saveButton()); + await waitFor(() => expect(status()).toBe('Saved')); + + fireEvent.click(screen.getByRole('tab', { name: 'Execution' })); + + const panel = screen.getByRole('tabpanel', { name: 'Execution' }); + expect(await within(panel).findByTestId('node-stage-write')).toBeTruthy(); + expect(within(panel).queryByTestId('unsaved-note')).toBeNull(); + }); }); describe('a workflow that the server says is read-only', () => { it('turns off every control that would change it', async () => { - await openDesigner({ + await openDesigner(acceptingEdits(), { ...HELLO_DOCUMENT, readOnly: true, }); @@ -393,20 +1035,19 @@ describe('a workflow that the server says is read-only', () => { expect(isOff(deleteButton())).toBe(true); expect(reasonOf('Add stage')).toBe('The workflow is read-only.'); expect(reasonOf('Delete stage')).toBe('The workflow is read-only.'); + expect(isOff(saveButton())).toBe(true); }); - it('puts the note in a block that takes the whole row of the layout', async () => { - await openDesigner({ ...HELLO_DOCUMENT, readOnly: true }); + it('keeps the note out of the grid of the graph and Properties', async () => { + await openDesigner(undefined, { ...HELLO_DOCUMENT, readOnly: true }); - const block = screen - .getByTestId('read-only-note') - .closest('.wf-designer-note'); - - expect(block?.parentElement?.classList.contains('wf-designer')).toBe(true); + expect( + screen.getByTestId('read-only-note').closest('.wf-designer'), + ).toBeNull(); }); it('says in a note that it is read-only', async () => { - await openDesigner({ ...HELLO_DOCUMENT, readOnly: true }); + await openDesigner(undefined, { ...HELLO_DOCUMENT, readOnly: true }); expect(screen.getByTestId('read-only-note').textContent).toContain( 'The workflow is read-only.', @@ -415,6 +1056,22 @@ describe('a workflow that the server says is read-only', () => { }); }); +describe('a workflow that cannot be edited', () => { + it('is only drawn when it has no file', async () => { + render( + , + ); + + expect(await screen.findByText('Nothing to draw')).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'Save' })).toBeNull(); + }); +}); + describe('what a screen reader is told', () => { const live = () => screen.getByTestId('designer-live').textContent; diff --git a/workflow-canvas/src/designer/DesignerEditor.tsx b/workflow-canvas/src/designer/DesignerEditor.tsx index 767df0eb..97041705 100644 --- a/workflow-canvas/src/designer/DesignerEditor.tsx +++ b/workflow-canvas/src/designer/DesignerEditor.tsx @@ -9,14 +9,17 @@ import { stageNodeId } from '../graph'; import { canAddStage, canEdit, + changeBlocker, MAX_STAGES, nextStageName, READ_ONLY_REASON, } from './core'; import '../execution/announcements.css'; import './designer.css'; +import { EditAlerts, ProblemAlerts, RewriteNotice } from './EditAlerts'; import { PropertiesPane } from './PropertiesPane'; -import { useFocusRequest } from './use-focus'; +import { SaveControls } from './SaveControls'; +import { useFocusRequest, useFocusWhenDone } from './use-focus'; import type { Editor } from './use-editor'; const STAGE_LIMIT = `A workflow has at most ${MAX_STAGES} stages.`; @@ -36,9 +39,10 @@ function useAnnouncer(): [ } /** - * The editable Designer: the graph with Add stage, and Properties for the - * selected stage. Focus goes to the name of a stage after it is added, - * selected or deleted, so the keyboard never loses its place. + * The editable Designer: the graph with Add stage and Save, Properties for the + * selected stage, and the alerts. Focus goes to the name of a stage after it + * is added, selected or deleted, and after the file is loaded again, so the + * keyboard never loses its place. */ export function DesignerEditor({ editor, @@ -50,6 +54,11 @@ export function DesignerEditor({ const { state } = editor; const nameRef = useRef(null); const focusName = useFocusRequest(nameRef); + useFocusWhenDone( + nameRef, + state.save.phase === 'reloading', + state.save.phase === 'reloaded', + ); const spec = useMemo( () => ({ @@ -78,9 +87,10 @@ export function DesignerEditor({ ); const editable = canEdit(state); - const addable = editable && canAddStage(state.draft); + const blocker = changeBlocker(state); + const addable = blocker === undefined && canAddStage(state.draft); return ( -
+
{announcement.text}
+ + {editable && } {!editable && ( -
- - {READ_ONLY_REASON} You can look at the stages and select one, but - not change them. - -
+ + {READ_ONLY_REASON} You can look at the stages and select one, but not + change them. + )} - { - const name = nextStageName(state.draft); - editor.add(); - focusName({ select: true }); - announce(`Stage ${name} added and selected.`); +
+ } + toolbar={ + + } + /> +
+ { + const stage = state.draft.stages.find((s) => s.id === id); + editor.remove(id); + focusName(); + announce(`Stage ${stage?.name || 'with no name'} deleted.`); }} - > - Add stage - - } - /> - { - const stage = state.draft.stages.find((s) => s.id === id); - editor.remove(id); - focusName(); - announce(`Stage ${stage?.name || 'with no name'} deleted.`); - }} - /> + /> + +
+
); } diff --git a/workflow-canvas/src/designer/EditAlerts.tsx b/workflow-canvas/src/designer/EditAlerts.tsx new file mode 100644 index 00000000..431d0643 --- /dev/null +++ b/workflow-canvas/src/designer/EditAlerts.tsx @@ -0,0 +1,228 @@ +// SPDX-License-Identifier: Apache-2.0 +import Alert from '@cloudscape-design/components/alert'; +import Button from '@cloudscape-design/components/button'; +import Textarea from '@cloudscape-design/components/textarea'; +import { useState, type ReactNode } from 'react'; +import type { Draft, EditState, Problem } from './core'; +import { + errorsOf, + listedOf, + notesOf, + problemsOfDraft, + textOf, + willRewriteWhole, +} from './core'; +import { MessageText } from '../workflow/MessageText'; + +const CONFLICT_TEXT = + 'The file was changed after this page loaded, so nothing was saved. Load it again to see it as it is now; that replaces the changes you made here.'; + +// What to call the stage of a problem: its name, or its place in the list when +// it has none, so that a line never starts with an empty word. +function stageLabel(draft: Draft, id: string | undefined): string | undefined { + const index = draft.stages.findIndex((stage) => stage.id === id); + if (index < 0) return undefined; + return draft.stages[index]?.name || `Stage ${index + 1}`; +} + +// A problem in words, led by the stage it is about when it is about one. +function lineOf(problem: Problem, draft: Draft): ReactNode { + const label = stageLabel(draft, problem.stageId); + const message = ; + return label === undefined ? ( + message + ) : ( + <> + {label}: {message} + + ); +} + +// Puts the text on the clipboard. A page that may not, or has none, shows the +// text instead, so the changes can always be taken away. +function CopyVersion({ text }: { text: string }) { + const [shown, setShown] = useState<'idle' | 'copied' | 'text'>('idle'); + const copy = async () => { + try { + await navigator.clipboard.writeText(text); + setShown('copied'); + } catch { + setShown('text'); + } + }; + return ( + <> + + {shown === 'copied' &&
Copied.
} + {shown === 'text' && ( +