diff --git a/packages/server/src/cache.ts b/packages/server/src/cache.ts index dd140ee..f29644a 100644 --- a/packages/server/src/cache.ts +++ b/packages/server/src/cache.ts @@ -55,6 +55,10 @@ export function setCached(key: string, data: unknown) { persistCache(); } +export function patchCache(key: string, fn: (data: T | null) => T): void { + setCached(key, fn(getCached(key))); +} + export function cacheAge(key: string): number | null { const entry = store.get(key); if (!entry) return null; diff --git a/packages/server/src/routes.test.ts b/packages/server/src/routes.test.ts index 87c30df..921e8b6 100644 --- a/packages/server/src/routes.test.ts +++ b/packages/server/src/routes.test.ts @@ -54,6 +54,9 @@ vi.mock("./cache.js", () => ({ setCached: (key: string, data: unknown) => { cacheStore.set(key, data); }, + patchCache: (key: string, fn: (data: T | null) => T) => { + cacheStore.set(key, fn((cacheStore.get(key) ?? null) as T | null)); + }, })); vi.mock("./config.js", () => configStub); @@ -372,6 +375,41 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/merge", () => { "Repository rule violations found — A conversation must be resolved before this pull request can be merged.", }); }); + + it("removes the merged PR from cached prs/reviews even when GitHub still reports it as open", async () => { + cacheStore.set("github:prs", [ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + cacheStore.set("github:reviews", [ + { id: 11, repo: "o/r", number: 5, title: "x" }, + { id: 12, repo: "o/r", number: 7, title: "other" }, + ]); + cacheStore.set("github:recent-prs", []); + mockOctokit.pulls.merge.mockResolvedValue({}); + + // Simulate GitHub eventual consistency: search index still returns the PR. + fetchersStub.fetchPrs.mockResolvedValue([ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + fetchersStub.fetchReviews.mockResolvedValue([ + { id: 11, repo: "o/r", number: 5, title: "x" }, + { id: 12, repo: "o/r", number: 7, title: "other" }, + ]); + fetchersStub.fetchRecentPrs.mockResolvedValue([]); + + const res = await call("/github/prs/o/r/5/merge", { method: "POST" }); + expect(res.status).toBe(200); + await waitForPendingResyncs(); + + const prs = cacheStore.get("github:prs") as Array<{ number: number }>; + const reviews = cacheStore.get("github:reviews") as Array<{ + number: number; + }>; + expect(prs.map((p) => p.number)).toEqual([9]); + expect(reviews.map((r) => r.number)).toEqual([7]); + }); }); describe("POST /:instanceId/prs/:owner/:repo/:prNumber/close", () => { @@ -395,6 +433,38 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/close", () => { expect(fetchersStub.fetchReviews).toHaveBeenCalledWith("github"); expect(fetchersStub.fetchRecentPrs).toHaveBeenCalledWith("github"); }); + + it("removes the closed PR from cached prs/reviews even when GitHub still reports it as open", async () => { + cacheStore.set("github:prs", [ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + cacheStore.set("github:reviews", [ + { id: 11, repo: "o/r", number: 5, title: "x" }, + ]); + cacheStore.set("github:recent-prs", []); + mockOctokit.pulls.update.mockResolvedValue({}); + + fetchersStub.fetchPrs.mockResolvedValue([ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + fetchersStub.fetchReviews.mockResolvedValue([ + { id: 11, repo: "o/r", number: 5, title: "x" }, + ]); + fetchersStub.fetchRecentPrs.mockResolvedValue([]); + + const res = await call("/github/prs/o/r/5/close", { method: "POST" }); + expect(res.status).toBe(200); + await waitForPendingResyncs(); + + const prs = cacheStore.get("github:prs") as Array<{ number: number }>; + const reviews = cacheStore.get("github:reviews") as Array<{ + number: number; + }>; + expect(prs.map((p) => p.number)).toEqual([9]); + expect(reviews.map((r) => r.number)).toEqual([]); + }); }); describe("PATCH /:instanceId/prs/:owner/:repo/:prNumber", () => { @@ -466,6 +536,36 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/toggle-draft", () => { { id: "PR_1" }, ); }); + + it("flips draft on the cached PR even when GitHub still reports the old draft state", async () => { + cacheStore.set("github:prs", [ + { id: 1, repo: "o/r", number: 5, title: "x", draft: true }, + { id: 2, repo: "o/r", number: 9, title: "other", draft: false }, + ]); + mockOctokit.pulls.get.mockResolvedValue({ + data: { draft: true, node_id: "PR_1" }, + }); + mockOctokit.graphql.mockResolvedValue({}); + + // GitHub eventual consistency: still says draft=true. + fetchersStub.fetchPrs.mockResolvedValue([ + { id: 1, repo: "o/r", number: 5, title: "x", draft: true }, + { id: 2, repo: "o/r", number: 9, title: "other", draft: false }, + ]); + + const res = await call("/github/prs/o/r/5/toggle-draft", { + method: "POST", + }); + expect(res.status).toBe(200); + await waitForPendingResyncs(); + + const prs = cacheStore.get("github:prs") as Array<{ + number: number; + draft: boolean; + }>; + expect(prs.find((p) => p.number === 5)?.draft).toBe(false); + expect(prs.find((p) => p.number === 9)?.draft).toBe(false); + }); }); describe("POST /:instanceId/prs/:owner/:repo/:prNumber/rerun-ci", () => { diff --git a/packages/server/src/routes.ts b/packages/server/src/routes.ts index f38234f..701a99a 100644 --- a/packages/server/src/routes.ts +++ b/packages/server/src/routes.ts @@ -1,6 +1,6 @@ import { Hono } from "hono"; import { Octokit } from "@octokit/rest"; -import { getCached, setCached } from "./cache.js"; +import { getCached, patchCache, setCached } from "./cache.js"; import { type ConfigSchema, configExists, @@ -17,7 +17,27 @@ import { latestCheckRunsByName, } from "./fetchers.js"; import { clearClients, getClient, getInstance } from "./github-client.js"; -import { scheduleResync } from "./sync.js"; +import { recordMutation, scheduleResync } from "./sync.js"; + +interface CachedListItem { + repo: string; + number: number; + draft?: boolean; +} + +function removeFromList(repo: string, number: number) { + return (data: CachedListItem[] | null): CachedListItem[] => + (data ?? []).filter( + (item) => !(item.repo === repo && item.number === number), + ); +} + +function setDraftInList(repo: string, number: number, draft: boolean) { + return (data: CachedListItem[] | null): CachedListItem[] => + (data ?? []).map((item) => + item.repo === repo && item.number === number ? { ...item, draft } : item, + ); +} const api = new Hono(); @@ -279,13 +299,15 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/auto-merge", async (c) => { // Merge a PR directly (squash) api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { const { instanceId, owner, repo, prNumber } = c.req.param(); + const num = Number(prNumber); + const fullRepo = `${owner}/${repo}`; const client = await getClient(instanceId); try { await client.pulls.merge({ owner, repo, - pull_number: Number(prNumber), + pull_number: num, merge_method: "squash", }); } catch (err) { @@ -305,6 +327,9 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { return c.json({ error: "merge_rejected", message }, 422); } + patchCache(`${instanceId}:prs`, removeFromList(fullRepo, num)); + patchCache(`${instanceId}:reviews`, removeFromList(fullRepo, num)); + recordMutation(instanceId, { kind: "removed", repo: fullRepo, number: num }); scheduleResync(instanceId, ["prs", "reviews", "recent-prs"]); return c.json({ ok: true }); @@ -313,15 +338,20 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { // Close a PR api.post("/:instanceId/prs/:owner/:repo/:prNumber/close", async (c) => { const { instanceId, owner, repo, prNumber } = c.req.param(); + const num = Number(prNumber); + const fullRepo = `${owner}/${repo}`; const client = await getClient(instanceId); await client.pulls.update({ owner, repo, - pull_number: Number(prNumber), + pull_number: num, state: "closed", }); + patchCache(`${instanceId}:prs`, removeFromList(fullRepo, num)); + patchCache(`${instanceId}:reviews`, removeFromList(fullRepo, num)); + recordMutation(instanceId, { kind: "removed", repo: fullRepo, number: num }); scheduleResync(instanceId, ["prs", "reviews", "recent-prs"]); return c.json({ ok: true }); @@ -355,6 +385,7 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/toggle-draft", async (c) => { const { instanceId, owner, repo, prNumber } = c.req.param(); const client = await getClient(instanceId); const num = Number(prNumber); + const fullRepo = `${owner}/${repo}`; const { data: pr } = await client.pulls.get({ owner, @@ -369,9 +400,17 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/toggle-draft", async (c) => { await client.graphql(mutation, { id: pr.node_id }); + const newDraft = !pr.draft; + patchCache(`${instanceId}:prs`, setDraftInList(fullRepo, num, newDraft)); + recordMutation(instanceId, { + kind: "draft", + repo: fullRepo, + number: num, + draft: newDraft, + }); scheduleResync(instanceId, ["prs"]); - return c.json({ ok: true, draft: !pr.draft }); + return c.json({ ok: true, draft: newDraft }); }); // Rerun CI for a PR diff --git a/packages/server/src/sync.ts b/packages/server/src/sync.ts index 1d2b41f..349b76d 100644 --- a/packages/server/src/sync.ts +++ b/packages/server/src/sync.ts @@ -25,6 +25,88 @@ const ALL_KEYS: ResyncKey[] = ["prs", "recent-prs", "reviews", "notifications"]; const pending = new Set>(); +// Tracks recent client-driven mutations so a stale resync (GitHub's search +// index lags by seconds after a merge/close/draft toggle) doesn't re-introduce +// the old state. Entries expire after MUTATION_TTL. +type MutationRecord = + | { kind: "removed"; repo: string; number: number; expiresAt: number } + | { + kind: "draft"; + repo: string; + number: number; + draft: boolean; + expiresAt: number; + }; + +const MUTATION_TTL = 60_000; +const mutations = new Map(); + +function mutationKey(instanceId: string, repo: string, number: number) { + return `${instanceId}:${repo}:${number}`; +} + +export function recordMutation( + instanceId: string, + m: + | { kind: "removed"; repo: string; number: number } + | { kind: "draft"; repo: string; number: number; draft: boolean }, +): void { + mutations.set(mutationKey(instanceId, m.repo, m.number), { + ...m, + expiresAt: Date.now() + MUTATION_TTL, + }); +} + +function activeMutations(instanceId: string): MutationRecord[] { + const now = Date.now(); + const out: MutationRecord[] = []; + for (const [k, v] of mutations) { + if (v.expiresAt <= now) { + mutations.delete(k); + continue; + } + if (k.startsWith(`${instanceId}:`)) out.push(v); + } + return out; +} + +interface ListItem { + repo: string; + number: number; + draft?: boolean; +} + +function applyMutations( + instanceId: string, + key: ResyncKey, + data: unknown, +): unknown { + if (key !== "prs" && key !== "reviews") return data; + const muts = activeMutations(instanceId); + if (muts.length === 0) return data; + const items = data as ListItem[]; + + const filtered = items.filter( + (item) => + !muts.some( + (m) => + m.kind === "removed" && + m.repo === item.repo && + m.number === item.number, + ), + ); + + if (key === "reviews") return filtered; + + return filtered.map((item) => { + const draftMut = muts.find( + (m): m is Extract => + m.kind === "draft" && m.repo === item.repo && m.number === item.number, + ); + return draftMut ? { ...item, draft: draftMut.draft } : item; + }); +} + export async function resyncInstance( instanceId: string, keys: ResyncKey[] = ALL_KEYS, @@ -33,7 +115,10 @@ export async function resyncInstance( keys.map(async (key) => { try { const data = await RESYNC_FETCHERS[key](instanceId); - setCached(`${instanceId}:${key}`, data); + setCached( + `${instanceId}:${key}`, + applyMutations(instanceId, key, data), + ); } catch (err) { console.error( `Sync failed for ${instanceId}:${key}:`,