Skip to content

fix: stop native tooltips overlapping custom popovers #27

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

Merged
rrbe merged 2 commits into from
Jun 3, 2026
Merged

fix: stop native tooltips overlapping custom popovers #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c7b5c9
fix: stop native tooltips overlapping custom popovers
rrbe Jun 3, 2026
30049f3
fix: lazy-load review thread details and drop in-popover tooltips
rrbe Jun 3, 2026
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
142 changes: 96 additions & 46 deletions src/features/pr-review-status.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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<void> {
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();
Expand Down Expand Up @@ -43,40 +52,68 @@ 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;
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);
Expand All @@ -87,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.",
);
Expand All @@ -99,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();
Expand Down Expand Up @@ -143,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.
Expand All @@ -165,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([
Expand Down
Loading