From 25dbb9bb82ab296ee24989ad111db96f43df3af6 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 15 Jun 2026 19:15:00 +0000 Subject: [PATCH] feat(projects): edit existing registered projects Closes #15: projects could only be added or removed, with no way to edit a registered project's name, repo, fork, or base branch. Backend: - store.UpdateProject does a full UPDATE by id, setting every editable field explicitly so emptied fields actually clear (unlike UpsertProject, which is keyed by repo and preserves empty values) and the repo itself can change. Returns ErrNotFound when no row matches and a new ErrConflict when the new repo collides with another project (repo is UNIQUE). - PUT /api/projects/{id} -> updateProject handler (reuses upsertProjectReq; repo required -> 400). ErrNotFound -> 404, ErrConflict -> 409 via the existing httpStatusFor helper. - Go tests for the store update (success, field-clear, not-found, conflict) and the PUT endpoint (200, 404, 400). Frontend: - api.put mirroring api.post; an "edit" (square-pen) icon. - AddProjectModal refactored into a shared ProjectModal that pre-fills and PUTs when given a project ("Edit project") and POSTs otherwise ("Add project", unchanged). Each project row gains an Edit button next to Remove. Verification: - Playwright e2e (ui/e2e/projects.spec.ts) drives the full stack against a real orcha server (fake agents, in-memory SQLite): adds a project, edits its name and base branch, and asserts the change is reflected. Added @playwright/test, playwright.config.ts, and a test:e2e npm script. --- .gitignore | 3 ++ internal/api/api.go | 29 +++++++++++++++- internal/api/api_test.go | 56 +++++++++++++++++++++++++++++++ internal/store/projects.go | 32 ++++++++++++++++++ internal/store/store.go | 4 +++ internal/store/store_test.go | 43 ++++++++++++++++++++++++ ui/e2e/projects.spec.ts | 46 ++++++++++++++++++++++++++ ui/package-lock.json | 64 ++++++++++++++++++++++++++++++++++++ ui/package.json | 4 ++- ui/playwright.config.ts | 38 +++++++++++++++++++++ ui/src/api.ts | 8 +++++ ui/src/icons.tsx | 1 + ui/src/pages/Projects.tsx | 53 +++++++++++++++++++++-------- 13 files changed, 366 insertions(+), 15 deletions(-) create mode 100644 ui/e2e/projects.spec.ts create mode 100644 ui/playwright.config.ts diff --git a/.gitignore b/.gitignore index e10d6a2..22eaf4a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,7 @@ ui/node_modules/ +ui/test-results/ +ui/playwright-report/ +ui/blob-report/ orcha.db orcha.db-wal orcha.db-shm diff --git a/internal/api/api.go b/internal/api/api.go index 47fc6a7..16a6e0e 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -65,6 +65,7 @@ func (s *Server) Handler() http.Handler { mux.HandleFunc("GET /api/projects", s.listProjects) mux.HandleFunc("POST /api/projects", s.upsertProject) + mux.HandleFunc("PUT /api/projects/{id}", s.updateProject) mux.HandleFunc("DELETE /api/projects/{id}", s.deleteProject) mux.HandleFunc("GET /api/questions", s.listQuestions) @@ -146,7 +147,8 @@ func httpStatusFor(err error) int { case errors.Is(err, store.ErrNotFound): return http.StatusNotFound case errors.Is(err, store.ErrNoCapacity), errors.Is(err, orch.ErrNoTarget), - errors.Is(err, store.ErrLockHeld), errors.Is(err, orch.ErrUnsafePublish): + errors.Is(err, store.ErrLockHeld), errors.Is(err, orch.ErrUnsafePublish), + errors.Is(err, store.ErrConflict): return http.StatusConflict default: return http.StatusInternalServerError @@ -255,6 +257,31 @@ func (s *Server) upsertProject(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, p) } +// updateProject edits an existing project by id. Unlike upsertProject (keyed by +// repo, empty fields keep old values), this is a full update: empty fields clear, +// and the repo itself can change. ErrNotFound → 404, a repo-unique collision with +// another project → 409. +func (s *Server) updateProject(w http.ResponseWriter, r *http.Request) { + var req upsertProjectReq + if err := decode(r, &req); err != nil { + writeErr(w, http.StatusBadRequest, err) + return + } + if req.Repo == "" { + writeErr(w, http.StatusBadRequest, errors.New("repo is required (owner/repo)")) + return + } + p := &model.Project{ + ID: r.PathValue("id"), Name: req.Name, Repo: req.Repo, PushRepo: req.PushRepo, + BaseBranch: req.BaseBranch, CloneURL: req.CloneURL, + } + if err := s.st.UpdateProject(p); err != nil { + writeErr(w, httpStatusFor(err), err) + return + } + writeJSON(w, http.StatusOK, p) +} + func (s *Server) deleteProject(w http.ResponseWriter, r *http.Request) { if err := s.st.DeleteProject(r.PathValue("id")); err != nil { writeErr(w, httpStatusFor(err), err) diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 9d7ceca..972a5c3 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -235,6 +235,62 @@ func TestSessionScreen_NoLiveScreenReturns204(t *testing.T) { } } +func putJSON(t *testing.T, url string, body any) *http.Response { + t.Helper() + b, _ := json.Marshal(body) + req, err := http.NewRequest(http.MethodPut, url, bytes.NewReader(b)) + if err != nil { + t.Fatalf("new PUT request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("PUT %s: %v", url, err) + } + return resp +} + +func TestUpdateProject_Endpoint(t *testing.T) { + srv, _, st := newTestServer(t) + p := &model.Project{Repo: "octo/repo", BaseBranch: "main"} + if err := st.UpsertProject(p); err != nil { + t.Fatalf("seed: %v", err) + } + + // 200: editing name and base branch persists and is returned. + resp := putJSON(t, srv.URL+"/api/projects/"+p.ID, map[string]any{ + "repo": "octo/repo", "name": "Edited", "base_branch": "develop", + }) + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("update status=%d, want 200", resp.StatusCode) + } + var got model.Project + _ = json.NewDecoder(resp.Body).Decode(&got) + if got.Name != "Edited" || got.BaseBranch != "develop" { + t.Fatalf("response did not reflect edit: %+v", got) + } + var listed []model.Project + getJSON(t, srv.URL+"/api/projects", &listed) + if len(listed) != 1 || listed[0].Name != "Edited" || listed[0].BaseBranch != "develop" { + t.Fatalf("edit not reflected in list: %+v", listed) + } + + // 404: unknown id. + resp404 := putJSON(t, srv.URL+"/api/projects/missing", map[string]any{"repo": "x/y"}) + resp404.Body.Close() + if resp404.StatusCode != http.StatusNotFound { + t.Fatalf("missing project status=%d, want 404", resp404.StatusCode) + } + + // 400: repo is required. + resp400 := putJSON(t, srv.URL+"/api/projects/"+p.ID, map[string]any{"name": "x"}) + resp400.Body.Close() + if resp400.StatusCode != http.StatusBadRequest { + t.Fatalf("missing repo status=%d, want 400", resp400.StatusCode) + } +} + func TestCreateTarget_RegistersAndHealthChecks(t *testing.T) { srv, _, _ := newTestServer(t) // A local target health-checks instantly and comes up online. diff --git a/internal/store/projects.go b/internal/store/projects.go index 06a9847..9c6b46f 100644 --- a/internal/store/projects.go +++ b/internal/store/projects.go @@ -3,6 +3,7 @@ package store import ( "database/sql" "errors" + "strings" "github.com/nathanwhit/orcha/internal/model" ) @@ -47,6 +48,37 @@ func (s *Store) UpsertProject(p *model.Project) error { return err } +// UpdateProject performs a full update of an existing project by id, setting +// every editable field explicitly so empty fields actually clear — unlike +// UpsertProject, which is keyed by repo and preserves empty values. This is the +// editing path: it can also change the repo. If name is empty it defaults to the +// repo (consistent with UpsertProject). It returns ErrNotFound when no project +// has the given id, and ErrConflict when the new repo collides with another +// project (the repo column is UNIQUE). +func (s *Store) UpdateProject(p *model.Project) error { + if p.Name == "" { + p.Name = p.Repo + } + p.UpdatedAt = s.Now() + res, err := s.db.Exec( + `UPDATE projects SET name=?, repo=?, push_repo=?, clone_url=?, base_branch=?, updated_at=? WHERE id=?`, + p.Name, p.Repo, p.PushRepo, p.CloneURL, p.BaseBranch, p.UpdatedAt, p.ID) + if err != nil { + if strings.Contains(err.Error(), "UNIQUE constraint failed") { + return ErrConflict + } + return err + } + n, err := res.RowsAffected() + if err != nil { + return err + } + if n == 0 { + return ErrNotFound + } + return nil +} + var projectCols = `id, name, repo, push_repo, clone_url, base_branch, created_at, updated_at` func scanProject(r rowScanner) (*model.Project, error) { diff --git a/internal/store/store.go b/internal/store/store.go index 79086f0..cbbeea1 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -26,6 +26,10 @@ var ErrNotFound = errors.New("store: not found") // holder owns it. var ErrLockHeld = errors.New("store: lock already held") +// ErrConflict is returned when a write violates a uniqueness constraint, e.g. +// updating a project's repo to one another project already owns. +var ErrConflict = errors.New("store: conflict") + // Clock returns the current time. It is injectable for deterministic tests. type Clock func() time.Time diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 2c5caff..af82950 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -340,6 +340,49 @@ func TestDeduplicatePRs(t *testing.T) { } } +func TestUpdateProject(t *testing.T) { + st := newTestStore(t) + p := &model.Project{Repo: "octo/repo", PushRepo: "me/repo", BaseBranch: "main"} + if err := st.UpsertProject(p); err != nil { + t.Fatalf("seed: %v", err) + } + + // Success: change every editable field, including clearing push_repo. + p.Name = "Renamed" + p.Repo = "octo/renamed" + p.PushRepo = "" + p.BaseBranch = "develop" + if err := st.UpdateProject(p); err != nil { + t.Fatalf("update: %v", err) + } + got, err := st.GetProject(p.ID) + if err != nil { + t.Fatalf("get: %v", err) + } + if got.Name != "Renamed" || got.Repo != "octo/renamed" || got.BaseBranch != "develop" { + t.Fatalf("update did not persist: %+v", got) + } + // An emptied field actually clears (unlike UpsertProject). + if got.PushRepo != "" { + t.Fatalf("push_repo should have cleared, got %q", got.PushRepo) + } + + // Not found. + if err := st.UpdateProject(&model.Project{ID: "nope", Repo: "x/y"}); !errors.Is(err, ErrNotFound) { + t.Fatalf("expected ErrNotFound, got %v", err) + } + + // Repo conflict: another project already owns the target repo. + other := &model.Project{Repo: "other/repo"} + if err := st.UpsertProject(other); err != nil { + t.Fatalf("seed other: %v", err) + } + got.Repo = "other/repo" + if err := st.UpdateProject(got); !errors.Is(err, ErrConflict) { + t.Fatalf("expected ErrConflict on repo collision, got %v", err) + } +} + func TestHasOpenQuestionBySession(t *testing.T) { st := newTestStore(t) if st.HasOpenQuestionBySession("s1") { diff --git a/ui/e2e/projects.spec.ts b/ui/e2e/projects.spec.ts new file mode 100644 index 0000000..f4c4ca4 --- /dev/null +++ b/ui/e2e/projects.spec.ts @@ -0,0 +1,46 @@ +import { test, expect } from "@playwright/test"; + +// Drives the full add -> edit flow for a registered project through the real UI +// and API, verifying the issue's ask: there is now a way to EDIT an existing +// project, not just add/remove. +test("a project can be added and then edited", async ({ page }) => { + // Unique repo per run so reruns against a reused server don't collide. + const repo = `octo/widget-${Date.now()}`; + // Distinct, non-overlapping names so "old name gone" can be asserted. + const name = "Widget Alpha"; + const renamed = "Gadget Beta"; + + await page.goto("/#/projects"); + await expect(page.getByRole("heading", { name: "Projects" })).toBeVisible(); + + // --- Add --- + await page.getByRole("button", { name: "Add project" }).click(); + await expect(page.getByRole("heading", { name: "Add project" })).toBeVisible(); + await page.getByPlaceholder("defaults to repo").fill(name); + await page.getByPlaceholder("owner/repo").fill(repo); + await page.getByPlaceholder("main").fill("develop"); + await page.getByRole("button", { name: "Save project" }).click(); + + // The new project shows up in the list with its repo and base branch. + await expect(page.getByText(repo)).toBeVisible(); + await expect(page.getByText("base develop")).toBeVisible(); + + // --- Edit --- + await page.getByRole("button", { name: "Edit project" }).click(); + await expect(page.getByRole("heading", { name: "Edit project" })).toBeVisible(); + // Fields are pre-filled from the existing project. + await expect(page.getByPlaceholder("defaults to repo")).toHaveValue(name); + await expect(page.getByPlaceholder("owner/repo")).toHaveValue(repo); + await expect(page.getByPlaceholder("main")).toHaveValue("develop"); + + // Change the name and base branch, then save. + await page.getByPlaceholder("defaults to repo").fill(renamed); + await page.getByPlaceholder("main").fill("trunk"); + await page.getByRole("button", { name: "Save project" }).click(); + + // The edit is reflected in the list; the old values are gone. + await expect(page.getByText(renamed)).toBeVisible(); + await expect(page.getByText("base trunk")).toBeVisible(); + await expect(page.getByText(name)).toHaveCount(0); + await expect(page.getByText("base develop")).toHaveCount(0); +}); diff --git a/ui/package-lock.json b/ui/package-lock.json index 9220abb..7cf118f 100644 --- a/ui/package-lock.json +++ b/ui/package-lock.json @@ -12,6 +12,7 @@ "react-dom": "^19.2.7" }, "devDependencies": { + "@playwright/test": "^1.49.0", "@tailwindcss/vite": "^4.3.0", "@types/react": "^19.2.17", "@types/react-dom": "^19.2.3", @@ -134,6 +135,22 @@ "url": "https://github.com/sponsors/Boshen" } }, + "node_modules/@playwright/test": { + "version": "1.61.0", + "resolved": "https://registry.npmjs.org/@playwright/test/-/test-1.61.0.tgz", + "integrity": "sha512-cKA5B6lpFEMyMGjxF54QihfYpB4FkEGH+qZhtArDEG+wezQAJY8Pq6C7T1SjWz+FFzt3TbyoXBQYk/0292TdJA==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright": "1.61.0" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=18" + } + }, "node_modules/@rolldown/binding-android-arm64": { "version": "1.0.3", "resolved": "https://registry.npmjs.org/@rolldown/binding-android-arm64/-/binding-android-arm64-1.0.3.tgz", @@ -1160,6 +1177,53 @@ "url": "https://github.com/sponsors/jonschlinkert" } }, + "node_modules/playwright": { + "version": "1.61.0", + "resolved": "https://registry.npmjs.org/playwright/-/playwright-1.61.0.tgz", + "integrity": "sha512-Z+7BeeqQPRRzklHsVFP4KTGIyMxKUmfeRA4WisM6G3/XW6nwGeX6fX9qYaDa+CiUqpOkb2f6X3nar05R3kSuJQ==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "playwright-core": "1.61.0" + }, + "bin": { + "playwright": "cli.js" + }, + "engines": { + "node": ">=18" + }, + "optionalDependencies": { + "fsevents": "2.3.2" + } + }, + "node_modules/playwright-core": { + "version": "1.61.0", + "resolved": "https://registry.npmjs.org/playwright-core/-/playwright-core-1.61.0.tgz", + "integrity": "sha512-caX7TrY3Ml6egyDX0WUcTHDxodl/b51y5wJOdCEA36QviK/s2g081hvmGs8eaE3DWb6NYZQ6BjO/QkNRPenoPA==", + "dev": true, + "license": "Apache-2.0", + "bin": { + "playwright-core": "cli.js" + }, + "engines": { + "node": ">=18" + } + }, + "node_modules/playwright/node_modules/fsevents": { + "version": "2.3.2", + "resolved": "https://registry.npmjs.org/fsevents/-/fsevents-2.3.2.tgz", + "integrity": "sha512-xiqMQR4xAeHTuB9uWm+fFRcIOgKBMiOBP+eXiyT7jsgVCq1bkVygt00oASowB7EdtpOHaaPgKt812P9ab+DDKA==", + "dev": true, + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": "^8.16.0 || ^10.6.0 || >=11.0.0" + } + }, "node_modules/postcss": { "version": "8.5.15", "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.15.tgz", diff --git a/ui/package.json b/ui/package.json index d3bb7ad..9adc807 100644 --- a/ui/package.json +++ b/ui/package.json @@ -6,13 +6,15 @@ "scripts": { "dev": "vite", "build": "tsc && vite build", - "preview": "vite preview" + "preview": "vite preview", + "test:e2e": "playwright test" }, "dependencies": { "react": "^19.2.7", "react-dom": "^19.2.7" }, "devDependencies": { + "@playwright/test": "^1.49.0", "@tailwindcss/vite": "^4.3.0", "@types/react": "^19.2.17", "@types/react-dom": "^19.2.3", diff --git a/ui/playwright.config.ts b/ui/playwright.config.ts new file mode 100644 index 0000000..8701157 --- /dev/null +++ b/ui/playwright.config.ts @@ -0,0 +1,38 @@ +import { defineConfig, devices } from "@playwright/test"; + +// End-to-end tests drive the full stack against a real orcha server. +// +// The webServer below builds the UI (which writes into internal/webui/dist) and +// then runs the Go server with `go run`, so the binary embeds the freshly built +// dashboard and serves both the SPA and the /api endpoints on one port. It uses +// fake agents and an in-memory SQLite DB, so each run starts from a clean slate +// and needs no external services. +// +// Run with: npm run test:e2e (from ui/). Requires a Go toolchain on PATH and +// browsers installed via `npx playwright install chromium`. +const PORT = 8137; +const BASE_URL = `http://127.0.0.1:${PORT}`; + +export default defineConfig({ + testDir: "./e2e", + fullyParallel: false, + forbidOnly: !!process.env.CI, + retries: 0, + workers: 1, + reporter: "list", + use: { + baseURL: BASE_URL, + trace: "on-first-retry", + }, + projects: [ + { name: "chromium", use: { ...devices["Desktop Chrome"] } }, + ], + webServer: { + // Build the UI then run the Go server from the repo root (one dir up). + command: `npm run build --prefix ui && go run ./cmd/orcha -fake-agents -addr 127.0.0.1:${PORT} -db ':memory:'`, + cwd: "..", + url: BASE_URL, + reuseExistingServer: !process.env.CI, + timeout: 180_000, + }, +}); diff --git a/ui/src/api.ts b/ui/src/api.ts index e4ae6a9..270fbca 100644 --- a/ui/src/api.ts +++ b/ui/src/api.ts @@ -257,6 +257,14 @@ export function post(url: string, body?: unknown): Promise { }).then(parse); } +export function put(url: string, body?: unknown): Promise { + return fetch(url, { + method: "PUT", + headers: body ? { "Content-Type": "application/json" } : undefined, + body: body ? JSON.stringify(body) : undefined, + }).then(parse); +} + export function del(url: string): Promise { return fetch(url, { method: "DELETE" }).then(parse); } diff --git a/ui/src/icons.tsx b/ui/src/icons.tsx index e58606f..e00e657 100644 --- a/ui/src/icons.tsx +++ b/ui/src/icons.tsx @@ -25,6 +25,7 @@ const PATHS = { target: "M12 22a10 10 0 1 0 0-20 10 10 0 0 0 0 20Z M12 16a4 4 0 1 0 0-8 4 4 0 0 0 0 8Z", stethoscope: "M5 3v6a5 5 0 0 0 10 0V3 M3 3h4 M13 3h4 M15 14v2a5 5 0 0 0 10 0v-1 M20 13.5a1.5 1.5 0 1 0 0-3 1.5 1.5 0 0 0 0 3Z", folder: "M3 7a2 2 0 0 1 2-2h4l2 2h8a2 2 0 0 1 2 2v10a2 2 0 0 1-2 2H5a2 2 0 0 1-2-2Z", + edit: "M11 4H4a2 2 0 0 0-2 2v14a2 2 0 0 0 2 2h14a2 2 0 0 0 2-2v-7 M18.5 2.5a2.12 2.12 0 0 1 3 3L12 15l-4 1 1-4Z", } as const; export type IconName = keyof typeof PATHS; diff --git a/ui/src/pages/Projects.tsx b/ui/src/pages/Projects.tsx index 0cb040f..20864cb 100644 --- a/ui/src/pages/Projects.tsx +++ b/ui/src/pages/Projects.tsx @@ -18,6 +18,7 @@ import { export function ProjectsPage() { const projects = usePoll(() => api.get("/api/projects"), 5000); const [adding, setAdding] = useState(false); + const [editing, setEditing] = useState(null); const ps = projects.data ?? []; return ( @@ -67,6 +68,11 @@ export function ProjectsPage() {

+ setEditing(p)} + /> setAdding(false)} - onAdded={() => { + onSaved={() => { setAdding(false); projects.reload(); }} /> )} + {editing && ( + setEditing(null)} + onSaved={() => { + setEditing(null); + projects.reload(); + }} + /> + )} ); } -function AddProjectModal({ +// ProjectModal both adds and edits a project. With no `project` it POSTs a new +// one ("Add project"); with one it pre-fills the fields and PUTs the edit +// ("Edit project"). +function ProjectModal({ + project, onClose, - onAdded, + onSaved, }: { + project?: api.Project; onClose: () => void; - onAdded: () => void; + onSaved: () => void; }) { - const [name, setName] = useState(""); - const [repo, setRepo] = useState(""); - const [pushRepo, setPushRepo] = useState(""); - const [base, setBase] = useState(""); + const editing = project != null; + const [name, setName] = useState(project?.name ?? ""); + const [repo, setRepo] = useState(project?.repo ?? ""); + const [pushRepo, setPushRepo] = useState(project?.push_repo ?? ""); + const [base, setBase] = useState(project?.base_branch ?? ""); const [busy, setBusy] = useState(false); const [err, setErr] = useState(null); @@ -119,13 +141,18 @@ function AddProjectModal({ setBusy(true); setErr(null); try { - await api.post("/api/projects", { + const body = { name, repo, push_repo: pushRepo, base_branch: base, - }); - onAdded(); + }; + if (editing) { + await api.put(`/api/projects/${project.id}`, body); + } else { + await api.post("/api/projects", body); + } + onSaved(); } catch (ex) { setErr(ex instanceof Error ? ex.message : String(ex)); setBusy(false); @@ -133,7 +160,7 @@ function AddProjectModal({ }; return ( - +