From 2c7b5c9c3f29e773094a3a13990f36bd63828d4e Mon Sep 17 00:00:00 2001 From: shawn Date: Wed, 3 Jun 2026 11:05:37 +0800 Subject: [PATCH 1/2] fix: stop native tooltips overlapping custom popovers The pr-review-status badge set a native `title` and also opened a click popover; on hover the browser tooltip overlapped the open popover. The star/fork/watch counters carry GitHub's own `title` (the exact count), which overlapped our hover popup. - pr-review-status: drop the `title` on badges that have a popover (the popover is the richer summary). Keep the `title` only on badges with no expandable details. - watch-fork-star: stash and remove the counter's native `title` on mouseenter, restore on mouseleave / cleanup. Our popup opens before the browser's title delay, so the native tooltip never shows. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/features/pr-review-status.test.ts | 4 +++ src/features/pr-review-status.ts | 6 ++++- src/features/watch-fork-star-popup.test.ts | 28 ++++++++++++++++++++ src/features/watch-fork-star-popup.ts | 30 ++++++++++++++++++++++ 4 files changed, 67 insertions(+), 1 deletion(-) diff --git a/src/features/pr-review-status.test.ts b/src/features/pr-review-status.test.ts index 710a96a..b6ef2ae 100644 --- a/src/features/pr-review-status.test.ts +++ b/src/features/pr-review-status.test.ts @@ -78,6 +78,10 @@ describe("injectPRReviewStatus", () => { const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; expect(badge.classList.contains("better-github-review-has-popover")).toBe(true); + // The popover supersedes the native tooltip — a badge with a popover must not + // also carry a `title`, or the browser tooltip overlaps the open popover. + expect(badge.hasAttribute("title")).toBe(false); + const items = badge.querySelectorAll(".better-github-review-popover-item"); expect(items).toHaveLength(2); diff --git a/src/features/pr-review-status.ts b/src/features/pr-review-status.ts index 2eae4f3..be61fac 100644 --- a/src/features/pr-review-status.ts +++ b/src/features/pr-review-status.ts @@ -206,13 +206,17 @@ export async function injectPRReviewStatus(): Promise { } else { badge.classList.add("better-github-review-unresolved"); badge.textContent = `${unresolved} unresolved`; - badge.title = `${status.resolvedThreads}/${status.totalThreads} review thread(s) resolved`; const details = status.unresolved ?? []; if (details.length > 0) { + // The click popover is the richer summary; a native `title` on the same + // badge would just hover-overlap the open popover, so we drop it here. badge.classList.add("better-github-review-has-popover"); badge.appendChild(buildPopover(details)); makeBadgeInteractive(badge); + } else { + // No expandable details → the native tooltip is the only summary, keep it. + badge.title = `${status.resolvedThreads}/${status.totalThreads} review thread(s) resolved`; } } diff --git a/src/features/watch-fork-star-popup.test.ts b/src/features/watch-fork-star-popup.test.ts index 8e0c5ea..628c62c 100644 --- a/src/features/watch-fork-star-popup.test.ts +++ b/src/features/watch-fork-star-popup.test.ts @@ -63,6 +63,34 @@ describe("watch/fork/star popup", () => { vi.useRealTimers(); }); + it("hides the counter's native title on hover and restores it on leave", () => { + injectWatchForkStarPopup(); + const star = document.getElementById("star-counter")!; + // GitHub ships the exact count as a native `title` on the counter span. + star.setAttribute("title", "1,234"); + + // Hover → native tooltip suppressed so it can't overlap our popup. + star.dispatchEvent(new Event("mouseenter")); + expect(star.hasAttribute("title")).toBe(false); + + // Leave → the native title comes back untouched. + star.dispatchEvent(new Event("mouseleave")); + expect(star.getAttribute("title")).toBe("1,234"); + }); + + it("restores a mid-hover stashed title on cleanup", () => { + injectWatchForkStarPopup(); + const star = document.getElementById("star-counter")!; + star.setAttribute("title", "1,234"); + + star.dispatchEvent(new Event("mouseenter")); + expect(star.hasAttribute("title")).toBe(false); + + // Tear down while still hovered — the title must not be lost. + cleanupWatchForkStarPopup(); + expect(star.getAttribute("title")).toBe("1,234"); + }); + afterEach(() => { vi.restoreAllMocks(); }); diff --git a/src/features/watch-fork-star-popup.ts b/src/features/watch-fork-star-popup.ts index b50a007..9249943 100644 --- a/src/features/watch-fork-star-popup.ts +++ b/src/features/watch-fork-star-popup.ts @@ -6,9 +6,31 @@ import type { ForkInfo } from "../lib/github-api"; const WRAP_CLASS = "bg-wfs-counter-wrap"; const POPUP_CLASS = "bg-wfs-popup"; +// GitHub puts the exact count in a native `title` on the counter span (e.g. +// title="1,234" behind a visible "1.2k"). Our popup attaches to that same span, +// so the browser tooltip would otherwise overlap it. We stash the title here +// while the pointer is over the counter and restore it on leave / cleanup. +const STASH_ATTR = "data-bg-wfs-title"; + const HOVER_OPEN_DELAY = 300; const HOVER_CLOSE_DELAY = 200; +/** Hide the counter's native `title` tooltip so it can't overlap our popup. */ +function suppressNativeTitle(wrap: HTMLElement): void { + const title = wrap.getAttribute("title"); + if (title === null) return; + wrap.setAttribute(STASH_ATTR, title); + wrap.removeAttribute("title"); +} + +/** Put the stashed native `title` back once the popup is no longer in play. */ +function restoreNativeTitle(wrap: HTMLElement): void { + const stashed = wrap.getAttribute(STASH_ATTR); + if (stashed === null) return; + wrap.setAttribute("title", stashed); + wrap.removeAttribute(STASH_ATTR); +} + interface PopupConfig { title: string; countText: string; @@ -116,10 +138,16 @@ function setupHover( } wrap.addEventListener("mouseenter", () => { + // Drop the native tooltip up front — our popup opens at HOVER_OPEN_DELAY, + // which beats the browser's title delay, so it never gets a chance to show. + suppressNativeTitle(wrap); openTimer = setTimeout(show, HOVER_OPEN_DELAY); }); wrap.addEventListener("mouseleave", () => { + // mouseleave only fires once the pointer leaves the counter *and* the popup + // (a descendant), so by here the popup is dismissed and the title is safe. + restoreNativeTitle(wrap); hide(); }); @@ -225,6 +253,8 @@ export function injectWatchForkStarPopup(): void { export function cleanupWatchForkStarPopup(): void { for (const counter of document.querySelectorAll(`.${WRAP_CLASS}`)) { counter.classList.remove(WRAP_CLASS); + // If we tear down mid-hover, give the counter its native title back. + restoreNativeTitle(counter); counter.querySelectorAll(`.${POPUP_CLASS}`).forEach((popup) => popup.remove()); } } From 30049f3b0f2458df36f7947e3bfacad4fb505aea Mon Sep 17 00:00:00 2001 From: shawn Date: Wed, 3 Jun 2026 12:08:38 +0800 Subject: [PATCH 2/2] fix: lazy-load review thread details and drop in-popover tooltips Address two review findings on the unresolved-review-thread popover: - The list-page query (FETCH_PR_REVIEW_STATUSES) now fetches counts only (totalCount + per-thread isResolved). The heavy path/author/body detail query runs lazily, one PR at a time, only when a badge's popover is first opened (new FETCH_PR_REVIEW_THREAD_DETAILS), capped at MAX_REVIEW_THREADS. The popover shows a loading state and degrades to a retryable "couldn't load" message on failure; the count badge no longer depends on the detail fetch, so a failed/slow detail query never blanks the row. - Popover rows now carry the full path on aria-label instead of a native title, so hovering a truncated path no longer spawns a browser tooltip over the custom popover. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/features/pr-review-status.test.ts | 140 +++++++++++++++++--------- src/features/pr-review-status.ts | 126 ++++++++++++++++++----- src/lib/github-api.ts | 21 +++- src/lib/messages.ts | 3 +- src/service-worker.test.ts | 74 +++++++++----- src/service-worker.ts | 65 +++++++++--- src/styles/content.css | 9 ++ 7 files changed, 323 insertions(+), 115 deletions(-) diff --git a/src/features/pr-review-status.test.ts b/src/features/pr-review-status.test.ts index b6ef2ae..a728d64 100644 --- a/src/features/pr-review-status.test.ts +++ b/src/features/pr-review-status.test.ts @@ -1,7 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { setUrl } from "../test-utils/url"; import { injectPRReviewStatus } from "./pr-review-status"; -import { fetchPRReviewStatuses } from "../lib/github-api"; +import { fetchPRReviewStatuses, fetchReviewThreadDetails } from "../lib/github-api"; // Replace the content<->worker bridge with auto-mocked vi.fn()s so the feature // can be driven with canned data, never touching chrome.runtime. @@ -16,6 +16,15 @@ function twoPRRows(): void { `; } +/** Open a badge's popover and wait for its lazy contents to settle. */ +async function openPopover(badge: HTMLElement): Promise { + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + // The detail fetch resolves on a microtask; let the popover body swap in. + await vi.waitFor(() => + expect(badge.querySelector(".better-github-review-popover-message, .better-github-review-popover-item")).not.toBeNull(), + ); +} + describe("injectPRReviewStatus", () => { beforeEach(() => { vi.clearAllMocks(); @@ -43,45 +52,69 @@ describe("injectPRReviewStatus", () => { expect(numbers).toContain(7); expect(numbers).toContain(8); - // All resolved → check-marked "All resolved" state. + // All resolved → check-marked "All resolved" state, with a simple tooltip + // (no popover, so no overlap risk). const badge7 = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; expect(badge7).not.toBeNull(); expect(badge7.classList.contains("better-github-review-resolved")).toBe(true); expect(badge7.classList.contains("better-github-review-unresolved")).toBe(false); expect(badge7.textContent).toBe("✓ All resolved"); expect(badge7.title).toBe("4 review thread(s), all resolved"); + expect(badge7.classList.contains("better-github-review-has-popover")).toBe(false); - // Partially resolved → count of the remaining unresolved threads (5 - 2 = 3). + // Partially resolved → count of the remaining unresolved threads (5 - 2 = 3), + // a click popover, and crucially NO native title to overlap that popover. const badge8 = document.querySelector("#issue_8 .better-github-review-status") as HTMLElement; expect(badge8.classList.contains("better-github-review-unresolved")).toBe(true); expect(badge8.classList.contains("better-github-review-resolved")).toBe(false); expect(badge8.textContent).toBe("3 unresolved"); - expect(badge8.title).toBe("2/5 review thread(s) resolved"); + expect(badge8.classList.contains("better-github-review-has-popover")).toBe(true); + expect(badge8.hasAttribute("title")).toBe(false); }); - it("builds a thread-detail popover on the unresolved badge", async () => { + it("does not fetch thread details until a badge is opened", async () => { twoPRRows(); vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ - { - number: 7, - totalThreads: 2, - resolvedThreads: 0, - unresolved: [ - { path: "src/lib/github-api.ts", line: 42, isOutdated: false, author: "alice", snippet: "Guard the null case before dereferencing this.", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, - { path: "README.md", line: null, isOutdated: true, author: "bob", snippet: "Stale wording here.", url: "https://github.com/owner/repo/pull/7#discussion_r2" }, - ], - }, + { number: 7, totalThreads: 2, resolvedThreads: 0 }, + ]); + vi.mocked(fetchReviewThreadDetails).mockResolvedValue([ + { path: "a.ts", line: 1, isOutdated: false, author: "alice", snippet: "fix", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, ]); await injectPRReviewStatus(); + // The list query ran, but the heavy per-PR detail query has NOT — it is lazy. + expect(fetchPRReviewStatuses).toHaveBeenCalledTimes(1); + expect(fetchReviewThreadDetails).not.toHaveBeenCalled(); + const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; - expect(badge.classList.contains("better-github-review-has-popover")).toBe(true); + await openPopover(badge); + // Opening the badge triggers exactly one detail fetch for that single PR. + expect(fetchReviewThreadDetails).toHaveBeenCalledTimes(1); + expect(vi.mocked(fetchReviewThreadDetails).mock.calls[0]).toEqual(["owner", "repo", 7]); + }); + + it("builds a thread-detail popover lazily when the unresolved badge opens", async () => { + twoPRRows(); + vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ + { number: 7, totalThreads: 2, resolvedThreads: 0 }, + ]); + vi.mocked(fetchReviewThreadDetails).mockResolvedValue([ + { path: "src/lib/github-api.ts", line: 42, isOutdated: false, author: "alice", snippet: "Guard the null case before dereferencing this.", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, + { path: "README.md", line: null, isOutdated: true, author: "bob", snippet: "Stale wording here.", url: "https://github.com/owner/repo/pull/7#discussion_r2" }, + ]); + + await injectPRReviewStatus(); + + const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; + expect(badge.classList.contains("better-github-review-has-popover")).toBe(true); // The popover supersedes the native tooltip — a badge with a popover must not // also carry a `title`, or the browser tooltip overlaps the open popover. expect(badge.hasAttribute("title")).toBe(false); + await openPopover(badge); + const items = badge.querySelectorAll(".better-github-review-popover-item"); expect(items).toHaveLength(2); @@ -91,7 +124,10 @@ describe("injectPRReviewStatus", () => { expect(first.getAttribute("href")).toBe("https://github.com/owner/repo/pull/7#discussion_r1"); const loc = first.querySelector(".better-github-review-popover-loc-text") as HTMLElement; expect(loc.textContent).toBe("github-api.ts:42"); - expect(loc.title).toBe("src/lib/github-api.ts:42"); // full path preserved in tooltip + // Full path lives on `aria-label`, NOT `title` — a native tooltip inside the + // popover is exactly the overlap bug this feature avoids. + expect(loc.getAttribute("aria-label")).toBe("src/lib/github-api.ts:42"); + expect(loc.hasAttribute("title")).toBe(false); expect(first.querySelector(".better-github-review-popover-body")?.textContent).toBe( "@alice: Guard the null case before dereferencing this.", ); @@ -103,17 +139,45 @@ describe("injectPRReviewStatus", () => { expect(second.querySelector(".better-github-review-popover-outdated")?.textContent).toBe("outdated"); }); + it("shows a retryable message when the detail fetch comes back empty", async () => { + twoPRRows(); + vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ + { number: 7, totalThreads: 2, resolvedThreads: 0 }, + ]); + // Empty result (a failed fetch swallows to []) → degraded popover, count intact. + vi.mocked(fetchReviewThreadDetails).mockResolvedValue([]); + + await injectPRReviewStatus(); + + const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; + await openPopover(badge); + + const message = badge.querySelector(".better-github-review-popover-message"); + expect(message?.textContent).toBe("Couldn't load thread details."); + expect(badge.querySelector(".better-github-review-popover-item")).toBeNull(); + // The count badge itself is unaffected by the detail failure. + expect(badge.textContent?.startsWith("2 unresolved")).toBe(true); + + // Re-opening retries the fetch (it stayed in the idle state after failure). + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); // close + vi.mocked(fetchReviewThreadDetails).mockResolvedValue([ + { path: "a.ts", line: 1, isOutdated: false, author: "alice", snippet: "fix", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, + ]); + await openPopover(badge); + + expect(fetchReviewThreadDetails).toHaveBeenCalledTimes(2); + await vi.waitFor(() => + expect(badge.querySelectorAll(".better-github-review-popover-item")).toHaveLength(1), + ); + }); + it("toggles the popover on click and dismisses it on an outside click / Escape", async () => { twoPRRows(); vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ - { - number: 7, - totalThreads: 1, - resolvedThreads: 0, - unresolved: [ - { path: "a.ts", line: 1, isOutdated: false, author: "alice", snippet: "fix", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, - ], - }, + { number: 7, totalThreads: 1, resolvedThreads: 0 }, + ]); + vi.mocked(fetchReviewThreadDetails).mockResolvedValue([ + { path: "a.ts", line: 1, isOutdated: false, author: "alice", snippet: "fix", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, ]); await injectPRReviewStatus(); @@ -147,20 +211,16 @@ describe("injectPRReviewStatus", () => { it("does not toggle the popover when a thread row inside it is clicked", async () => { twoPRRows(); vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ - { - number: 7, - totalThreads: 1, - resolvedThreads: 0, - unresolved: [ - { path: "a.ts", line: 1, isOutdated: false, author: "alice", snippet: "fix", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, - ], - }, + { number: 7, totalThreads: 1, resolvedThreads: 0 }, + ]); + vi.mocked(fetchReviewThreadDetails).mockResolvedValue([ + { path: "a.ts", line: 1, isOutdated: false, author: "alice", snippet: "fix", url: "https://github.com/owner/repo/pull/7#discussion_r1" }, ]); await injectPRReviewStatus(); const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; - badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + await openPopover(badge); expect(badge.classList.contains("better-github-review-popover-open")).toBe(true); // Clicking the link row must fall through to navigation, leaving the popover open. @@ -169,20 +229,6 @@ describe("injectPRReviewStatus", () => { expect(badge.classList.contains("better-github-review-popover-open")).toBe(true); }); - it("omits the popover when the API returned no unresolved details", async () => { - twoPRRows(); - vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ - { number: 7, totalThreads: 3, resolvedThreads: 1 }, // legacy payload, no `unresolved` - ]); - - await injectPRReviewStatus(); - - const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; - expect(badge.textContent).toBe("2 unresolved"); - expect(badge.classList.contains("better-github-review-has-popover")).toBe(false); - expect(badge.querySelector(".better-github-review-popover")).toBeNull(); - }); - it("renders no badge for a PR with zero review threads", async () => { twoPRRows(); vi.mocked(fetchPRReviewStatuses).mockResolvedValue([ diff --git a/src/features/pr-review-status.ts b/src/features/pr-review-status.ts index be61fac..9b343df 100644 --- a/src/features/pr-review-status.ts +++ b/src/features/pr-review-status.ts @@ -1,5 +1,5 @@ import { isPRListPage, getRepoInfo } from "../lib/page-detect"; -import { fetchPRReviewStatuses } from "../lib/github-api"; +import { fetchPRReviewStatuses, fetchReviewThreadDetails } from "../lib/github-api"; import type { ReviewThreadDetail } from "../lib/messages"; import { getOrCreateInfoRow } from "../lib/info-row"; @@ -31,8 +31,12 @@ function ensureGlobalListeners(): void { }); } -/** Wire a badge to open/close its popover on click (and keyboard). */ -function makeBadgeInteractive(badge: HTMLElement): void { +/** + * Wire a badge to open/close its popover on click (and keyboard). `onOpen` runs + * every time the popover transitions to open, so the caller can lazily load and + * cache its contents (and retry after a failed load). + */ +function makeBadgeInteractive(badge: HTMLElement, onOpen: () => void): void { badge.setAttribute("role", "button"); badge.setAttribute("tabindex", "0"); // "dialog" rather than "true"/"menu": the popover is a list of links, not a menu. @@ -44,6 +48,7 @@ function makeBadgeInteractive(badge: HTMLElement): void { closeAllPopovers(); badge.classList.toggle(OPEN_CLASS, willOpen); badge.setAttribute("aria-expanded", String(willOpen)); + if (willOpen) onOpen(); }; // Only react to events on the badge itself; events bubbling up from inside the @@ -79,13 +84,11 @@ function basename(path: string): string { } /** - * Build the click-to-open popover listing each unresolved thread. Styled after - * GitHub's own hovercard / checks dropdown: a caret, a bordered header, and tidy - * rows. Long text is tamed entirely in CSS (path → basename + ellipsis, body → - * 2-line clamp), so the popover keeps a fixed width no matter how verbose a - * comment is. Each row links to its first comment's `#discussion_r…` anchor. + * Empty popover shell: just the caret and the click-trap. Its body is filled in + * later (loading message → thread rows, or an error message) so the actual + * thread details can be fetched lazily on first open. */ -function buildPopover(threads: ReviewThreadDetail[]): HTMLElement { +function createPopoverShell(): HTMLElement { const popover = document.createElement("div"); popover.className = POPOVER_CLASS; @@ -93,11 +96,40 @@ function buildPopover(threads: ReviewThreadDetail[]): HTMLElement { // it stays open while a thread link navigates (and selecting text won't close it). popover.addEventListener("click", (e) => e.stopPropagation()); - // Upward caret pointing back at the badge. + // Upward caret pointing back at the badge — stays put across content swaps. const caret = document.createElement("span"); caret.className = "better-github-review-popover-caret"; popover.appendChild(caret); + return popover; +} + +/** Swap the popover body, preserving the caret (its first child). */ +function setPopoverBody(popover: HTMLElement, ...nodes: Node[]): void { + while (popover.childNodes.length > 1) { + popover.removeChild(popover.lastChild as ChildNode); + } + popover.append(...nodes); +} + +/** A single muted status line (loading / load-failed) inside the popover. */ +function buildMessage(text: string): HTMLElement { + const message = document.createElement("div"); + message.className = "better-github-review-popover-message"; + message.textContent = text; + return message; +} + +/** + * Build the rows listing each unresolved thread. Styled after GitHub's own + * hovercard / checks dropdown: a bordered header and tidy rows. Long text is + * tamed entirely in CSS (path → basename + ellipsis, body → 2-line clamp), so + * the popover keeps a fixed width no matter how verbose a comment is. Each row + * links to its first comment's `#discussion_r…` anchor. + */ +function buildThreadRows(threads: ReviewThreadDetail[]): Node[] { + const nodes: Node[] = []; + // Header row, like "3 unresolved threads" with a comment glyph. const header = document.createElement("div"); header.className = "better-github-review-popover-header"; @@ -107,7 +139,7 @@ function buildPopover(threads: ReviewThreadDetail[]): HTMLElement { const headerLabel = document.createElement("span"); headerLabel.textContent = `${threads.length} unresolved thread${threads.length === 1 ? "" : "s"}`; header.append(icon, headerLabel); - popover.appendChild(header); + nodes.push(header); for (const t of threads.slice(0, MAX_POPOVER_ITEMS)) { // An so clicking jumps straight to the thread; falls back to a div when @@ -124,7 +156,9 @@ function buildPopover(threads: ReviewThreadDetail[]): HTMLElement { const locText = document.createElement("span"); locText.className = "better-github-review-popover-loc-text"; locText.textContent = t.line != null ? `${file}:${t.line}` : file; - if (t.path) locText.title = t.line != null ? `${t.path}:${t.line}` : t.path; + // Full path goes on `aria-label`, not `title`: a native tooltip here would + // hover-overlap the very popover this feature is meant to keep clean. + if (t.path) locText.setAttribute("aria-label", t.line != null ? `${t.path}:${t.line}` : t.path); head.appendChild(locText); if (t.isOutdated) { const tag = document.createElement("span"); @@ -141,17 +175,63 @@ function buildPopover(threads: ReviewThreadDetail[]): HTMLElement { item.appendChild(head); item.appendChild(body); - popover.appendChild(item); + nodes.push(item); } if (threads.length > MAX_POPOVER_ITEMS) { const more = document.createElement("div"); more.className = "better-github-review-popover-more"; more.textContent = `+${threads.length - MAX_POPOVER_ITEMS} more`; - popover.appendChild(more); + nodes.push(more); } - return popover; + return nodes; +} + +/** + * Attach a lazily-loaded thread popover to an unresolved badge. The heavy + * detail query runs only when the badge is first opened; a failed or empty + * load shows a "couldn't load" message and is retried on the next open (the + * count on the badge itself never depended on this fetch). + */ +function setupPopover( + badge: HTMLElement, + owner: string, + repo: string, + prNumber: number, +): void { + badge.classList.add("better-github-review-has-popover"); + + const popover = createPopoverShell(); + badge.appendChild(popover); + + let state: "idle" | "loading" | "loaded" = "idle"; + const load = async () => { + if (state !== "idle") return; // in flight or already populated + state = "loading"; + setPopoverBody(popover, buildMessage("Loading…")); + + let details: ReviewThreadDetail[] = []; + try { + details = (await fetchReviewThreadDetails(owner, repo, prNumber)) ?? []; + } catch { + details = []; + } + + // We only attach popovers to badges that already counted ≥1 unresolved + // thread, so an empty result means the detail fetch failed (or the threads + // were resolved since the list query). Either way, let a later click retry. + if (details.length === 0) { + setPopoverBody(popover, buildMessage("Couldn't load thread details.")); + state = "idle"; + return; + } + + setPopoverBody(popover, ...buildThreadRows(details)); + state = "loaded"; + }; + + makeBadgeInteractive(badge, load); } export async function injectPRReviewStatus(): Promise { @@ -206,18 +286,10 @@ export async function injectPRReviewStatus(): Promise { } else { badge.classList.add("better-github-review-unresolved"); badge.textContent = `${unresolved} unresolved`; - - const details = status.unresolved ?? []; - if (details.length > 0) { - // The click popover is the richer summary; a native `title` on the same - // badge would just hover-overlap the open popover, so we drop it here. - badge.classList.add("better-github-review-has-popover"); - badge.appendChild(buildPopover(details)); - makeBadgeInteractive(badge); - } else { - // No expandable details → the native tooltip is the only summary, keep it. - badge.title = `${status.resolvedThreads}/${status.totalThreads} review thread(s) resolved`; - } + // The click popover is the richer summary and is loaded lazily on open; + // we deliberately set no native `title` here, as it would hover-overlap + // the open popover. + setupPopover(badge, info.owner, info.repo, prNumber); } infoRow.appendChild(badge); diff --git a/src/lib/github-api.ts b/src/lib/github-api.ts index d162168..3ec2f16 100644 --- a/src/lib/github-api.ts +++ b/src/lib/github-api.ts @@ -1,4 +1,4 @@ -import type { ServiceWorkerRequest, PRBranchInfo, PRReviewStatus, PRDiffStats, CommitDiffStats, PRApproveResult, TagInfo, StargazerInfo, WatcherInfo, ForkInfo } from "./messages"; +import type { ServiceWorkerRequest, PRBranchInfo, PRReviewStatus, ReviewThreadDetail, PRDiffStats, CommitDiffStats, PRApproveResult, TagInfo, StargazerInfo, WatcherInfo, ForkInfo } from "./messages"; function sendMessage(request: ServiceWorkerRequest): Promise { return new Promise((resolve, reject) => { @@ -25,7 +25,7 @@ function sendMessage(request: ServiceWorkerRequest): Promise { }); } -export type { PRReviewStatus, PRDiffStats, CommitDiffStats, PRApproveResult, TagInfo, StargazerInfo, WatcherInfo, ForkInfo }; +export type { PRReviewStatus, ReviewThreadDetail, PRDiffStats, CommitDiffStats, PRApproveResult, TagInfo, StargazerInfo, WatcherInfo, ForkInfo }; export async function fetchPRBranches( owner: string, @@ -65,6 +65,23 @@ export async function fetchPRReviewStatuses( } } +export async function fetchReviewThreadDetails( + owner: string, + repo: string, + prNumber: number, +): Promise { + // Unlike the list-level fetchers above, this one lets errors propagate so the + // popover can surface a "couldn't load" state and retry on the next open, + // rather than swallowing the failure into an empty list that reads as + // "no threads". The count badge already stands on its own. + return sendMessage({ + type: "FETCH_PR_REVIEW_THREAD_DETAILS", + owner, + repo, + prNumber, + }); +} + export async function fetchPRDiffStats( owner: string, repo: string, diff --git a/src/lib/messages.ts b/src/lib/messages.ts index f68c2e3..f08bdb3 100644 --- a/src/lib/messages.ts +++ b/src/lib/messages.ts @@ -22,8 +22,6 @@ export interface PRReviewStatus { number: number; totalThreads: number; resolvedThreads: number; - /** Details for the unresolved threads only (resolved ones are omitted to keep the payload small). */ - unresolved?: ReviewThreadDetail[]; } export interface PRDiffStats { @@ -74,6 +72,7 @@ export interface ForkInfo { export type ServiceWorkerRequest = | { type: "FETCH_PR_BRANCHES"; owner: string; repo: string; state: string; page: number } | { type: "FETCH_PR_REVIEW_STATUSES"; owner: string; repo: string; prNumbers: number[] } + | { type: "FETCH_PR_REVIEW_THREAD_DETAILS"; owner: string; repo: string; prNumber: number } | { type: "FETCH_PR_DIFF_STATS"; owner: string; repo: string; prNumbers: number[] } | { type: "FETCH_COMMIT_DIFF_STATS"; owner: string; repo: string; shas: string[] } | { type: "APPROVE_PR"; owner: string; repo: string; prNumber: number; body?: string } diff --git a/src/service-worker.test.ts b/src/service-worker.test.ts index a3a8efa..73f78f6 100644 --- a/src/service-worker.test.ts +++ b/src/service-worker.test.ts @@ -231,9 +231,10 @@ describe("service worker", () => { ); }); - it("clears review-status caches after approving a PR", async () => { + it("clears both review caches after approving a PR", async () => { const state = await loadWorker("token"); state.sessionStore["cache:reviews:owner/repo:1,2"] = { data: [], timestamp: 1 }; + state.sessionStore["cache:reviewdetails:owner/repo:1"] = { data: [], timestamp: 1 }; state.sessionStore["cache:branches:owner/repo:open:1"] = { data: [], timestamp: 1 }; vi.mocked(fetch).mockResolvedValue(jsonResponse({})); @@ -247,7 +248,10 @@ describe("service worker", () => { await Promise.resolve(); expect(response).toEqual({ ok: true, data: { success: true } }); + // Both the count cache and the detail cache for this repo are invalidated... expect(state.sessionStore["cache:reviews:owner/repo:1,2"]).toBeUndefined(); + expect(state.sessionStore["cache:reviewdetails:owner/repo:1"]).toBeUndefined(); + // ...but unrelated caches survive. expect(state.sessionStore["cache:branches:owner/repo:open:1"]).toBeDefined(); }); @@ -275,7 +279,7 @@ describe("service worker", () => { expect(state.sessionStore["cache:branches:owner/repo:open:1"]).toBeDefined(); }); - it("maps GraphQL review threads into counts plus unresolved-thread details", async () => { + it("maps GraphQL review threads into counts only (no eager detail payload)", async () => { const state = await loadWorker("token"); vi.mocked(fetch).mockResolvedValue( jsonResponse({ @@ -284,6 +288,41 @@ describe("service worker", () => { pr_1: { reviewThreads: { totalCount: 3, + // The list query asks for `isResolved` only — no path/author/body. + nodes: [{ isResolved: true }, { isResolved: true }, { isResolved: false }], + }, + }, + }, + }, + }), + ); + + const response = await sendMessage(state.messageListeners[0], { + type: "FETCH_PR_REVIEW_STATUSES", + owner: "owner", + repo: "repo", + prNumbers: [1], + }); + + expect(response).toEqual({ + ok: true, + data: [{ number: 1, totalThreads: 3, resolvedThreads: 2 }], + }); + expect(vi.mocked(fetch).mock.calls[0][0]).toBe("https://api.github.com/graphql"); + // The list query must NOT pull the heavy per-thread fields. + const listBody = JSON.parse(vi.mocked(fetch).mock.calls[0][1]?.body as string).query as string; + expect(listBody).not.toContain("bodyText"); + expect(listBody).not.toContain("comments("); + }); + + it("fetches a single PR's unresolved thread details on demand", async () => { + const state = await loadWorker("token"); + vi.mocked(fetch).mockResolvedValue( + jsonResponse({ + data: { + repository: { + pr_1: { + reviewThreads: { nodes: [ { isResolved: true, isOutdated: false, path: "a.ts", line: 1, originalLine: 1, comments: { nodes: [{ author: { login: "alice" }, bodyText: "done", url: "u1" }] } }, { isResolved: true, isOutdated: false, path: "b.ts", line: 2, originalLine: 2, comments: { nodes: [{ author: { login: "bob" }, bodyText: "ok", url: "u2" }] } }, @@ -298,30 +337,23 @@ describe("service worker", () => { ); const response = await sendMessage(state.messageListeners[0], { - type: "FETCH_PR_REVIEW_STATUSES", + type: "FETCH_PR_REVIEW_THREAD_DETAILS", owner: "owner", repo: "repo", - prNumbers: [1], + prNumber: 1, }); + // Only the single unresolved thread is returned; resolved ones are dropped. expect(response).toEqual({ ok: true, data: [ - { - number: 1, - totalThreads: 3, - resolvedThreads: 2, - // Only the single unresolved thread is detailed; resolved ones are dropped. - unresolved: [ - { path: "src/c.ts", line: 42, isOutdated: true, author: "carol", snippet: "needs a null check", url: "u3" }, - ], - }, + { path: "src/c.ts", line: 42, isOutdated: true, author: "carol", snippet: "needs a null check", url: "u3" }, ], }); expect(vi.mocked(fetch).mock.calls[0][0]).toBe("https://api.github.com/graphql"); }); - it("tolerates threads with missing author / path / comments", async () => { + it("tolerates detail threads with missing author / path / comments", async () => { const state = await loadWorker("token"); vi.mocked(fetch).mockResolvedValue( jsonResponse({ @@ -329,7 +361,6 @@ describe("service worker", () => { repository: { pr_1: { reviewThreads: { - totalCount: 1, nodes: [ { isResolved: false, isOutdated: false, path: null, line: null, comments: { nodes: [{ author: null, bodyText: "", url: "" }] } }, ], @@ -341,22 +372,15 @@ describe("service worker", () => { ); const response = await sendMessage(state.messageListeners[0], { - type: "FETCH_PR_REVIEW_STATUSES", + type: "FETCH_PR_REVIEW_THREAD_DETAILS", owner: "owner", repo: "repo", - prNumbers: [1], + prNumber: 1, }); expect(response).toEqual({ ok: true, - data: [ - { - number: 1, - totalThreads: 1, - resolvedThreads: 0, - unresolved: [{ path: "", line: null, isOutdated: false, author: "", snippet: "", url: "" }], - }, - ], + data: [{ path: "", line: null, isOutdated: false, author: "", snippet: "", url: "" }], }); }); diff --git a/src/service-worker.ts b/src/service-worker.ts index 2a62034..d78e8c1 100644 --- a/src/service-worker.ts +++ b/src/service-worker.ts @@ -1,4 +1,4 @@ -import type { ServiceWorkerRequest, ServiceWorkerResponse, PRBranchInfo, PRReviewStatus, PRDiffStats, CommitDiffStats, PRApproveResult, TagInfo, StargazerInfo, WatcherInfo, ForkInfo } from "./lib/messages"; +import type { ServiceWorkerRequest, ServiceWorkerResponse, PRBranchInfo, PRReviewStatus, ReviewThreadDetail, PRDiffStats, CommitDiffStats, PRApproveResult, TagInfo, StargazerInfo, WatcherInfo, ForkInfo } from "./lib/messages"; const CACHE_TTL = 5 * 60 * 1000; // 5 minutes @@ -152,6 +152,13 @@ ${entries} }); } +// Per-PR cap on how many review threads we enumerate, for both the count query +// and the detail query. GraphQL can't filter reviewThreads by isResolved +// server-side, so the detail query over-fetches up to this many and drops the +// resolved ones client-side; `totalCount` keeps the badge count accurate even +// when a PR has more threads than this. +const MAX_REVIEW_THREADS = 100; + async function fetchPRReviewStatuses( owner: string, repo: string, @@ -163,12 +170,42 @@ async function fetchPRReviewStatuses( repo, keys: [...prNumbers].sort((a, b) => a - b), aliasFor: (n) => `pr_${n}`, - // We fetch path/line/comments for *every* thread even though only unresolved - // ones are detailed below — GraphQL can't filter reviewThreads by isResolved - // server-side, and totalCount is needed for the resolved/total count anyway. + // List-page badges only need counts, so this batch query stays cheap: just + // `totalCount` plus a per-thread `isResolved`. The heavy path/author/body + // details are fetched lazily, one PR at a time, when a badge's popover is + // actually opened (see fetchPRReviewThreadDetails). buildNodeQuery: (n) => `pullRequest(number: ${n}) { - reviewThreads(first: 100) { + reviewThreads(first: ${MAX_REVIEW_THREADS}) { totalCount + nodes { + isResolved + } + } + }`, + parseNode: (n, pr) => { + const threads = pr.reviewThreads as { + totalCount: number; + nodes: Array<{ isResolved: boolean }>; + }; + const resolvedThreads = threads.nodes.filter((t) => t.isResolved).length; + return { number: n, totalThreads: threads.totalCount, resolvedThreads }; + }, + }); +} + +async function fetchPRReviewThreadDetails( + owner: string, + repo: string, + prNumber: number, +): Promise { + const batched = await fetchGraphQLBatch({ + cachePrefix: "reviewdetails", + owner, + repo, + keys: [prNumber], + aliasFor: (n) => `pr_${n}`, + buildNodeQuery: (n) => `pullRequest(number: ${n}) { + reviewThreads(first: ${MAX_REVIEW_THREADS}) { nodes { isResolved isOutdated @@ -185,9 +222,8 @@ async function fetchPRReviewStatuses( } } }`, - parseNode: (n, pr) => { + parseNode: (_n, pr) => { const threads = pr.reviewThreads as { - totalCount: number; nodes: Array<{ isResolved: boolean; isOutdated: boolean; @@ -197,8 +233,7 @@ async function fetchPRReviewStatuses( comments: { nodes: Array<{ author: { login: string } | null; bodyText: string; url: string }> }; }>; }; - const resolvedThreads = threads.nodes.filter((t) => t.isResolved).length; - const unresolved = threads.nodes + return threads.nodes .filter((t) => !t.isResolved) .map((t) => { const first = t.comments?.nodes?.[0]; @@ -212,9 +247,9 @@ async function fetchPRReviewStatuses( url: first?.url ?? "", }; }); - return { number: n, totalThreads: threads.totalCount, resolvedThreads, unresolved }; }, }); + return batched[0] ?? []; } async function fetchPRDiffStats( @@ -303,9 +338,13 @@ async function approvePR( return { success: false, error: msg }; } - // Fire-and-forget: invalidate review caches for this repo + // Fire-and-forget: invalidate both review caches (counts + detail) for this repo chrome.storage.session.get(null).then((all) => { - const keys = Object.keys(all).filter((k) => k.startsWith(`cache:reviews:${owner}/${repo}:`)); + const keys = Object.keys(all).filter( + (k) => + k.startsWith(`cache:reviews:${owner}/${repo}:`) || + k.startsWith(`cache:reviewdetails:${owner}/${repo}:`), + ); if (keys.length > 0) chrome.storage.session.remove(keys); }).catch(() => {}); @@ -521,6 +560,8 @@ async function handleMessage(request: ServiceWorkerRequest): Promise