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 ( - +