UI rework polish

ui/playwright/helpers/app.ts

@@ -17,7 +17,9 @@ export const routes = {
   models: "/models",
   modelNew: "/models/new",
   mcpServers: "/mcp",
+  mcpServerNew: "/mcp/new",
   prompts: "/prompts",
+  promptNew: "/prompts/new",
   substrate: "/substrate",
   /* The templates list is a tab of the agents page now. The old address still
      resolves — it redirects here — but a test should go where the reader goes. */

ui/playwright/tests/forms/required-fields.spec.ts

@@ -0,0 +1,157 @@
+import type { Locator, Page } from "@playwright/test";
+import { test, expect } from "../../fixtures/test";
+import { expectSettled, loadPage, routes } from "../../helpers/app";
+
+/** The fixture configuration the edit form opens on. */
+const modelEdit = "/models/kagent/default-model-config/edit";
+
+/** The fixture template the details page renders read-only. */
+const templateDetail = "/agent-templates/kagent/k8s-agent-7f3a91c";
+
+/**
+ * The asterisk on a field the form will not submit without.
+ *
+ * antd draws it from `required` on a `Form.Item`, and every authoring surface here
+ * gates its own submit in code — `draftProblems`, `modelDraftIssues`,
+ * `validateMcpServerForm` — rather than through antd's rules. So the mark and the
+ * gate are two separate statements about the same field, and nothing but a test
+ * keeps them agreeing. They had already come apart: the whole agent-template form
+ * carried no mark at all while refusing to save without a model configuration, and
+ * the model form's API key was required to create with nothing on screen to say so.
+ *
+ * Asserted as a pair each time — what is marked *and* what is not. A test that only
+ * checked the marks would pass just as well on a form that marked every field, which
+ * tells a reader nothing about which ones matter.
+ */
+
+/** One field's label, by the text a reader sees on it. */
+function fieldLabel(page: Page, text: string): Locator {
+  // Exact, because these labels are prefixes of each other: "Name" would otherwise
+  // match "Namespace" too, and match it first.
+  return page
+    .locator(".ant-form-item-label label")
+    .filter({ has: page.getByText(text, { exact: true }) });
+}
+
+/**
+ * The labels that carry the mark, and the ones that must not.
+ *
+ * Both lists in one helper so a call site reads as the claim it is making.
+ */
+async function expectRequired(
+  page: Page,
+  { marked, unmarked }: { marked: string[]; unmarked: string[] },
+): Promise<void> {
+  for (const text of marked) {
+    const label = fieldLabel(page, text);
+    await expect(label, `"${text}" is required, so it must be marked`).toHaveCount(1);
+    await expect(label).toHaveClass(/ant-form-item-required/);
+  }
+  for (const text of unmarked) {
+    const label = fieldLabel(page, text);
+    await expect(label, `"${text}" is optional, so it must not be marked`).toHaveCount(1);
+    await expect(label).not.toHaveClass(/ant-form-item-required/);
+  }
+}
+
+test("forms: a field the form refuses to submit without is marked required", async ({
+  page,
+}) => {
+  await test.step("1. the agent template form", async () => {
+    await loadPage(page, routes.agentTemplateNew, { title: "New agent template" });
+    await expectSettled(page);
+
+    // Name and the model configuration are what `draftProblems` refuses to submit
+    // without. Everything else on this form is genuinely optional — including the
+    // system prompt, which a template may take from its harness instead.
+    await expectRequired(page, {
+      marked: ["Name", "Model configuration"],
+      unmarked: ["Description", "System prompt"],
+    });
+  });
+
+  await test.step("2. the model form, where the API key is required only to create", async () => {
+    await loadPage(page, routes.modelNew, { title: "New model" });
+    await expectSettled(page);
+
+    await expectRequired(page, {
+      marked: ["Provider", "Model", "Name", "Namespace", "API key"],
+      // A radio group that arrives with a choice already made cannot be missing one.
+      unmarked: ["Authentication"],
+    });
+  });
+
+  await test.step("3. and not on an edit, which keeps the key it already has", async () => {
+    await loadPage(page, `${modelEdit}?mock=ok`);
+    await expectSettled(page);
+
+    // The fixture holds its credential in a Secret, so the inline-key field is not
+    // on screen until that is what the reader is choosing.
+    await page
+      .getByTestId("model-auth-type")
+      .getByText("API key", { exact: true })
+      .click();
+
+    // The label says the same thing in words; the mark has to agree with it, or the
+    // form is asking for a credential the cluster already holds.
+    await expectRequired(page, {
+      marked: ["Name", "Namespace"],
+      unmarked: ["API key (leave blank to keep existing)"],
+    });
+  });
+
+  await test.step("4. the harness form", async () => {
+    await loadPage(page, routes.harnessNew, { title: "New harness" });
+    await expectSettled(page);
+
+    await expectRequired(page, {
+      marked: [
+        "Namespace",
+        "Name",
+        "Runtime adapter",
+        "Workload image",
+        "Worker pool",
+        "Snapshot location",
+      ],
+      // Optional in the CRD's sense and warned about on screen instead: a harness
+      // with no selector is created and admits nothing.
+      unmarked: ["Admits agent templates labelled"],
+    });
+  });
+
+  await test.step("5. the MCP server form, whose namespace really is optional", async () => {
+    await loadPage(page, routes.mcpServerNew, { title: "New MCP server" });
+    await expectSettled(page);
+
+    // Unlike every other form here: `validateMcpServerForm` accepts a blank
+    // namespace and the controller defaults it, so a mark would be a lie.
+    await expectRequired(page, {
+      marked: ["Name", "Server URL"],
+      unmarked: ["Namespace", "TLS", "Headers"],
+    });
+  });
+
+  await test.step("6. the prompt library form", async () => {
+    await loadPage(page, routes.promptNew, { title: "New prompt library" });
+    await expectSettled(page);
+
+    await expectRequired(page, { marked: ["Namespace", "Name"], unmarked: [] });
+  });
+});
+
+/**
+ * Read-only is not a form, so it asks for nothing.
+ *
+ * The template details page renders the same component with `readOnly`, and an
+ * asterisk there would be asking a reader to supply something they are only looking
+ * at — on a template that already has it.
+ */
+test("forms: the read-only template view marks nothing as required", async ({ page }) => {
+  await loadPage(page, templateDetail);
+  await expectSettled(page);
+
+  await expect(page.locator(".ant-form-item-label label").first()).toBeVisible();
+  await expect(page.locator(".ant-form-item-label label.ant-form-item-required")).toHaveCount(
+    0,
+  );
+});

ui/playwright/tests/substrate/substrate.spec.ts

@@ -1,5 +1,6 @@
 import { test, expect } from "../../fixtures/test";
 import { expectSettled, loadPage, routes } from "../../helpers/app";
+import { paint, settledPaint } from "../../helpers/style";
 
 /**
  * Substrate — the inventory, its scope, and the three ways the read can answer.
@@ -316,58 +317,84 @@ test("substrate: each list narrows on its own, and a match is found wherever it
 });
 
 /**
- * Which tables offer a sort, which is currently a decision nobody has revisited.
+ * All four tables sort the same way, and the paged two say honestly what they sorted.
  *
- * Every column of the actor and worker tables used to sort. The sorters were taken away
- * when those lists became paged reads, and the reasoning was sound: a client-side sorter
- * reorders the page it was handed, so "sort by status descending" shows the last status
- * on *this page* rather than in the cluster, and the first row of a sorted 410,110
- * actors is almost certainly not among the hundred on screen.
+ * The actor and worker columns used to carry a header of this page's own: a button around
+ * the title, an arrow beside it, and nothing outside those few words to click. It was
+ * written that way to avoid antd's `sorter`, which reorders the rows the table was handed
+ * — and one page out of 410,110 reordered is not the cluster sorted.
  *
- * That reason has gone. `ListSubstrateActors` and `ListSubstrateWorkers` were removed,
- * `GetSubstrateStatus` returns the whole inventory again, and these tables hold every
- * row — so a sorter here would now be as honest as the one the templates table keeps.
- * The sorters have not come back, which is why this test's name no longer claims a
- * principle: it pins what the page does today, and the open question is whether the
- * actor and worker tables should sort again now that they could.
+ * The concern was right and the remedy was not: the page ended up with two tables that
+ * sort by clicking a header and two that sort by clicking the words inside one, which is
+ * a page a reader has to learn twice. What the columns declare now is `sorter: true` —
+ * antd's header, with no comparator behind it — so the whole cell is the target and the
+ * chevrons show the direction, while the table still reorders nothing itself. A click
+ * becomes the next read, which orders every row before this page gets a slice of it.
  *
- * Kept rather than deleted because the assertion is still load-bearing in one direction.
- * If these reads are paged again — and `DEFERRED.md` explains why they were the first
- * time — a sorter added in the meantime becomes exactly the half-truth described above,
- * and this is what would object.
+ * Not the server, which takes a namespace and nothing else: the ordering is applied in
+ * `localPage` over the whole inventory. That is still the honest claim at this size —
+ * the order holds over the cluster rather than over the hundred rows on screen — and
+ * what the strip beside each table has to say, which is the half this pins. If a
+ * comparator is ever handed to one of these tables, the order would hold over the page
+ * alone and these assertions are what would object.
  */
-test("substrate: the actor and worker tables offer no sort, and the inline ones do", async ({
+test("substrate: every table sorts through the same header, and the paged two order the lot", async ({
   page,
 }) => {
   await loadPage(page, routes.substrate, { title: "Substrate" });
   await expectSettled(page);
 
-  await test.step("1. the actor and worker columns offer no sort", async () => {
-    for (const testId of ["substrate-actors-table", "substrate-workers-table"]) {
+  await test.step("1. every table's headers are antd's own sort controls", async () => {
+    for (const testId of [
+      "substrate-pools-table",
+      "substrate-templates-table",
+      "substrate-actors-table",
+      "substrate-workers-table",
+    ]) {
       const headers = page.getByTestId(testId).locator("th");
       await expect(headers.first()).toBeVisible();
       const sortable = await headers.evaluateAll((cells) =>
         cells.filter((cell) => cell.className.includes("column-has-sorters")).length,
       );
+      const total = await headers.count();
       expect(
         sortable,
-        `${testId} offers no sort today; adding one is a decision, and paging these reads again would make it wrong`,
-      ).toBe(0);
+        `${testId}: every column sorts, and through the header rather than a control inside it`,
+      ).toBe(total);
     }
   });
 
-  await test.step("2. the pools and templates do sort", async () => {
-    const headers = page.getByTestId("substrate-templates-table").locator("th");
-    await expect(headers.first()).toHaveClass(/column-has-sorters/);
+  await test.step("2. the actors' order covers every row, and cycles back to the default", async () => {
+    const order = page.getByTestId("substrate-actors-order");
+    await expect(order).toContainText("status, then actor");
+
+    // The header, not the words in it: clicking the cell is what a reader does on the
+    // two tables above, and this is the assertion that the same click works here.
+    const header = page.getByTestId("substrate-actors-table").locator("th").first();
+    await header.click();
+    await expect(order).toContainText("Sorted across the whole inventory: actor, ascending");
+
+    await header.click();
+    await expect(order).toContainText("Sorted across the whole inventory: actor, descending");
+
+    // antd's third click clears the sort, which for a read that always arrives ordered
+    // means the order it falls back to rather than no order at all.
+    await header.click();
+    await expect(order).toContainText("status, then actor");
   });
 
-  await test.step("3. the actors are grouped by status, in an order nobody asked for", async () => {
+  await test.step("3. and the workers' the same", async () => {
+    const order = page.getByTestId("substrate-workers-order");
+    await expect(order).toContainText("pool, then pod");
+
+    await page.getByTestId("substrate-workers-table").locator("th").nth(1).click();
+    await expect(order).toContainText("Sorted across the whole inventory: pool, ascending");
+  });
+
+  await test.step("4. the actors are grouped by status, in an order nobody asked for", async () => {
     // Stated rather than asked for: ate-api returns actors in whatever order it holds
     // them, so the same actor would appear somewhere different on every poll. Something
-    // has to impose an order, and with the paged reads gone that something is
-    // `api/grpc/operations.ts`, which sorts what it filtered — where it used to be the
-    // server. Either way the point is that a row does not move under the pointer while
-    // it is being read.
+    // has to impose an order, and that something is the read rather than the table.
     const statuses = await page
       .getByTestId("substrate-actors-table")
       .locator(".ant-table-row")
@@ -377,3 +404,51 @@ test("substrate: the actor and worker tables offer no sort, and the inline ones
     expect(statuses).toEqual([...statuses].sort());
   });
 });
+
+/**
+ * Nothing on this page is a link, and nothing on it lights up under the pointer.
+ *
+ * A row that changes colour on hover reads as a click target. None of these four is one:
+ * there is no page for an actor, a worker, a pool or a template to open. The app has a
+ * rule for exactly this — hover is opt-in through `clickable-table-row` — and it was
+ * written as `tr:hover > td`, which a virtual table has neither of. So the two windowed
+ * tables here went on hovering while every other static table in the app had stopped,
+ * and this page offered both behaviours at once.
+ *
+ * Both bodies are checked because they are different markup: the pools and templates are
+ * a real `table`, the actors and workers are divs from antd's virtual list.
+ */
+test("substrate: rows nobody can click do not light up under the pointer", async ({
+  page,
+}) => {
+  await loadPage(page, routes.substrate, { title: "Substrate" });
+  await expectSettled(page);
+
+  for (const testId of [
+    "substrate-pools-table",
+    "substrate-templates-table",
+    "substrate-actors-table",
+    "substrate-workers-table",
+  ]) {
+    const row = page.getByTestId(testId).locator(".ant-table-row").first();
+    await expect(row).toBeVisible();
+    const cell = row.locator(".ant-table-cell").first();
+
+    const atRest = (await paint(cell)).background;
+    await row.hover();
+    /*
+     * That the hover landed is asserted before what it painted. antd marks the hovered
+     * row's cells whatever the app then does with them, so this separates "the rule
+     * suppressed the highlight" from "the pointer never arrived" — which the colour
+     * comparison alone cannot do, and which a fixed wait on a loaded box invites.
+     */
+    await expect(cell).toHaveClass(/ant-table-cell-row-hover/);
+    // Waited out rather than polled: the claim is that nothing happens, and there is no
+    // event for a transition that never starts. See `helpers/style`.
+    const hovered = (await settledPaint(cell)).background;
+
+    expect(hovered, `${testId}: a row that cannot be clicked must not look clickable`).toBe(
+      atRest,
+    );
+  }
+});