diff --git a/packages/server/src/fetchers.integration.test.ts b/packages/server/src/fetchers.integration.test.ts index 1c0b259..b5a2878 100644 --- a/packages/server/src/fetchers.integration.test.ts +++ b/packages/server/src/fetchers.integration.test.ts @@ -23,10 +23,16 @@ vi.mock("./github-client.js", () => ({ }), })); +const cacheTimes = new Map(); vi.mock("./cache.js", () => ({ getCached: (key: string) => cacheStore.get(key) ?? null, setCached: (key: string, data: unknown) => { cacheStore.set(key, data); + cacheTimes.set(key, Date.now()); + }, + cacheAge: (key: string) => { + const t = cacheTimes.get(key); + return t == null ? null : Date.now() - t; }, })); diff --git a/packages/server/src/fetchers.ts b/packages/server/src/fetchers.ts index b5189de..6e3900b 100644 --- a/packages/server/src/fetchers.ts +++ b/packages/server/src/fetchers.ts @@ -1,5 +1,5 @@ import type { Octokit } from "@octokit/rest"; -import { getCached, setCached } from "./cache.js"; +import { cacheAge, getCached, setCached } from "./cache.js"; import { getClient, getInstance } from "./github-client.js"; interface RepoSettings { @@ -7,6 +7,7 @@ interface RepoSettings { } const inFlightRepoSettings = new Map>(); +const REPO_SETTINGS_TTL_MS = 2 * 60 * 1000; export async function getRepoSettings( client: Octokit, @@ -15,7 +16,11 @@ export async function getRepoSettings( repo: string, ): Promise { const key = `${instanceId}:repo-settings:${owner}/${repo}`; - const cached = getCached(key); + const age = cacheAge(key); + const cached = + age !== null && age < REPO_SETTINGS_TTL_MS + ? getCached(key) + : null; if (cached) return cached; const existing = inFlightRepoSettings.get(key); if (existing) return existing; diff --git a/packages/server/src/routes.test.ts b/packages/server/src/routes.test.ts index 02e5f69..87c30df 100644 --- a/packages/server/src/routes.test.ts +++ b/packages/server/src/routes.test.ts @@ -353,7 +353,7 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/merge", () => { expect(fetchersStub.fetchRecentPrs).toHaveBeenCalledWith("github"); }); - it("forwards GitHub's first error line when merge is rejected", async () => { + it("forwards GitHub's full error message (joining non-empty lines) when merge is rejected", async () => { const ghErr = Object.assign(new Error("405"), { status: 405, response: { @@ -368,7 +368,8 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/merge", () => { expect(res.status).toBe(422); expect(await res.json()).toEqual({ error: "merge_rejected", - message: "Repository rule violations found", + message: + "Repository rule violations found — A conversation must be resolved before this pull request can be merged.", }); }); }); diff --git a/packages/server/src/routes.ts b/packages/server/src/routes.ts index 21a220b..f38234f 100644 --- a/packages/server/src/routes.ts +++ b/packages/server/src/routes.ts @@ -289,12 +289,19 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { merge_method: "squash", }); } catch (err) { - // GitHub returns helpful messages here ("A conversation must be - // resolved…", "At least 1 approving review is required…"). Forward - // the first line so the toast is actionable. + // GitHub's message is often multi-line: a generic header + // ("Repository rule violations found") followed by the actionable + // detail ("A conversation must be resolved…"). Join non-empty lines + // so the toast carries the reason, not just the header. const e = err as { response?: { data?: { message?: string } } }; - const message = - e.response?.data?.message?.split("\n")[0] ?? "Failed to merge PR"; + const raw = e.response?.data?.message; + const message = raw + ? raw + .split("\n") + .map((l) => l.trim()) + .filter((l) => l.length > 0) + .join(" — ") + : "Failed to merge PR"; return c.json({ error: "merge_rejected", message }, 422); } diff --git a/packages/web/src/mutations.test.ts b/packages/web/src/mutations.test.ts new file mode 100644 index 0000000..53239df --- /dev/null +++ b/packages/web/src/mutations.test.ts @@ -0,0 +1,129 @@ +import { QueryClient } from "@tanstack/react-query"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { AutoMergeNotAllowedError } from "./api"; + +vi.mock("./api", async () => { + const actual = await vi.importActual("./api"); + return { + ...actual, + api: { + mergePr: vi.fn(), + toggleAutoMerge: vi.fn(), + }, + }; +}); + +vi.mock("sonner", () => ({ + toast: Object.assign(vi.fn(), { + success: vi.fn(), + error: vi.fn(), + }), +})); + +import { api } from "./api"; +import { mergePr } from "./mutations"; +import { toast } from "sonner"; +import type { PR } from "./types"; + +const mergePrMock = vi.mocked(api.mergePr); +const toggleAutoMergeMock = vi.mocked(api.toggleAutoMerge); +const toastSuccess = vi.mocked(toast.success); +const toastError = vi.mocked(toast.error); + +const target = { + instanceId: "github", + repo: "o/r", + number: 42, + title: "Add thing", + url: "https://github.com/o/r/pull/42", +}; + +function basePr(overrides: Partial = {}): PR { + return { + id: 1, + number: 42, + title: "Add thing", + body: "", + url: "https://github.com/o/r/pull/42", + repo: "o/r", + updatedAt: "2026-04-30T00:00:00Z", + author: "anton", + authorAvatar: "", + draft: false, + ciStatus: "success", + inMergeQueue: false, + autoMerge: false, + headBranch: "feat", + baseBranch: "main", + reviews: { approved: [], changesRequested: [] }, + additions: 1, + deletions: 0, + commits: 1, + commentCount: 0, + labels: [], + ...overrides, + }; +} + +describe("mergePr", () => { + let qc: QueryClient; + + beforeEach(() => { + qc = new QueryClient(); + qc.setQueryData(["prs", "github"], [basePr()]); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it("merges successfully and shows success toast", async () => { + mergePrMock.mockResolvedValue({ ok: true }); + await mergePr(qc, target); + expect(mergePrMock).toHaveBeenCalledWith("github", "o/r", 42); + expect(toggleAutoMergeMock).not.toHaveBeenCalled(); + expect(toastSuccess).toHaveBeenCalledWith("PR merged"); + expect(toastError).not.toHaveBeenCalled(); + }); + + it("falls back to arming auto-merge when direct merge fails", async () => { + mergePrMock.mockRejectedValue( + new Error( + "Repository rule violations found — A conversation must be resolved", + ), + ); + toggleAutoMergeMock.mockResolvedValue({ ok: true, autoMerge: true }); + + await mergePr(qc, target); + + expect(toggleAutoMergeMock).toHaveBeenCalledWith("github", "o/r", 42); + expect(toastSuccess).toHaveBeenCalledWith( + expect.stringContaining("Auto-merge enabled"), + ); + expect(toastError).not.toHaveBeenCalled(); + + // PR is restored to the cache and now flagged autoMerge=true + const prs = qc.getQueryData(["prs", "github"]); + expect(prs).toHaveLength(1); + expect(prs?.[0]?.autoMerge).toBe(true); + }); + + it("surfaces the original merge error when repo doesn't allow auto-merge", async () => { + const mergeErr = new Error( + "Repository rule violations found — required reviews missing", + ); + mergePrMock.mockRejectedValue(mergeErr); + toggleAutoMergeMock.mockRejectedValue(new AutoMergeNotAllowedError()); + + await expect(mergePr(qc, target)).rejects.toThrow(mergeErr); + + expect(toggleAutoMergeMock).toHaveBeenCalledWith("github", "o/r", 42); + expect(toastError).toHaveBeenCalledWith(mergeErr.message); + expect(toastSuccess).not.toHaveBeenCalled(); + + // PR is restored to the cache, autoMerge unchanged + const prs = qc.getQueryData(["prs", "github"]); + expect(prs).toHaveLength(1); + expect(prs?.[0]?.autoMerge).toBe(false); + }); +}); diff --git a/packages/web/src/mutations.ts b/packages/web/src/mutations.ts index 8ee97be..1fb752c 100644 --- a/packages/web/src/mutations.ts +++ b/packages/web/src/mutations.ts @@ -101,7 +101,28 @@ export async function mergePr( restore(qc, prsSnap); restore(qc, reviewsSnap); restore(qc, recentSnap); - toast.error(err instanceof Error ? err.message : "Failed to merge PR"); + + // mergeStateStatus="CLEAN" can lie when repo rulesets impose extra + // requirements GraphQL doesn't reflect. Always attempt to arm + // auto-merge as a fallback — the server returns AutoMergeNotAllowedError + // when the repo really disallows it (more reliable than the cached + // autoMergeAllowed flag, which can be stale). + const reason = err instanceof Error ? err.message : "Failed to merge PR"; + try { + await api.toggleAutoMerge(target.instanceId, target.repo, target.number); + qc.setQueriesData({ queryKey: ["prs"] }, (old) => + old?.map((pr) => + matches(target)(pr) ? { ...pr, autoMerge: true } : pr, + ), + ); + toast.success(`Auto-merge enabled — direct merge blocked: ${reason}`); + return; + } catch { + // Fallback failed (repo doesn't allow auto-merge, or some other + // error) — surface the original merge error since that's what the + // user tried to do. + } + toast.error(reason); throw err; } }