Skip to content

Commit 26f4458

Browse files
committed
fix(extension): guard overlay refresh against out-of-order pull-context responses
mountOverlay's refresh button called load() with no request-ordering guard, so two rapid clicks issued two independent pull-context round-trips and whichever resolved last won. Under network jitter the older click's response could resolve after the newer one, silently leaving the overlay showing stale data. Move load() into a per-overlay-instance createOverlayLoader closure that stamps each invocation with a monotonically incrementing ticket and discards any resolved response whose ticket is no longer the latest, so the rendered state always reflects the most recently initiated request. Single-click behavior (loading text, error and success rendering) is unchanged. Closes #7463
1 parent 1370f24 commit 26f4458

2 files changed

Lines changed: 61 additions & 14 deletions

File tree

apps/loopover-extension/content.js

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ function mountOverlay(target) {
3838
} else {
3939
document.body.appendChild(container);
4040
}
41+
const load = createOverlayLoader(container, target);
4142
const refresh = container.querySelector(".loopover-overlay__refresh");
42-
refresh?.addEventListener("click", () => load(container, target));
43-
void load(container, target);
43+
refresh?.addEventListener("click", () => load());
44+
void load();
4445
}
4546

4647
function findPullRequestSidebar() {
@@ -52,17 +53,23 @@ function findPullRequestSidebar() {
5253
);
5354
}
5455

55-
async function load(container, target) {
56-
const body = container.querySelector(".loopover-overlay__body");
57-
if (!body) return;
58-
body.textContent = "Loading private context...";
59-
const response = await chrome.runtime.sendMessage({ type: "loopover:pull-context", ...target });
60-
if (!response?.ok) {
61-
body.innerHTML = `<div class="loopover-overlay__error">${escapeHtml(response?.error || "Context unavailable")}</div>`;
62-
return;
63-
}
64-
body.innerHTML = renderPullContext(response.payload);
65-
renderActions(body, response.payload?.actions);
56+
function createOverlayLoader(container, target) {
57+
let latestTicket = 0;
58+
return async function load() {
59+
const body = container.querySelector(".loopover-overlay__body");
60+
if (!body) return;
61+
const ticket = ++latestTicket;
62+
body.textContent = "Loading private context...";
63+
const response = await chrome.runtime.sendMessage({ type: "loopover:pull-context", ...target });
64+
// A newer load() has started while this request was in flight; discard the stale response.
65+
if (ticket !== latestTicket) return;
66+
if (!response?.ok) {
67+
body.innerHTML = `<div class="loopover-overlay__error">${escapeHtml(response?.error || "Context unavailable")}</div>`;
68+
return;
69+
}
70+
body.innerHTML = renderPullContext(response.payload);
71+
renderActions(body, response.payload?.actions);
72+
};
6673
}
6774

6875
function renderPullContext(payload) {
@@ -185,6 +192,7 @@ if (globalThis.__LOOPOVER_EXTENSION_TEST__) {
185192
globalThis.__loopoverContentInternals = {
186193
matchGitHubPageTarget,
187194
matchPullRequestTarget,
195+
createOverlayLoader,
188196
renderPullContext,
189197
renderSection,
190198
renderLegacyPanels,

test/unit/extension-content.test.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,46 @@ describe("extension content script", () => {
7070
expect(html).toContain("public");
7171
expect(html).toContain("no");
7272
});
73+
74+
it("discards an out-of-order refresh response so the overlay keeps the newest request's payload", async () => {
75+
const resolvers: Array<(response: unknown) => void> = [];
76+
const sendMessage = vi.fn(() => new Promise((resolve) => resolvers.push(resolve)));
77+
const internals = loadContentInternals({
78+
chrome: { runtime: { sendMessage } },
79+
});
80+
81+
const body = { innerHTML: "", textContent: "" };
82+
const container = { querySelector: vi.fn(() => body) };
83+
const load = internals.createOverlayLoader(container, {
84+
kind: "pull_request",
85+
owner: "JSONbored",
86+
repo: "loopover",
87+
pullNumber: 42,
88+
});
89+
90+
// Two rapid refresh clicks: the second (newer) request is issued while the first is still in flight.
91+
const first = load();
92+
const second = load();
93+
expect(sendMessage).toHaveBeenCalledTimes(2);
94+
const [resolveFirst, resolveSecond] = resolvers;
95+
if (!resolveFirst || !resolveSecond) throw new Error("expected two in-flight pull-context requests");
96+
97+
// The newer request resolves and renders first...
98+
resolveSecond({ ok: true, payload: { sections: [{ label: "Newer context" }] } });
99+
await second;
100+
expect(body.innerHTML).toContain("Newer context");
101+
102+
// ...then the older, first-issued request resolves last — the out-of-order case. Its stale
103+
// payload must be discarded rather than clobbering the fresher render already on screen.
104+
resolveFirst({ ok: true, payload: { sections: [{ label: "Stale context" }] } });
105+
await first;
106+
107+
expect(body.innerHTML).toContain("Newer context");
108+
expect(body.innerHTML).not.toContain("Stale context");
109+
});
73110
});
74111

75-
function loadContentInternals() {
112+
function loadContentInternals(overrides: Record<string, unknown> = {}) {
76113
const context: Record<string, unknown> = {
77114
__LOOPOVER_EXTENSION_TEST__: true,
78115
location: { pathname: "/JSONbored/loopover/issues/146" },
@@ -84,6 +121,7 @@ function loadContentInternals() {
84121
body: { appendChild: vi.fn() },
85122
},
86123
chrome: { runtime: { sendMessage: vi.fn() } },
124+
...overrides,
87125
};
88126
context.globalThis = context;
89127
const vmContext = createContext(context);
@@ -93,6 +131,7 @@ function loadContentInternals() {
93131
pathname: string,
94132
) => { kind: "pull_request"; owner: string; repo: string; pullNumber: number } | { kind: "issue"; owner: string; repo: string; issueNumber: number } | null;
95133
matchPullRequestTarget: (pathname: string) => { owner: string; repo: string; pullNumber: number } | null;
134+
createOverlayLoader: (container: { querySelector: (selector: string) => unknown }, target: unknown) => () => Promise<void>;
96135
renderPullContext: (payload: unknown) => string;
97136
};
98137
}

0 commit comments

Comments
 (0)