diff --git a/src/__tests__/mergify.test.js b/src/__tests__/mergify.test.js index fb375fd..ea745f7 100644 --- a/src/__tests__/mergify.test.js +++ b/src/__tests__/mergify.test.js @@ -1276,6 +1276,35 @@ describe("gatherPrStatuses", () => { await gatherPrStatuses(items, cache, () => {}, 3); expect(peak).toBeLessThanOrEqual(3); }); + + it("dedupes concurrent fetches for the same PR (org/repo/num/head_sha)", async () => { + const cache = new PrStatusCache(); + let resolveFetch; + let fetchCallCount = 0; + global.fetch = jest.fn(() => { + fetchCallCount += 1; + return new Promise((res) => { + resolveFetch = () => + res({ + ok: true, + text: () => + Promise.resolve( + '', + ), + }); + }); + }); + const items = [{ org: "o", repo: "r", num: 1, head_sha: "h" }]; + const p1 = gatherPrStatuses(items, cache, () => {}); + const p2 = gatherPrStatuses(items, cache, () => {}); + await Promise.resolve(); + await Promise.resolve(); + // Two concurrent gathers for the same PR → only one network fetch. + expect(fetchCallCount).toBe(1); + resolveFetch(); + await p1; + await p2; + }); }); describe("buildContextPanel", () => { @@ -2052,6 +2081,93 @@ describe("renderMergifyContext", () => { expect(document.querySelector("#mergify-context")).toBeNull(); }); + it("backs off after an edit_form failure (negative cache)", async () => { + document.body.innerHTML = + '
' + + '
' + + '
' + + '
Mergify stack
' + + "
"; + const fetchSpy = jest.fn(() => + Promise.resolve({ ok: false, status: 404 }), + ); + global.fetch = fetchSpy; + await renderMergifyContext({ org: "o", repo: "r", number: 122 }); + expect(fetchSpy).toHaveBeenCalledTimes(1); + await renderMergifyContext({ org: "o", repo: "r", number: 122 }); + // Second tick: the failure is cached, no extra fetch fired. + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + + it("removes a stale panel/nav when bodies become empty", async () => { + const stackPayload = { + schema_version: 1, + stack_id: "x", + pulls: [ + { + number: 122, + change_id: "i1", + head_sha: "h1", + base_branch: "main", + dest_branch: "x", + is_current: true, + }, + { + number: 123, + change_id: "i2", + head_sha: "h2", + base_branch: "x", + dest_branch: "x", + is_current: false, + }, + ], + }; + const stackBody = + "Stack:\n" + + "| 0 | T1 | [#122](https://x/122) | 👈 |\n" + + "| 1 | T2 | [#123](https://x/123) | |\n" + + ``; + document.body.innerHTML = + '
' + + '
' + + '
' + + '
' + + '
Mergify stack
' + + "
"; + const escaped = stackBody + .replace(/&/g, "&") + .replace(//g, ">"); + global.fetch = jest.fn((url) => { + if ( + typeof url === "string" && + /\/issue_comments\/\d+\/edit_form$/.test(url) + ) { + return Promise.resolve({ + ok: true, + text: () => + Promise.resolve( + ``, + ), + }); + } + return Promise.resolve({ + ok: true, + text: () => Promise.resolve(""), + }); + }); + await renderMergifyContext({ org: "o", repo: "r", number: 122 }); + expect(document.querySelector("#mergify-context")).not.toBeNull(); + expect(document.querySelector("#mergify-stack-nav")).not.toBeNull(); + // Now the comments disappear. + for (const el of document.querySelectorAll(".TimelineItem")) { + el.remove(); + } + await renderMergifyContext({ org: "o", repo: "r", number: 122 }); + expect(document.querySelector("#mergify-context")).toBeNull(); + expect(document.querySelector("#mergify-stack-nav")).toBeNull(); + }); + it("resetQueueState removes any existing #mergify-context panel", () => { document.body.innerHTML = '
'; @@ -2324,6 +2440,42 @@ describe("injectStackNav", () => { expect(document.querySelector("#mergify-stack-nav")).toBeNull(); }); + it("resets toolbar overrides when null even if our nav is already gone", () => { + // React may have replaced the sticky element after we tweaked its + // inline styles: the nav node is gone but flexWrap and padding-bottom + // we set linger. injectStackNav(null, ...) must restore them. + document.body.innerHTML = + '
'; + const sticky = document.querySelector( + '[class*="use-sticky-header-module__stickyHeader"]', + ); + sticky.style.flexWrap = "wrap"; + sticky.style.setProperty("padding-bottom", "0", "important"); + injectStackNav(null, { org: "o", repo: "r", number: 1 }); + expect(sticky.style.flexWrap).toBe(""); + expect(sticky.style.getPropertyValue("padding-bottom")).toBe(""); + }); + + it("injects a sticky-offset CSS rule and removes it on null", () => { + document.body.innerHTML = + '
'; + const stack = { + schema_version: 1, + stack_id: "x", + pulls: [pull(1, { is_current: true }), pull(2)], + }; + injectStackNav(stack, { org: "o", repo: "r", number: 1 }); + const style = document.getElementById("mergify-sticky-offset-fix"); + expect(style).not.toBeNull(); + // Rule targets GitHub's lower sticky pane and has !important. + expect(style.textContent).toMatch( + /\[class\*="DiffComparisonViewer-module__Pane"\]\s*\{\s*top:\s*\d+px\s*!important;\s*\}/, + ); + // Removing the nav removes the style. + injectStackNav(null, { org: "o", repo: "r", number: 1 }); + expect(document.getElementById("mergify-sticky-offset-fix")).toBeNull(); + }); + it("no-ops when no sticky target is found", () => { document.body.innerHTML = "
"; const stack = { diff --git a/src/stacks.js b/src/stacks.js index 69cd418..89a444d 100644 --- a/src/stacks.js +++ b/src/stacks.js @@ -180,18 +180,24 @@ export async function fetchCommentBodies(org, repo, _prNumber) { while (queue.length > 0) { const id = queue.shift(); const cached = _commentBodyCache.get(id); - if ( - cached && - Date.now() - cached.timestamp < COMMENTS_CACHE_TTL_MS - ) { - bodies.push(cached.body); - continue; + if (cached) { + const age = Date.now() - cached.timestamp; + if (cached.body && age < COMMENTS_CACHE_TTL_MS) { + bodies.push(cached.body); + continue; + } + // Negative cache: a recent failure is remembered for a short + // back-off window so we don't refetch on every tryInject tick. + if (!cached.body && age < COMMENTS_NEGATIVE_CACHE_TTL_MS) { + continue; + } } const body = await fetchCommentBodyMarkdown(org, repo, id); - if (body) { - _commentBodyCache.set(id, { body, timestamp: Date.now() }); - bodies.push(body); - } + _commentBodyCache.set(id, { + body: body || null, + timestamp: Date.now(), + }); + if (body) bodies.push(body); } }); await Promise.all(workers); @@ -214,6 +220,10 @@ export async function fetchPrStatus(org, repo, num) { } } +function _statusKey(item) { + return `${item.org}/${item.repo}/${item.num}/${item.head_sha}`; +} + export async function gatherPrStatuses( items, cache, @@ -235,7 +245,17 @@ export async function gatherPrStatuses( onResolve(item, cached); continue; } - const status = await fetchPrStatus(item.org, item.repo, item.num); + // Dedupe concurrent fetches for the same PR — multiple tryInject + // ticks during React reconciliation can otherwise spawn the same + // request many times before the first response lands. + const key = _statusKey(item); + let promise = _inflightStatusFetches.get(key); + if (!promise) { + promise = fetchPrStatus(item.org, item.repo, item.num); + _inflightStatusFetches.set(key, promise); + promise.finally(() => _inflightStatusFetches.delete(key)); + } + const status = await promise; // Don't cache "unknown" — it usually means a transient fetch // failure, and we want the next tick to retry rather than serve // gray for the full TTL. @@ -640,6 +660,11 @@ export function injectContextPanel(panel) { target.insertBefore(panel, target.firstChild); } +export function removeContextSurfaces(currentPull) { + document.querySelector("#mergify-context")?.remove(); + injectStackNav(null, currentPull); +} + export async function renderMergifyContext(currentPull) { const generation = ++_contextRenderGeneration; const bodies = await fetchCommentBodies( @@ -648,12 +673,18 @@ export async function renderMergifyContext(currentPull) { currentPull.number, ); if (generation !== _contextRenderGeneration) return; - if (bodies.length === 0) return; + if (bodies.length === 0) { + removeContextSurfaces(currentPull); + return; + } const stackData = parseStackMarker(bodies, currentPull.number); const revisionData = parseRevisionMarker(bodies, currentPull.number); const panel = buildContextPanel(stackData, revisionData, currentPull); - if (!panel) return; + if (!panel) { + removeContextSurfaces(currentPull); + return; + } injectContextPanel(panel); injectStackNav(stackData, currentPull); @@ -805,17 +836,43 @@ export function buildStackNav(stackData, currentPull) { return root; } +// GitHub's diff-view file-tree pane is a separate `position: sticky` element +// with a hardcoded `top: 60px` (the toolbar's original height). Once we grow +// the toolbar with our nav row, the pane sticks UNDER our nav and disappears. +// Override its top via a `