From 68d27f8c1325d30a2c2f6f381e82c4d2583eda97 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 20 Aug 2026 15:46:38 -0700 Subject: [PATCH 1/4] Add tests for the skill detail page and diff flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers the line-diff utility (edits, insertions, removals, line numbering, empty sides), the skill detail page's diff-confirmed save (Save… opens the review with the diff and writes nothing, Keep editing keeps the edit, Confirm & save publishes), the version list with compare and restore, and the new read of a skill at one commit. The roster and route suites move to the roster-only contract: /skills lists, and a single skill lives at its own route. Claude-Session: https://claude.ai/code/session_01Shhie5zM8L54bLHq5gFQti --- apps/web/test/routes.test.tsx | 8 +- apps/web/test/skill-detail-page.test.tsx | 288 +++++++++++++++++++++++ apps/web/test/skills-page.test.tsx | 116 +-------- apps/web/test/stage-top-nav.test.tsx | 31 +-- packages/skills/test/routes.test.ts | 39 +++ packages/text-diff/src/line-diff.test.ts | 98 ++++++++ 6 files changed, 450 insertions(+), 130 deletions(-) create mode 100644 apps/web/test/skill-detail-page.test.tsx create mode 100644 packages/text-diff/src/line-diff.test.ts diff --git a/apps/web/test/routes.test.tsx b/apps/web/test/routes.test.tsx index 86bfa5472..c7625484e 100644 --- a/apps/web/test/routes.test.tsx +++ b/apps/web/test/routes.test.tsx @@ -357,8 +357,14 @@ describe("routes render", () => { expect(markup).not.toContain("still being built"); }); + test("/skills/:slug renders the skill's own page, not a placeholder", async () => { + const markup = await renderApp("/skills/pr-review"); + expect(stagePageTitle(markup)).toBe("pr-review"); + expect(markup).not.toContain("Back to Skills"); + expect(activeFooterLabel(markup)).toBe("Skills"); + }); + test.each([ - ["/skills/pr-review", "pr-review", "Skills"], ["/plugins/linear", "linear", "Plugins"], ])( "%s titles the detail placeholder %s with its roster row lit", diff --git a/apps/web/test/skill-detail-page.test.tsx b/apps/web/test/skill-detail-page.test.tsx new file mode 100644 index 000000000..aa3225e74 --- /dev/null +++ b/apps/web/test/skill-detail-page.test.tsx @@ -0,0 +1,288 @@ +// The skill detail page at /skills/ (CL-6416): the editor, the +// diff-confirmed save, the version list, compare, and restore. Every case +// stubs `fetch` at the registry seam the page reads +// (`/api/tenants/:id/skills/...`) — no live server. + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; + +import { SkillDetailPage } from "../src/pages/skill-detail-page"; +import { TestQueryProvider } from "./test-query-provider"; + +const TENANT = "tnt_1"; +const NAME = "triage"; +const HEAD_BODY = "Read the report.\nPick one label."; + +const SKILL = { + assetId: "ast_1", + name: NAME, + description: "Sorts inbound issues.", + scope: "private", + creatorPrincipalId: "prn_1", + updatedAtIso: "2026-08-05T11:00:00.000Z", + body: HEAD_BODY, +}; + +const VERSIONS = [ + { + commitSha: "abcdef1234", + message: "Update triage", + author: "Ada", + committedAtIso: "2026-08-05T11:00:00.000Z", + current: true, + }, + { + commitSha: "0123456789", + message: "Create triage", + author: "Grace", + committedAtIso: "2026-08-04T11:00:00.000Z", + current: false, + }, +]; + +const BASE = `/api/tenants/${TENANT}/skills/${NAME}`; + +const ROUTES: Record = { + [`GET ${BASE}`]: { + skill: SKILL, + pinnedBy: [{ definitionId: "def_1", name: "Research Buddy" }], + }, + [`GET ${BASE}/versions`]: { versions: VERSIONS }, + [`GET ${BASE}/versions/0123456789`]: { + skill: { ...SKILL, body: "Read the report." }, + }, + [`PUT ${BASE}`]: { skill: SKILL }, + [`POST ${BASE}/restore`]: { skill: SKILL }, +}; + +let container: HTMLDivElement | null = null; +let root: Root | null = null; +let requested: { method: string; path: string; body: unknown }[] = []; +const originalFetch = globalThis.fetch; + +function stubRegistry(): void { + globalThis.fetch = (async (input: unknown, init?: RequestInit) => { + const path = String(input); + const method = init?.method ?? "GET"; + requested.push({ + method, + path, + body: + init?.body === undefined ? undefined : JSON.parse(String(init.body)), + }); + const key = `${method} ${path}`; + if (!(key in ROUTES)) { + return new Response( + JSON.stringify({ error: { message: `no stub for ${key}` } }), + { status: 404 }, + ); + } + return new Response(JSON.stringify(ROUTES[key]), { status: 200 }); + }) as unknown as typeof fetch; +} + +beforeEach(() => { + requested = []; + stubRegistry(); +}); + +afterEach(() => { + globalThis.fetch = originalFetch; + if (root !== null) { + act(() => { + root?.unmount(); + }); + root = null; + } + container?.remove(); + container = null; +}); + +async function settle() { + await act(async () => { + await Promise.resolve(); + }); + await act(async () => { + await Promise.resolve(); + }); +} + +async function mount(): Promise { + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); + await act(async () => { + root?.render( + + + , + ); + }); + await settle(); + if (container === null) throw new Error("container went away"); + return container; +} + +function buttonsIn(scope: ParentNode): HTMLButtonElement[] { + return Array.from(scope.querySelectorAll("button")); +} + +function buttonNamed(scope: ParentNode, label: string): HTMLButtonElement { + const found = buttonsIn(scope).find((button) => + button.textContent?.includes(label), + ); + if (found === undefined) throw new Error(`no "${label}" button`); + return found; +} + +async function click(button: HTMLButtonElement) { + await act(async () => { + button.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + await settle(); +} + +function typeInto(id: string, value: string) { + const el = document.getElementById(id) as HTMLTextAreaElement | null; + if (el === null) throw new Error(`no field #${id}`); + const setter = Object.getOwnPropertyDescriptor( + window.HTMLTextAreaElement.prototype, + "value", + )?.set; + if (setter === undefined) throw new Error("no native value setter"); + setter.call(el, value); + el.dispatchEvent(new Event("input", { bubbles: true })); +} + +function saves(): { method: string; path: string; body: unknown }[] { + return requested.filter( + (entry) => entry.method === "PUT" && entry.path === BASE, + ); +} + +describe("SkillDetailPage", () => { + test("renders the editor seeded from the published version, with its pins", async () => { + const el = await mount(); + const body = document.getElementById( + "skill-body", + ) as HTMLTextAreaElement | null; + expect(body?.value).toBe(HEAD_BODY); + expect(el.textContent).toContain("Research Buddy"); + }); + + test("the version list renders each commit's note, author, and when", async () => { + const el = await mount(); + const table = el.querySelector('table[aria-label="Versions"]'); + expect(table).not.toBeNull(); + expect(table?.textContent).toContain("Create triage"); + expect(table?.textContent).toContain("Grace"); + expect(table?.textContent).toContain("Version 1"); + expect(table?.textContent).toContain("current"); + }); + + test("Save… is offered only once the editor differs from the published version", async () => { + const el = await mount(); + const bar = el.querySelector('[data-testid="stage-top-bar-actions"]'); + expect(bar).not.toBeNull(); + if (bar === null) return; + expect(buttonNamed(bar, "Save…").disabled).toBe(true); + + await act(async () => { + typeInto("skill-body", `${HEAD_BODY}\nEscalate anything on fire.`); + }); + expect(buttonNamed(bar, "Save…").disabled).toBe(false); + }); + + test("Save… opens a confirmation showing the diff, and writes nothing yet", async () => { + const el = await mount(); + await act(async () => { + typeInto("skill-body", "Read the report.\nPick two labels."); + }); + await click(buttonNamed(el, "Save…")); + + expect(document.body.textContent).toContain("Review this save"); + const diff = document.body.querySelector('[data-testid="diff-view"]'); + expect(diff).not.toBeNull(); + expect(diff?.textContent).toContain("Pick one label."); + expect(diff?.textContent).toContain("Pick two labels."); + expect(saves()).toHaveLength(0); + }); + + test("Keep editing closes the review with the edit intact and nothing written", async () => { + const el = await mount(); + await act(async () => { + typeInto("skill-body", "Read the report.\nPick two labels."); + }); + await click(buttonNamed(el, "Save…")); + await click(buttonNamed(document.body, "Keep editing")); + + expect(document.body.textContent).not.toContain("Review this save"); + expect(saves()).toHaveLength(0); + const body = document.getElementById( + "skill-body", + ) as HTMLTextAreaElement | null; + expect(body?.value).toBe("Read the report.\nPick two labels."); + }); + + test("Confirm & save is what publishes the new version", async () => { + const el = await mount(); + await act(async () => { + typeInto("skill-body", "Read the report.\nPick two labels."); + }); + await click(buttonNamed(el, "Save…")); + await click(buttonNamed(document.body, "Confirm & save")); + + expect(saves()).toHaveLength(1); + expect(saves()[0]?.body).toEqual({ + description: "Sorts inbound issues.", + body: "Read the report.\nPick two labels.", + }); + }); + + test("Compare reads the chosen version and diffs it against the current one", async () => { + const el = await mount(); + const table = el.querySelector('table[aria-label="Versions"]'); + if (table === null) throw new Error("no version table"); + const compares = buttonsIn(table).filter( + (button) => button.textContent?.includes("Compare") && !button.disabled, + ); + expect(compares).toHaveLength(1); + await click(compares[0] as HTMLButtonElement); + + expect( + requested.some((entry) => entry.path === `${BASE}/versions/0123456789`), + ).toBe(true); + expect(el.textContent).toContain("compared with the current version"); + expect(el.textContent).toContain("Pick one label."); + }); + + test("Restore posts the chosen commit to the registry", async () => { + const el = await mount(); + const table = el.querySelector('table[aria-label="Versions"]'); + if (table === null) throw new Error("no version table"); + const restores = buttonsIn(table).filter( + (button) => button.textContent === "Restore" && !button.disabled, + ); + expect(restores).toHaveLength(1); + await click(restores[0] as HTMLButtonElement); + + const call = requested.find((entry) => entry.path === `${BASE}/restore`); + expect(call?.method).toBe("POST"); + expect(call?.body).toEqual({ commitSha: "0123456789" }); + }); + + test("a failed read says so rather than showing an empty editor", async () => { + globalThis.fetch = (async () => + new Response(JSON.stringify({ error: { message: "registry is down" } }), { + status: 503, + })) as unknown as typeof fetch; + const el = await mount(); + expect(el.textContent).toContain("Couldn't load this skill"); + expect(el.textContent).not.toContain("registry is down"); + }); +}); diff --git a/apps/web/test/skills-page.test.tsx b/apps/web/test/skills-page.test.tsx index 3d86a09f7..e26fe962c 100644 --- a/apps/web/test/skills-page.test.tsx +++ b/apps/web/test/skills-page.test.tsx @@ -69,7 +69,6 @@ afterEach(() => { async function mount( props: { readonly tenantId?: string | null; - readonly entityId?: string | null; readonly navigate?: (to: string) => void; } = {}, ) { @@ -148,19 +147,12 @@ describe("SkillsPage", () => { expect(el.textContent).toContain("Private"); }); - test("Create skill posts directly to the registry and opens the new skill's detail", async () => { + test("Create skill posts directly to the registry and opens the new skill's page", async () => { stubRoutes({ ...EMPTY_REGISTRY, [`POST /api/tenants/${TENANT}/skills`]: { skill: { ...TRIAGE, name: "summarize" }, }, - [`GET /api/tenants/${TENANT}/skills/summarize`]: { - skill: { ...TRIAGE, name: "summarize", body: "Do it." }, - pinnedBy: [], - }, - [`GET /api/tenants/${TENANT}/skills/summarize/versions`]: { - versions: [], - }, }); const navigated: string[] = []; const el = await mount({ navigate: (to) => navigated.push(to) }); @@ -262,115 +254,29 @@ describe("SkillsPage", () => { ).toHaveLength(1); }); - test("entityId opens the skill's detail with its version history and pins", async () => { + test("opening a row leaves the roster listed — a skill is never rendered inline", async () => { stubRoutes({ ...EMPTY_REGISTRY, [`GET /api/tenants/${TENANT}/skills`]: { skills: [TRIAGE] }, - [`GET /api/tenants/${TENANT}/skills/triage`]: { - skill: { ...TRIAGE, body: "Pick exactly one label." }, - pinnedBy: [{ definitionId: "def_1", name: "Research Buddy" }], - }, - [`GET /api/tenants/${TENANT}/skills/triage/versions`]: { - versions: [ - { - commitSha: "abcdef1234", - message: "Publish triage", - author: "workbench", - committedAtIso: "2026-08-05T11:00:00.000Z", - current: true, - }, - { - commitSha: "0123456789", - message: "Draft triage", - author: "workbench", - committedAtIso: "2026-08-04T11:00:00.000Z", - current: false, - }, - ], - }, - }); - const el = await mount({ entityId: "triage" }); - expect(el.textContent).toContain("Pick exactly one label."); - expect(el.textContent).toContain("Research Buddy"); - expect(el.textContent).toContain("Publish triage"); - expect(el.textContent).toContain("Restore"); - }); - - test("Restore posts the chosen commit to the registry", async () => { - stubRoutes({ - ...EMPTY_REGISTRY, - [`GET /api/tenants/${TENANT}/skills`]: { skills: [TRIAGE] }, - [`GET /api/tenants/${TENANT}/skills/triage`]: { - skill: { ...TRIAGE, body: "Pick exactly one label." }, - pinnedBy: [], - }, - [`GET /api/tenants/${TENANT}/skills/triage/versions`]: { - versions: [ - { - commitSha: "abcdef1234", - message: "Publish triage", - author: "workbench", - committedAtIso: "2026-08-05T11:00:00.000Z", - current: true, - }, - { - commitSha: "0123456789", - message: "Draft triage", - author: "workbench", - committedAtIso: "2026-08-04T11:00:00.000Z", - current: false, - }, - ], - }, - [`POST /api/tenants/${TENANT}/skills/triage/restore`]: { skill: TRIAGE }, - }); - const el = await mount({ entityId: "triage" }); - const restore = Array.from(el.querySelectorAll("button")).filter( - (button) => button.textContent === "Restore" && !button.disabled, - ); - expect(restore).toHaveLength(1); - await act(async () => { - restore[0]?.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); - const call = requested.find((entry) => - entry.path.endsWith("/skills/triage/restore"), - ); - expect(call?.body).toEqual({ commitSha: "0123456789" }); - }); - - test("Share with workbench shares a private skill with the whole workbench", async () => { - stubRoutes({ - ...EMPTY_REGISTRY, - [`GET /api/tenants/${TENANT}/skills`]: { skills: [TRIAGE] }, - [`GET /api/tenants/${TENANT}/skills/triage`]: { - skill: { ...TRIAGE, body: "Pick exactly one label." }, - pinnedBy: [], - }, - [`GET /api/tenants/${TENANT}/skills/triage/versions`]: { versions: [] }, - [`PUT /api/tenants/${TENANT}/skills/triage/scope`]: { skill: TRIAGE }, - }); - const el = await mount({ entityId: "triage" }); - const share = Array.from(el.querySelectorAll("button")).find( - (button) => button.textContent === "Share with workbench", + const el = await mount({ navigate: () => undefined }); + const row = Array.from(el.querySelectorAll("tr")).find((tr) => + tr.textContent?.includes("triage"), ); await act(async () => { - share?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + row?.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); - const call = requested.find((entry) => - entry.path.endsWith("/skills/triage/scope"), - ); - expect(call?.body).toEqual({ scope: "tenant" }); + expect(el.querySelector('table[aria-label="Skills"]')).not.toBeNull(); + expect(el.textContent).not.toContain("Version history"); + expect( + requested.some((entry) => entry.path.endsWith("/skills/triage")), + ).toBe(false); }); test("navigate is called with the skill's name when a row is selected", async () => { stubRoutes({ ...EMPTY_REGISTRY, [`GET /api/tenants/${TENANT}/skills`]: { skills: [TRIAGE] }, - [`GET /api/tenants/${TENANT}/skills/triage`]: { - skill: { ...TRIAGE, body: "Pick exactly one label." }, - pinnedBy: [], - }, - [`GET /api/tenants/${TENANT}/skills/triage/versions`]: { versions: [] }, }); const navigated: string[] = []; const el = await mount({ navigate: (to) => navigated.push(to) }); diff --git a/apps/web/test/stage-top-nav.test.tsx b/apps/web/test/stage-top-nav.test.tsx index 9e08439b0..ab0c2cdef 100644 --- a/apps/web/test/stage-top-nav.test.tsx +++ b/apps/web/test/stage-top-nav.test.tsx @@ -14,6 +14,7 @@ import { renderToStaticMarkup } from "react-dom/server"; import { NavigationProvider } from "../src/navigation"; import { BenchProvider } from "../src/bench-context"; import { PluginsRoute } from "../src/pages/plugins-page"; +import { SkillDetailPage } from "../src/pages/skill-detail-page"; import { SkillsPage } from "../src/pages/skills-page"; import { ProviderHealthProvider } from "../src/shell/provider-health-context"; import { StageTopBar } from "../src/shell/stage-top-bar"; @@ -213,8 +214,6 @@ describe("Skills declares its nav through the top-bar contract", () => { test("an open skill deep-links its parent level back to /skills", async () => { globalThis.fetch = ((input: RequestInfo | URL) => { const path = String(input); - if (path === `/api/tenants/${TENANT}/skills`) - return Promise.resolve(json({ skills: [SKILL] })); if (path === `/api/tenants/${TENANT}/skills/weekly-digest`) return Promise.resolve( json({ skill: { ...SKILL, body: "Do it." }, pinnedBy: [] }), @@ -227,7 +226,7 @@ describe("Skills declares its nav through the top-bar contract", () => { const el = await render( - + , ); @@ -237,13 +236,12 @@ describe("Skills declares its nav through the top-bar contract", () => { expect(trail?.querySelector('[aria-current="page"]')?.textContent).toBe( "weekly-digest", ); + expect(el.querySelector('table[aria-label="Skills"]')).toBeNull(); }); - test("the parent crumb is the way back: the route it navigates to puts the list view back", async () => { + test("the parent crumb is the way back: clicking it navigates to /skills", async () => { globalThis.fetch = ((input: RequestInfo | URL) => { const path = String(input); - if (path === `/api/tenants/${TENANT}/skills`) - return Promise.resolve(json({ skills: [SKILL] })); if (path === `/api/tenants/${TENANT}/skills/weekly-digest`) return Promise.resolve( json({ skill: { ...SKILL, body: "Do it." }, pinnedBy: [] }), @@ -254,21 +252,14 @@ describe("Skills declares its nav through the top-bar contract", () => { }) as typeof fetch; const navigated: string[] = []; - const at = (entityId: string | null) => ( + const el = await render( navigated.push(to)}> - navigated.push(to)} - entityId={entityId} - /> + - + , ); - const el = await render(at("weekly-digest")); - expect(el.querySelector('table[aria-label="Skills"]')).toBeNull(); - const parent = el.querySelector("a.stage-crumb-link"); await act(async () => { parent?.dispatchEvent( @@ -276,14 +267,6 @@ describe("Skills declares its nav through the top-bar contract", () => { ); }); expect(navigated).toContain("/skills"); - - // The router re-renders the same mounted page at the new path — the - // route, not click-local state, is what closes the detail view. - const back = await render(at(null)); - expect(back.querySelector('table[aria-label="Skills"]')).not.toBeNull(); - expect(back.querySelector('[aria-current="page"]')?.textContent).toBe( - "Skills", - ); }); }); diff --git a/packages/skills/test/routes.test.ts b/packages/skills/test/routes.test.ts index 17dbaa774..a65ba5674 100644 --- a/packages/skills/test/routes.test.ts +++ b/packages/skills/test/routes.test.ts @@ -102,6 +102,45 @@ test("PUT /:name creates a new version and leaves the prior one restorable", asy expect(restored.skill.body).toBe("Read the report. Pick one label."); }); +test("GET /:name/versions/:commitSha reads a prior version without cutting one", async () => { + const app = buildApp(); + await createSkill(app); + await app.request("/triage", { + method: "PUT", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + description: "Sorts inbound issues.", + body: "Read the report. Pick a severity label.", + }), + }); + + const versions = (await (await app.request("/triage/versions")).json()) as { + versions: { commitSha: string }[]; + }; + const prior = versions.versions[1]?.commitSha ?? ""; + + const response = await app.request(`/triage/versions/${prior}`); + expect(response.status).toBe(200); + const payload = (await response.json()) as { skill: { body: string } }; + expect(payload.skill.body).toBe("Read the report. Pick one label."); + + const after = (await (await app.request("/triage/versions")).json()) as { + versions: { commitSha: string }[]; + }; + expect(after.versions).toHaveLength(2); + const current = (await (await app.request("/triage")).json()) as { + skill: { body: string }; + }; + expect(current.skill.body).toBe("Read the report. Pick a severity label."); +}); + +test("GET /:name/versions/:commitSha for an unknown commit is a 404", async () => { + const app = buildApp(); + await createSkill(app); + const response = await app.request("/triage/versions/deadbeef"); + expect(response.status).toBe(404); +}); + test("PUT /:name for an unknown skill is a 404", async () => { const app = buildApp(); const response = await app.request("/does-not-exist", { diff --git a/packages/text-diff/src/line-diff.test.ts b/packages/text-diff/src/line-diff.test.ts new file mode 100644 index 000000000..fea03733b --- /dev/null +++ b/packages/text-diff/src/line-diff.test.ts @@ -0,0 +1,98 @@ +import { describe, expect, test } from "bun:test"; + +import { diffLines, diffTotals, hasChanges } from "./line-diff"; + +function render(before: string, after: string): string[] { + return diffLines(before, after).map((line) => { + const marker = + line.kind === "added" ? "+" : line.kind === "removed" ? "-" : " "; + return `${marker}${line.text}`; + }); +} + +describe("diffLines", () => { + test("identical text is all context", () => { + const lines = diffLines("one\ntwo", "one\ntwo"); + expect(lines.map((line) => line.kind)).toEqual(["context", "context"]); + expect(hasChanges(lines)).toBe(false); + expect(diffTotals(lines)).toEqual({ added: 0, removed: 0 }); + }); + + test("an edit in the middle leaves the surrounding lines as context", () => { + expect(render("one\ntwo\nthree", "one\nTWO\nthree")).toEqual([ + " one", + "-two", + "+TWO", + " three", + ]); + }); + + test("an inserted line is the only change", () => { + expect(render("one\nthree", "one\ntwo\nthree")).toEqual([ + " one", + "+two", + " three", + ]); + expect(diffTotals(diffLines("one\nthree", "one\ntwo\nthree"))).toEqual({ + added: 1, + removed: 0, + }); + }); + + test("a removed line is the only change", () => { + expect(render("one\ntwo\nthree", "one\nthree")).toEqual([ + " one", + "-two", + " three", + ]); + }); + + test("line numbers point at each side's own revision", () => { + const lines = diffLines("a\nb", "a\nc\nb"); + expect( + lines.map((line) => [ + line.kind, + line.beforeLineNumber, + line.afterLineNumber, + ]), + ).toEqual([ + ["context", 1, 1], + ["added", null, 2], + ["context", 2, 3], + ]); + }); + + test("empty before is all additions and empty after is all removals", () => { + expect(render("", "one\ntwo")).toEqual(["+one", "+two"]); + expect(render("one\ntwo", "")).toEqual(["-one", "-two"]); + expect(diffLines("", "")).toEqual([]); + }); + + test("carriage returns do not read as changed lines", () => { + expect(hasChanges(diffLines("one\r\ntwo", "one\ntwo"))).toBe(false); + }); + + test("a trailing blank line is a visible addition", () => { + expect(render("one", "one\n")).toEqual([" one", "+"]); + }); + + test("a rewritten block reads as its removals then its additions", () => { + expect(render("a\nb\nc\nd", "a\nx\ny\nd")).toEqual([ + " a", + "-b", + "-c", + "+x", + "+y", + " d", + ]); + }); + + test("a moved line is not reported as unchanged in both places", () => { + const lines = diffLines("header\nbody", "body\nheader"); + expect(diffTotals(lines).added).toBeGreaterThan(0); + expect(diffTotals(lines).removed).toBeGreaterThan(0); + expect( + lines.filter((line) => line.kind === "context").map((line) => line.text), + ).toHaveLength(1); + }); +}); From 2a6550067171f49f90a83fd2a48d4935aa3c5e20 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 20 Aug 2026 15:46:49 -0700 Subject: [PATCH 2/4] Skills: full detail page with diff-confirmed saves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /skills/ is now a real page instead of a placeholder: the skill's content editor, its version list, and a diff view — no new storage, since a skill's git history already is its version store. A save is never silent. "Save…" in the top bar opens a review step showing the diff between the published version and the editor buffer, with "Confirm & save" and "Keep editing"; the commit happens only on confirm. The same renderer draws the comparison between any earlier version and the current one, read through a new GET /:name/versions/:commitSha that reads a skill at one commit without writing anything. The line diff itself is @corbits/text-diff, a dependency-free longest-common-subsequence script so an edit in the middle of a document reads as that one edit. The roster keeps only what a roster does: the inline skill panel and the create dialog's edit mode are gone, so there is one skill editor rather than two. Claude-Session: https://claude.ai/code/session_01Shhie5zM8L54bLHq5gFQti --- apps/web/package.json | 1 + apps/web/src/pages/create-skill-dialog.tsx | 75 +-- apps/web/src/pages/detail-placeholders.tsx | 19 +- apps/web/src/pages/diff-view.tsx | 108 +++++ apps/web/src/pages/skill-detail-page.tsx | 528 +++++++++++++++++++++ apps/web/src/pages/skills-page.tsx | 334 +------------ apps/web/src/routes.tsx | 12 +- apps/web/src/skills-api.ts | 17 + bun.lock | 17 +- packages/icons/src/index.tsx | 1 + packages/skills/src/registry.ts | 36 ++ packages/skills/src/routes.ts | 14 + packages/text-diff/package.json | 19 + packages/text-diff/src/index.ts | 8 + packages/text-diff/src/line-diff.ts | 133 ++++++ packages/text-diff/tsconfig.json | 7 + 16 files changed, 937 insertions(+), 392 deletions(-) create mode 100644 apps/web/src/pages/diff-view.tsx create mode 100644 apps/web/src/pages/skill-detail-page.tsx create mode 100644 packages/text-diff/package.json create mode 100644 packages/text-diff/src/index.ts create mode 100644 packages/text-diff/src/line-diff.ts create mode 100644 packages/text-diff/tsconfig.json diff --git a/apps/web/package.json b/apps/web/package.json index f9cdeb6c0..b546bdb82 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -37,6 +37,7 @@ "@corbits/shell-layout": "workspace:*", "@corbits/slug": "workspace:*", "@corbits/tasks-ui": "workspace:*", + "@corbits/text-diff": "workspace:*", "@corbits/url-path": "workspace:*", "@corbits/workflow-catalog": "workspace:*", "@corbits/icons": "workspace:*", diff --git a/apps/web/src/pages/create-skill-dialog.tsx b/apps/web/src/pages/create-skill-dialog.tsx index ffec0f80f..24f407fcd 100644 --- a/apps/web/src/pages/create-skill-dialog.tsx +++ b/apps/web/src/pages/create-skill-dialog.tsx @@ -12,11 +12,10 @@ // frontmatter must carry. Rejecting it here beats a server error after // the person has typed a whole skill body. // -// CL-6355: the same form doubles as the edit surface — `mode="edit"` seeds -// it from `initialValues` and locks the name field (a skill's name is its -// identity; renaming means creating a new one). No second editor -// component: `SkillDetailView`'s "Edit" affordance opens this dialog with -// `mode="edit"` rather than duplicating the form. +// Creation only. Editing an existing skill happens on its own page +// (`skill-detail-page.tsx`, CL-6416), where a save is reviewed as a diff +// before it publishes a new version — this dialog has no edit mode to +// duplicate that flow. import { Button, @@ -63,7 +62,8 @@ const NAME_FIELD: IntakeField = { help: "Lowercase letters, digits, and hyphens — this becomes the skill's name in the registry.", }; -const DESCRIPTION_AND_BODY_FIELDS: readonly IntakeField[] = [ +const FIELDS: readonly IntakeField[] = [ + NAME_FIELD, { name: "description", label: "Description", @@ -82,33 +82,12 @@ const DESCRIPTION_AND_BODY_FIELDS: readonly IntakeField[] = [ }, ]; -const CREATE_FIELDS: readonly IntakeField[] = [ - NAME_FIELD, - ...DESCRIPTION_AND_BODY_FIELDS, -]; - -/** Edit mode drops the name field entirely rather than disabling it — a - * skill's name is its identity, not an editable property; renaming means - * creating a differently-named skill. The dialog shows it as static text - * instead (see `DialogDescription` below). */ -const EDIT_FIELDS: readonly IntakeField[] = DESCRIPTION_AND_BODY_FIELDS; - /** Every reason a submission is not yet valid, in plain language — never * a generic "invalid form". Exported so the create flow can be proven * without SSR-rendering the portal-based dialog (Radix portals yield no - * static markup). `mode="edit"` skips name validation — the field isn't - * shown, and the value carried through unchanged is already a valid name. */ -export function validationIssues( - values: FormValues, - mode: "create" | "edit" = "create", -): readonly string[] { + * static markup). */ +export function validationIssues(values: FormValues): readonly string[] { const issues: string[] = []; - if (mode === "edit") { - if (values.description.trim() === "") - issues.push("Description is required."); - if (values.body.trim() === "") issues.push("Skill body is required."); - return issues; - } const name = values.name.trim(); if (name === "") { issues.push("Name is required."); @@ -128,34 +107,26 @@ export function CreateSkillDialog({ open, onOpenChange, onSubmit, - mode = "create", - initialValues, }: { readonly open: boolean; readonly onOpenChange: (open: boolean) => void; - /** Writes the skill to the registry — `createSkill` in create mode, - * `updateSkill` (a new version) in edit mode. A rejection's message is - * shown inline and the form is left as typed. */ + /** Writes the skill to the registry. A rejection's message is shown + * inline and the form is left as typed. */ readonly onSubmit: (input: SkillCreateInput) => Promise; - readonly mode?: "create" | "edit"; - /** Required in edit mode: seeds the form with the skill being edited. */ - readonly initialValues?: SkillCreateInput; }) { - const startingValues = initialValues ?? EMPTY_VALUES; - const [values, setValues] = useState(startingValues); + const [values, setValues] = useState(EMPTY_VALUES); const [showIssues, setShowIssues] = useState(false); const [serverError, setServerError] = useState(null); const [submitting, setSubmitting] = useState(false); function reset() { - setValues(startingValues); + setValues(EMPTY_VALUES); setShowIssues(false); setServerError(null); } function handleOpenChange(next: boolean) { - if (next) setValues(startingValues); - else reset(); + reset(); onOpenChange(next); } @@ -167,8 +138,7 @@ export function CreateSkillDialog({ }); } - const fields = mode === "edit" ? EDIT_FIELDS : CREATE_FIELDS; - const issues = validationIssues(values, mode); + const issues = validationIssues(values); async function handleSubmit() { if (issues.length > 0) { @@ -195,13 +165,10 @@ export function CreateSkillDialog({ - - {mode === "edit" ? `Edit ${values.name}` : "Create skill"} - + Create skill - {mode === "edit" - ? "Saving publishes a new version — the version it replaces stays in history and can be restored." - : "Define a reusable capability an agent can declare and this workbench can share."} + Define a reusable capability an agent can declare and this workbench + can share. @@ -221,10 +188,10 @@ export function CreateSkillDialog({

)} @@ -238,9 +205,9 @@ export function CreateSkillDialog({ diff --git a/apps/web/src/pages/detail-placeholders.tsx b/apps/web/src/pages/detail-placeholders.tsx index da4a9af32..13d97d48f 100644 --- a/apps/web/src/pages/detail-placeholders.tsx +++ b/apps/web/src/pages/detail-placeholders.tsx @@ -4,15 +4,12 @@ // testable before the page behind it exists. import { Button, EmptyState, PageShell } from "@corbits/react-ui"; -import { Lightning, SquaresFour } from "@corbits/icons"; +import { SquaresFour } from "@corbits/icons"; import type { Slug } from "@corbits/slug"; import type { ReactNode } from "react"; import { Link } from "../navigation"; -import { - PLUGINS_PATH_PREFIX, - SKILLS_PATH_PREFIX, -} from "../path-ids"; +import { PLUGINS_PATH_PREFIX } from "../path-ids"; import { StageTopBar } from "../shell/stage-top-bar"; function DetailPlaceholder({ @@ -49,18 +46,6 @@ function DetailPlaceholder({ ); } -export function SkillDetailPlaceholder({ slug }: { readonly slug: Slug }) { - return ( - } - /> - ); -} - export function PluginDetailPlaceholder({ slug }: { readonly slug: Slug }) { return ( = { + context: " ", + added: "+", + removed: "-", +}; + +const ROW_CLASS: Record = { + context: "text-muted-foreground", + added: "bg-success/10 text-foreground", + removed: "bg-destructive/10 text-foreground", +}; + +function lineNumber(value: number | null): string { + return value === null ? "" : String(value); +} + +export function DiffSummary({ + before, + after, +}: { + readonly before: string; + readonly after: string; +}) { + const totals = diffTotals(diffLines(before, after)); + return ( +

+ {`+${String(totals.added)} added, −${String(totals.removed)} removed`} +

+ ); +} + +export function DiffView({ + before, + after, + unchangedNotice = "No changes yet.", +}: { + readonly before: string; + readonly after: string; + readonly unchangedNotice?: string; +}) { + const lines = diffLines(before, after); + + if (!hasChanges(lines)) { + return ( +

+ {unchangedNotice} +

+ ); + } + + return ( +
+ +
+ + + {lines.map((line, index) => ( + + + + + + + ))} + +
+ {lineNumber(line.beforeLineNumber)} + + {lineNumber(line.afterLineNumber)} + + {MARKER[line.kind]} + + {line.text === "" ? " " : line.text} +
+
+
+ ); +} + +export function DiffHeading({ + beforeLabel, + afterLabel, +}: { + readonly beforeLabel: string; + readonly afterLabel: string; +}) { + return ( +
+ {beforeLabel} + + {afterLabel} +
+ ); +} diff --git a/apps/web/src/pages/skill-detail-page.tsx b/apps/web/src/pages/skill-detail-page.tsx new file mode 100644 index 000000000..796c4aacd --- /dev/null +++ b/apps/web/src/pages/skill-detail-page.tsx @@ -0,0 +1,528 @@ +// The skill detail page at `/skills/` (CL-6416). One full page per +// skill, replacing the placeholder that stood at this route and absorbing +// the version-history panel the Skills roster used to carry inline — there +// is one skill editor now, and it lives here. +// +// Versioning is git: every save is a commit on the skill's own SKILL.md +// (`@corbits/skills`), so "the version list" is that commit history and +// "restore" is a new commit carrying an older content. Nothing on this +// page stores a version itself. +// +// A save is never silent. "Save…" opens a review step showing the diff +// between the version currently published and what is in the editor; the +// commit happens only when the reader confirms it, so nobody publishes a +// change they have not seen. + +import { + Badge, + Button, + Dialog, + DialogBody, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, + PageShell, + RichEmptyState, + Section, + Table, + TableBody, + TableCell, + TableHead, + TableHeader, + TableRow, + Textarea, + formatRelativeTime, +} from "@corbits/react-ui"; +import { GitDiff, Lightning } from "@corbits/icons"; +import { WorkbenchLoadingState } from "@corbits/chat-ui"; +import { useCallback, useEffect, useState } from "react"; + +import { useBench } from "../bench-context"; +import { SKILLS_PATH_PREFIX, skillIdFromPath } from "../path-ids"; +import { StageTopBar } from "../shell/stage-top-bar"; +import { + listSkillVersions, + loadSkill, + loadSkillAtVersion, + restoreSkillVersion, + setSkillScope, + updateSkill, + type PinnedByEntry, + type SkillDetail, + type SkillVersion, +} from "../skills-api"; +import { DiffHeading, DiffView } from "./diff-view"; + +type Loaded = { + readonly skill: SkillDetail; + readonly pinnedBy: readonly PinnedByEntry[]; + readonly versions: readonly SkillVersion[]; +}; + +type PageState = + | { readonly status: "loading" } + | ({ readonly status: "ready" } & Loaded) + | { readonly status: "error"; readonly message: string }; + +type Comparison = { + readonly version: SkillVersion; + readonly body: string; +}; + +function messageOf(cause: unknown): string { + return cause instanceof Error ? cause.message : String(cause); +} + +/** "Version 3 of 7" numbering: history is newest-first, so a row's number + * counts up from the oldest commit. */ +function versionLabel(total: number, index: number): string { + return `Version ${String(total - index)}`; +} + +export function SkillDetailPage({ + tenantId, + name, + now = Date.now(), +}: { + readonly tenantId: string | null; + readonly name: string; + readonly now?: number; +}) { + const [state, setState] = useState({ status: "loading" }); + const [draft, setDraft] = useState<{ + readonly description: string; + readonly body: string; + } | null>(null); + const [confirming, setConfirming] = useState(false); + const [saveError, setSaveError] = useState(null); + const [busy, setBusy] = useState(false); + const [comparison, setComparison] = useState(null); + + const reload = useCallback(async () => { + if (tenantId === null) return; + setState({ status: "loading" }); + try { + const [detail, versions] = await Promise.all([ + loadSkill(tenantId, name), + listSkillVersions(tenantId, name), + ]); + setState({ + status: "ready", + skill: detail.skill, + pinnedBy: detail.pinnedBy, + versions, + }); + setDraft({ + description: detail.skill.description, + body: detail.skill.body, + }); + setComparison(null); + } catch (cause) { + setState({ status: "error", message: messageOf(cause) }); + } + }, [tenantId, name]); + + useEffect(() => { + void reload(); + }, [reload]); + + const crumbs = [ + { label: "Skills", href: SKILLS_PATH_PREFIX }, + { label: name }, + ]; + + function frame(actions: React.ReactNode, body: React.ReactNode) { + return ( +
+ +
+ + {body} + +
+
+ ); + } + + if (tenantId === null) { + return frame( + null, +

+ Pick a workbench to see this skill. +

, + ); + } + + if (state.status === "error") { + return frame( + null, + } + title="Couldn't load this skill" + description="Something went wrong on our side. Try again in a moment." + actions={[{ label: "Retry", onClick: () => void reload() }]} + />, + ); + } + + if (state.status === "loading" || draft === null) { + return frame(null, ); + } + + const registryTenantId: string = tenantId; + const { skill, pinnedBy, versions } = state; + const shared = skill.scope === "tenant"; + const edited = + draft.body !== skill.body || draft.description !== skill.description; + + async function run(action: () => Promise) { + setBusy(true); + try { + await action(); + await reload(); + } catch (cause) { + setState({ status: "error", message: messageOf(cause) }); + } finally { + setBusy(false); + } + } + + async function confirmSave() { + if (draft === null) return; + setSaveError(null); + setBusy(true); + try { + await updateSkill(registryTenantId, skill.name, { + description: draft.description.trim(), + body: draft.body, + }); + setConfirming(false); + await reload(); + } catch (cause) { + setSaveError(messageOf(cause)); + } finally { + setBusy(false); + } + } + + async function compare(version: SkillVersion) { + if (comparison?.version.commitSha === version.commitSha) { + setComparison(null); + return; + } + try { + const at = await loadSkillAtVersion( + registryTenantId, + skill.name, + version.commitSha, + ); + setComparison({ version, body: at.body }); + } catch (cause) { + setState({ status: "error", message: messageOf(cause) }); + } + } + + const saveAction = ( + + ); + + return frame( + saveAction, +
+
+
+

+ {skill.name} +

+

+ Updated {formatRelativeTime(skill.updatedAtIso, now)} +

+
+
+ + {shared ? "Shared" : "Private"} + + +
+
+ +
+
+
+