Skip to content

fix(extension): resolve sticky-offset overlap and apply missed PR-feedback fixes #400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jd wants to merge 1 commit into from
Closed

fix(extension): resolve sticky-offset overlap and apply missed PR-feedback fixes #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions src/__tests__/mergify.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
'<span data-status="pullOpened"></span>',
),
});
});
});
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", () => {
Expand Down Expand Up @@ -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 =
'<div id="discussion_bucket"></div>' +
'<div class="TimelineItem">' +
'<div id="issuecomment-1"></div>' +
'<div class="comment-body">Mergify stack</div>' +
"</div>";
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" +
`<!-- mergify-stack-data: ${JSON.stringify(stackPayload)} -->`;
document.body.innerHTML =
'<div id="discussion_bucket"></div>' +
'<section class="use-sticky-header-module__stickyHeader__abc PullRequestFilesToolbar-module__toolbar__def"></section>' +
'<div class="TimelineItem">' +
'<div id="issuecomment-1"></div>' +
'<div class="comment-body">Mergify stack</div>' +
"</div>";
const escaped = stackBody
.replace(/&/g, "&amp;")
.replace(/</g, "&lt;")
.replace(/>/g, "&gt;");
global.fetch = jest.fn((url) => {
if (
typeof url === "string" &&
/\/issue_comments\/\d+\/edit_form$/.test(url)
) {
return Promise.resolve({
ok: true,
text: () =>
Promise.resolve(
`<html><body><textarea>${escaped}</textarea></body></html>`,
),
});
}
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 =
'<div id="discussion_bucket"><div id="mergify-context"></div></div>';
Expand Down Expand Up @@ -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 =
'<section class="use-sticky-header-module__stickyHeader__abc"></section>';
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 =
'<section class="use-sticky-header-module__stickyHeader__abc PullRequestFilesToolbar-module__toolbar__def"></section>';
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 = "<div></div>";
const stack = {
Expand Down
101 changes: 78 additions & 23 deletions src/stacks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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(
Expand All @@ -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);

Expand Down Expand Up @@ -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 `<style>` rule whose value tracks the toolbar's
// actual current height. Selector is class-substring so it survives the
// random suffix Primer's CSS modules append.
const STICKY_OFFSET_STYLE_ID = "mergify-sticky-offset-fix";
const STICKY_OFFSET_SELECTOR = '[class*="DiffComparisonViewer-module__Pane"]';

function updateStickyOffsetFix(target) {
const newTop = Math.max(0, target.offsetHeight - 1);
let style = document.getElementById(STICKY_OFFSET_STYLE_ID);
if (!style) {
style = document.createElement("style");
style.id = STICKY_OFFSET_STYLE_ID;
document.head.appendChild(style);
}
style.textContent = `${STICKY_OFFSET_SELECTOR} { top: ${newTop}px !important; }`;
}

function clearStickyOffsetFix() {
document.getElementById(STICKY_OFFSET_STYLE_ID)?.remove();
}

export function injectStackNav(stackData, currentPull) {
const target = findStackNavTarget();
if (!target) return;
const existing = document.querySelector("#mergify-stack-nav");
const fresh = buildStackNav(stackData, currentPull);
if (!fresh) {
if (existing) {
existing.remove();
target.style.flexWrap = "";
target.style.removeProperty("padding-bottom");
}
// Always reset our toolbar overrides — React may have replaced the
// sticky element while our nav node was the only thing left holding
// the override identity. Run unconditionally.
target.style.flexWrap = "";
target.style.removeProperty("padding-bottom");
if (existing) existing.remove();
clearStickyOffsetFix();
return;
}
// The toolbar is a flex-row. Force flex-wrap so our 100%-width child
Expand All @@ -826,11 +883,9 @@ export function injectStackNav(stackData, currentPull) {
// the bottom edge of the sticky bar.
target.style.flexWrap = "wrap";
target.style.setProperty("padding-bottom", "0", "important");
if (existing) {
existing.replaceWith(fresh);
return;
}
target.appendChild(fresh);
if (existing) existing.replaceWith(fresh);
else target.appendChild(fresh);
updateStickyOffsetFix(target);
}

export function resetStackState() {
Expand Down