Skip to content

feat: show unresolved review thread details in a click popover#26

Merged
rrbe merged 2 commits into
mainfrom
feat/pr-unresolved-thread-details
Jun 3, 2026
Merged

feat: show unresolved review thread details in a click popover#26
rrbe merged 2 commits into
mainfrom
feat/pr-unresolved-thread-details

Conversation

@rrbe
Copy link
Copy Markdown
Owner

@rrbe rrbe commented Jun 3, 2026

What

The PR-list "N unresolved" badge was just a count. Now clicking it opens a
GitHub-style popover that lists each unresolved review thread, so you can see
what's unresolved without opening the PR — and jump straight to any thread.

Each row shows:

  • file:line (basename only; full path in the title tooltip)
  • an outdated marker when the diff has since changed
  • @author: comment — the first comment, clamped to 2 lines

Every row links to that comment's exact …/pull/N#discussion_r… anchor.

How

  • Data (service-worker.ts): expand the reviewThreads GraphQL query to
    fetch path / line / isOutdated and the first comment's
    author / bodyText / url; only unresolved threads are mapped into the new
    PRReviewStatus.unresolved field (resolved ones are dropped to keep the
    payload small).
  • UI (pr-review-status.ts + content.css): a click-to-open popover —
    caret, bordered header with the comment octicon, tidy rows, and a +N more
    footer (list capped at 8). Dismisses on outside-click / Escape; role=button,
    aria-expanded, and Enter/Space keyboard support. Clicks inside the popover
    don't dismiss it, so thread links navigate normally.
  • Long-text safety: handled entirely in CSS (path → basename + ellipsis,
    body → 2-line clamp), so the popover stays a fixed 340px wide no matter how
    verbose a comment is.

Notes / verification

  • Popover uses position: absolute. Checked the live PR-list DOM on a real repo
    (old GitHub DOM): no overflow: hidden/auto/scroll ancestor between the issue
    row and <body>, so it isn't clipped.
  • Legacy/cached PRReviewStatus payloads without unresolved degrade
    gracefully — the badge renders with no popover (the existing title tooltip
    stays).

Tests

pnpm typecheck + pnpm test green (178 passed). New cases cover the
GraphQL → details mapping (incl. missing author/path/comments), popover
structure, basename truncation, click toggle, outside-click/Escape dismissal,
inside-click not dismissing, and the no-details fallback.

🤖 Generated with Claude Code

rrbe and others added 2 commits June 3, 2026 10:08
The PR-list "N unresolved" badge now opens a GitHub-style popover that lists
each unresolved thread — file:line, author, a 2-line comment snippet, and an
"outdated" marker — with every row linking to its #discussion_r… anchor.

- Expand the reviewThreads GraphQL query to fetch path/line/isOutdated plus the
  first comment's author/bodyText/url, and map unresolved threads into a new
  PRReviewStatus.unresolved field.
- Render a click-to-open popover (caret + bordered header + rows + "+N more"
  footer) styled after GitHub's hovercard / checks dropdown; dismiss on
  outside-click or Escape, with role/aria and keyboard support.
- Tame long text entirely in CSS (path → basename + ellipsis, body → 2-line
  clamp) so the popover keeps a fixed 340px width.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Outdated threads carry their position in `originalLine`, not `line`
(which GitHub returns as null), so the popover was showing just the
filename with no line number for them. Use line ?? originalLine.

Also tighten aria-haspopup to "dialog" (the popover is a link list,
not a menu) and document why the query fetches comment bodies for
resolved threads it later drops.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rrbe rrbe merged commit 9cff0f8 into main Jun 3, 2026
1 check passed
@rrbe rrbe deleted the feat/pr-unresolved-thread-details branch June 3, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant