diff --git a/src/features/pr-review-status.test.ts b/src/features/pr-review-status.test.ts index 58101a5..710a96a 100644 --- a/src/features/pr-review-status.test.ts +++ b/src/features/pr-review-status.test.ts @@ -59,6 +59,126 @@ describe("injectPRReviewStatus", () => { expect(badge8.title).toBe("2/5 review thread(s) resolved"); }); + it("builds a thread-detail popover on the unresolved badge", 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" }, + ], + }, + ]); + + 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); + + const items = badge.querySelectorAll(".better-github-review-popover-item"); + expect(items).toHaveLength(2); + + // First item: anchor to the thread, basename-only location, author-prefixed snippet. + const first = items[0] as HTMLAnchorElement; + expect(first.tagName).toBe("A"); + 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 + expect(first.querySelector(".better-github-review-popover-body")?.textContent).toBe( + "@alice: Guard the null case before dereferencing this.", + ); + expect(first.querySelector(".better-github-review-popover-outdated")).toBeNull(); + + // Second item: no line number, flagged outdated. + const second = items[1] as HTMLElement; + expect(second.querySelector(".better-github-review-popover-loc-text")?.textContent).toBe("README.md"); + expect(second.querySelector(".better-github-review-popover-outdated")?.textContent).toBe("outdated"); + }); + + 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" }, + ], + }, + ]); + + await injectPRReviewStatus(); + + const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; + expect(badge.getAttribute("role")).toBe("button"); + expect(badge.getAttribute("aria-expanded")).toBe("false"); + + // Click the badge → opens. + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(true); + expect(badge.getAttribute("aria-expanded")).toBe("true"); + + // Click the badge again → closes (toggle). + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(false); + + // Open again, then an outside click dismisses it. + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(true); + document.body.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(false); + + // Open again, then Escape dismisses it. + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(true); + document.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(false); + }); + + 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" }, + ], + }, + ]); + + await injectPRReviewStatus(); + + const badge = document.querySelector("#issue_7 .better-github-review-status") as HTMLElement; + badge.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(badge.classList.contains("better-github-review-popover-open")).toBe(true); + + // Clicking the link row must fall through to navigation, leaving the popover open. + const item = badge.querySelector(".better-github-review-popover-item") as HTMLElement; + item.dispatchEvent(new MouseEvent("click", { bubbles: true, cancelable: true })); + 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 67ad586..2eae4f3 100644 --- a/src/features/pr-review-status.ts +++ b/src/features/pr-review-status.ts @@ -1,8 +1,158 @@ import { isPRListPage, getRepoInfo } from "../lib/page-detect"; import { fetchPRReviewStatuses } from "../lib/github-api"; +import type { ReviewThreadDetail } from "../lib/messages"; import { getOrCreateInfoRow } from "../lib/info-row"; const STATUS_CLASS = "better-github-review-status"; +const POPOVER_CLASS = "better-github-review-popover"; +const OPEN_CLASS = "better-github-review-popover-open"; + +/** Cap the list so a PR with dozens of threads can't grow an unwieldy popover. */ +const MAX_POPOVER_ITEMS = 8; + +/** Collapse every open popover (used by the outside-click / Escape handlers). */ +function closeAllPopovers(): void { + for (const open of document.querySelectorAll(`.${OPEN_CLASS}`)) { + open.classList.remove(OPEN_CLASS); + open.setAttribute("aria-expanded", "false"); + } +} + +// A single pair of document-level listeners dismisses popovers on an outside +// click or Escape — attached once, no matter how often the feature re-injects. +let globalListenersAttached = false; +function ensureGlobalListeners(): void { + if (globalListenersAttached) return; + globalListenersAttached = true; + // Badge clicks call stopPropagation, so this only fires for clicks elsewhere. + document.addEventListener("click", closeAllPopovers); + document.addEventListener("keydown", (e) => { + if (e.key === "Escape") closeAllPopovers(); + }); +} + +/** Wire a badge to open/close its popover on click (and keyboard). */ +function makeBadgeInteractive(badge: HTMLElement): void { + badge.setAttribute("role", "button"); + badge.setAttribute("tabindex", "0"); + // "dialog" rather than "true"/"menu": the popover is a list of links, not a menu. + badge.setAttribute("aria-haspopup", "dialog"); + badge.setAttribute("aria-expanded", "false"); + + const toggle = () => { + const willOpen = !badge.classList.contains(OPEN_CLASS); + closeAllPopovers(); + badge.classList.toggle(OPEN_CLASS, willOpen); + badge.setAttribute("aria-expanded", String(willOpen)); + }; + + // Only react to events on the badge itself; events bubbling up from inside the + // popover (a thread row click, Enter on a focused link) are left to navigate. + badge.addEventListener("click", (e) => { + if (e.target !== badge) return; + e.stopPropagation(); // don't let the document handler immediately re-close it + toggle(); + }); + badge.addEventListener("keydown", (e) => { + if (e.target !== badge) return; + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + e.stopPropagation(); + toggle(); + } + }); + + ensureGlobalListeners(); +} + +/** Octicon `comment` (16px) — GitHub's own glyph for review conversations. */ +const COMMENT_ICON = + '"; + +/** Last path segment, so a long `a/b/c/file.ts` shows as just `file.ts`. */ +function basename(path: string): string { + if (!path) return ""; + const parts = path.split("/"); + return parts[parts.length - 1] || path; +} + +/** + * 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. + */ +function buildPopover(threads: ReviewThreadDetail[]): HTMLElement { + const popover = document.createElement("div"); + popover.className = POPOVER_CLASS; + + // Keep clicks inside the popover from reaching the document dismiss handler, so + // 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. + const caret = document.createElement("span"); + caret.className = "better-github-review-popover-caret"; + popover.appendChild(caret); + + // Header row, like "3 unresolved threads" with a comment glyph. + const header = document.createElement("div"); + header.className = "better-github-review-popover-header"; + const icon = document.createElement("span"); + icon.className = "better-github-review-popover-header-icon"; + icon.innerHTML = COMMENT_ICON; + const headerLabel = document.createElement("span"); + headerLabel.textContent = `${threads.length} unresolved thread${threads.length === 1 ? "" : "s"}`; + header.append(icon, headerLabel); + popover.appendChild(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 + // the API gave us no permalink. + const item = document.createElement(t.url ? "a" : "div"); + item.className = "better-github-review-popover-item"; + if (t.url && item instanceof HTMLAnchorElement) { + item.href = t.url; + } + + const head = document.createElement("div"); + head.className = "better-github-review-popover-loc"; + const file = t.path ? basename(t.path) : "general comment"; + 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; + head.appendChild(locText); + if (t.isOutdated) { + const tag = document.createElement("span"); + tag.className = "better-github-review-popover-outdated"; + tag.textContent = "outdated"; + head.appendChild(tag); + } + + const body = document.createElement("div"); + body.className = "better-github-review-popover-body"; + const author = t.author ? `@${t.author}` : ""; + const snippet = t.snippet.trim(); + body.textContent = author && snippet ? `${author}: ${snippet}` : author || snippet || "(no comment)"; + + item.appendChild(head); + item.appendChild(body); + popover.appendChild(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); + } + + return popover; +} export async function injectPRReviewStatus(): Promise { if (!isPRListPage()) return; @@ -57,6 +207,13 @@ export async function injectPRReviewStatus(): Promise { 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) { + badge.classList.add("better-github-review-has-popover"); + badge.appendChild(buildPopover(details)); + makeBadgeInteractive(badge); + } } infoRow.appendChild(badge); diff --git a/src/lib/messages.ts b/src/lib/messages.ts index 07dbeb7..f68c2e3 100644 --- a/src/lib/messages.ts +++ b/src/lib/messages.ts @@ -3,10 +3,27 @@ export interface PRBranchInfo { headRef: string; } +export interface ReviewThreadDetail { + /** File path the thread is anchored to (empty for file-level / outdated threads). */ + path: string; + /** Line number in the diff, or null when the thread is outdated / not line-anchored. */ + line: number | null; + /** Whether the diff the thread was left on has since changed. */ + isOutdated: boolean; + /** Login of the author of the first comment in the thread. */ + author: string; + /** Plain-text body of the first comment (already stripped of Markdown by the API). */ + snippet: string; + /** Permalink to the first comment, for jumping straight to the thread. */ + url: string; +} + 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 { diff --git a/src/service-worker.test.ts b/src/service-worker.test.ts index 0947b90..a3a8efa 100644 --- a/src/service-worker.test.ts +++ b/src/service-worker.test.ts @@ -275,13 +275,23 @@ describe("service worker", () => { expect(state.sessionStore["cache:branches:owner/repo:open:1"]).toBeDefined(); }); - it("maps GraphQL review threads into resolved/total counts", async () => { + it("maps GraphQL review threads into counts plus unresolved-thread details", async () => { const state = await loadWorker("token"); vi.mocked(fetch).mockResolvedValue( jsonResponse({ data: { repository: { - pr_1: { reviewThreads: { totalCount: 3, nodes: [{ isResolved: true }, { isResolved: true }, { isResolved: false }] } }, + pr_1: { + reviewThreads: { + totalCount: 3, + 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" }] } }, + // Outdated: `line` is null, so the position falls back to `originalLine`. + { isResolved: false, isOutdated: true, path: "src/c.ts", line: null, originalLine: 42, comments: { nodes: [{ author: { login: "carol" }, bodyText: "needs a null check", url: "u3" }] } }, + ], + }, + }, }, }, }), @@ -294,10 +304,62 @@ describe("service worker", () => { prNumbers: [1], }); - expect(response).toEqual({ ok: true, data: [{ number: 1, totalThreads: 3, resolvedThreads: 2 }] }); + 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" }, + ], + }, + ], + }); expect(vi.mocked(fetch).mock.calls[0][0]).toBe("https://api.github.com/graphql"); }); + it("tolerates threads with missing author / path / comments", async () => { + const state = await loadWorker("token"); + vi.mocked(fetch).mockResolvedValue( + jsonResponse({ + data: { + repository: { + pr_1: { + reviewThreads: { + totalCount: 1, + nodes: [ + { isResolved: false, isOutdated: false, path: null, line: null, comments: { nodes: [{ author: null, bodyText: "", url: "" }] } }, + ], + }, + }, + }, + }, + }), + ); + + 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: 1, + resolvedThreads: 0, + unresolved: [{ path: "", line: null, isOutdated: false, author: "", snippet: "", url: "" }], + }, + ], + }); + }); + it("normalizes commit SHAs and parses GraphQL commit diff stats", async () => { const SHA = "0123456789abcdef0123456789abcdef01234567"; const state = await loadWorker("token"); diff --git a/src/service-worker.ts b/src/service-worker.ts index 46bdf39..2a62034 100644 --- a/src/service-worker.ts +++ b/src/service-worker.ts @@ -163,16 +163,56 @@ 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. buildNodeQuery: (n) => `pullRequest(number: ${n}) { reviewThreads(first: 100) { totalCount - nodes { isResolved } + nodes { + isResolved + isOutdated + path + line + originalLine + comments(first: 1) { + nodes { + author { login } + bodyText + url + } + } + } } }`, parseNode: (n, pr) => { - const threads = pr.reviewThreads as { totalCount: number; nodes: Array<{ isResolved: boolean }> }; + const threads = pr.reviewThreads as { + totalCount: number; + nodes: Array<{ + isResolved: boolean; + isOutdated: boolean; + path: string | null; + line: number | null; + originalLine: number | null; + comments: { nodes: Array<{ author: { login: string } | null; bodyText: string; url: string }> }; + }>; + }; const resolvedThreads = threads.nodes.filter((t) => t.isResolved).length; - return { number: n, totalThreads: threads.totalCount, resolvedThreads }; + const unresolved = threads.nodes + .filter((t) => !t.isResolved) + .map((t) => { + const first = t.comments?.nodes?.[0]; + return { + path: t.path ?? "", + // Outdated threads carry their position in `originalLine`, not `line`. + line: t.line ?? t.originalLine ?? null, + isOutdated: t.isOutdated, + author: first?.author?.login ?? "", + snippet: first?.bodyText ?? "", + url: first?.url ?? "", + }; + }); + return { number: n, totalThreads: threads.totalCount, resolvedThreads, unresolved }; }, }); } diff --git a/src/styles/content.css b/src/styles/content.css index 436647b..300ccd8 100644 --- a/src/styles/content.css +++ b/src/styles/content.css @@ -59,6 +59,140 @@ background-color: var(--bgColor-attention-muted, var(--color-attention-subtle, #fff8c5)); } +/* Unresolved-thread details popover (click) — styled after GitHub's hovercard / + commit-status checks dropdown: caret + bordered header + tidy rows. */ +.better-github-review-has-popover { + position: relative; + cursor: pointer; +} + +.better-github-review-popover { + display: none; + position: absolute; + top: 100%; + left: 0; + z-index: 100; + margin-top: 8px; + width: 340px; + font-weight: 400; + text-align: left; + white-space: normal; + cursor: auto; + background-color: var(--overlay-bgColor, var(--color-canvas-overlay, #fff)); + border: 1px solid var(--borderColor-default, var(--color-border-default, #d0d7de)); + border-radius: 6px; + box-shadow: var(--shadow-floating-large, var(--color-shadow-large, 0 8px 24px rgba(140, 149, 159, 0.2))); +} + +.better-github-review-popover-open .better-github-review-popover { + display: block; +} + +/* Upward caret: a rotated square exposing only its top + left borders. */ +.better-github-review-popover-caret { + position: absolute; + top: -5px; + left: 18px; + width: 9px; + height: 9px; + background-color: var(--overlay-bgColor, var(--color-canvas-overlay, #fff)); + border: 1px solid var(--borderColor-default, var(--color-border-default, #d0d7de)); + border-right: 0; + border-bottom: 0; + transform: rotate(45deg); +} + +.better-github-review-popover-header { + display: flex; + align-items: center; + gap: 6px; + padding: 8px 12px; + font-size: 12px; + font-weight: 600; + line-height: 18px; + color: var(--fgColor-default, var(--color-fg-default, #1f2328)); + border-bottom: 1px solid var(--borderColor-muted, var(--color-border-muted, #d8dee4)); +} + +.better-github-review-popover-header-icon { + display: inline-flex; + flex: 0 0 auto; + color: var(--fgColor-muted, var(--color-fg-muted, #656d76)); +} + +.better-github-review-popover-header-icon svg { + fill: currentColor; +} + +.better-github-review-popover-item { + display: block; + padding: 8px 12px; + text-decoration: none; + color: inherit; + border-bottom: 1px solid var(--borderColor-muted, var(--color-border-muted, #d8dee4)); +} + +.better-github-review-popover-item:last-child { + border-bottom: none; + border-bottom-left-radius: 6px; + border-bottom-right-radius: 6px; +} + +a.better-github-review-popover-item:hover { + background-color: var(--bgColor-muted, var(--color-canvas-subtle, #f6f8fa)); +} + +.better-github-review-popover-more { + padding: 6px 12px; + font-size: 12px; + color: var(--fgColor-muted, var(--color-fg-muted, #656d76)); + border-top: 1px solid var(--borderColor-muted, var(--color-border-muted, #d8dee4)); +} + +.better-github-review-popover-loc { + display: flex; + align-items: center; + gap: 6px; + font-family: + ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, "Liberation Mono", monospace; + font-size: 12px; + font-weight: 600; + line-height: 18px; + color: var(--fgColor-accent, var(--color-accent-fg, #0969da)); +} + +/* The file:line run truncates with an ellipsis; the outdated tag stays visible. */ +.better-github-review-popover-loc-text { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.better-github-review-popover-outdated { + flex: 0 0 auto; + padding: 0 6px; + font-family: inherit; + font-size: 11px; + font-weight: 500; + line-height: 16px; + color: var(--fgColor-muted, var(--color-fg-muted, #656d76)); + background-color: var(--bgColor-muted, var(--color-canvas-subtle, #f6f8fa)); + border-radius: 2em; +} + +.better-github-review-popover-body { + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 2; + line-clamp: 2; + overflow: hidden; + margin-top: 2px; + font-size: 12px; + line-height: 16px; + color: var(--fgColor-default, var(--color-fg-default, #1f2328)); + word-break: break-word; +} + /* Diff Stats Badge — used by PR list and commits list */ .better-github-diff-stats, .better-github-commit-diff-stats {