authorgravatar for 62337003+Bil0000@users.noreply.github.comBilal Bakr <62337003+Bil0000@users.noreply.github.com> 2026-08-17 21:01:50+03:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-08-17 20:01:50+02:00
logc7e6d711d3f2982e54854258987509e8f69b31cd
tree68c456e0a86bb4eac333265465f53dfbbb44677c
parentcd096b9ad5a4156ffeab85de617cbb219057007f
signature Signed by PGP key B5690EEEBB952194

feat(web): make review verdicts legible in the pull request detail (#7077)

6 files changed, 761 insertions(+), 41 deletions(-)

apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx+34
......@@ -103,6 +103,7 @@ import {
103103 buildResolveConflictsPrompt,
104104 handoffPrompt,
105105 handoffReviewComments,
106 latestPullRequestReviewOutcomes,
106107 pullRequestActionMenuHasGroup,
107108 pullRequestActionNeedsHostRefresh,
108109 pullRequestComposerTarget,
......@@ -123,7 +124,9 @@ import {
123124 PullRequestActorLabel,
124125 PullRequestDiffStat,
125126 PullRequestMetaLine,
127 PullRequestReviewOutcomeIcon,
126128 pullRequestChecksState,
129 pullRequestReviewOutcomeToneClassName,
127130 resolvePullRequestState,
128131 summarizePullRequestChecks,
129132} from "./pullRequestPresentation";
......@@ -1075,6 +1078,19 @@ export function PullRequestDetailPanel({
10751078 : null;
10761079 const checksSummary = detail ? summarizePullRequestChecks(detail.checks) : null;
10771080 const checksState = detail ? pullRequestChecksState(detail.checks) : null;
1081 // Approvals that still stand, and only those. A superseded one is dimmed beside the reviewer
1082 // who gave it, so counting it here would have the header assert in a number what the row next
1083 // to it has just qualified.
1084 //
1085 // Not counted at all from a conversation this page only holds the recent end of: an approval
1086 // older than the window would be missing, and "1" beside a tick is read as the whole answer.
1087 // The Summary tab's row can say it may be short; a bare number cannot, so it stays away.
1088 const approvalCount =
1089 detail && !detail.commentsTruncated
1090 ? latestPullRequestReviewOutcomes(detail.comments, detail.commits).filter(
1091 (entry) => entry.outcome === "approved" && !entry.stale,
1092 ).length
1093 : 0;
10781094
10791095 return (
10801096 <div className="flex h-full min-h-0 w-full flex-col bg-background">
......@@ -1813,6 +1829,24 @@ export function PullRequestDetailPanel({
18131829 ? "…"
18141830 : detail.commits.length.toLocaleString()}
18151831 </span>
1832 {/* Only once somebody has approved: a nought beside a tick would read as a
1833 verdict of its own on a change nobody has looked at yet. */}
1834 {approvalCount > 0 ? (
1835 <span
1836 className={cn(
1837 "inline-flex items-center gap-1",
1838 pullRequestReviewOutcomeToneClassName("approved"),
1839 )}
1840 >
1841 <PullRequestReviewOutcomeIcon outcome="approved" className="size-3" />
1842 {approvalCount.toLocaleString()}
1843 {/* The icon is decorative and a name written onto a generic span is
1844 not announced, so the bare number says what it counts in words. */}
1845 <span className="sr-only">
1846 {approvalCount === 1 ? "approval" : "approvals"}
1847 </span>
1848 </span>
1849 ) : null}
18161850 </PullRequestMetaLine>
18171851 <Button
18181852 size="xs"
apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx+159-39
......@@ -33,13 +33,20 @@ import {
3333 PullRequestActorAvatar,
3434 PullRequestActorLabel,
3535 PullRequestCheckStatusIcon,
36 PullRequestReviewOutcomeBadge,
3637 pullRequestCheckStatusLabel,
38 pullRequestReviewOutcomeLabel,
39 pullRequestReviewOutcomeRingClassName,
40 pullRequestReviewOutcomeStaleLabel,
3741} from "./pullRequestPresentation";
3842import { PullRequestReviewerPicker } from "./PullRequestReviewerPicker";
3943import { PullRequestActivityUnavailableState } from "./PullRequestActivityUnavailableState";
4044import {
45 latestPullRequestReviewOutcomes,
4146 orderPullRequestComments,
4247 pullRequestFindingKey,
48 pullRequestReviewOutcome,
49 visibleBody,
4350 type PullRequestFinding,
4451} from "./pullRequestDetail.logic";
4552import {
......@@ -52,6 +59,11 @@ import { PullRequestReactionBar } from "./PullRequestReactions";
5259import { PullRequestConversationGhost } from "./PullRequestGhosts";
5360import { sectionCollapseAnchorScrollTop } from "./pullRequestSummaryScroll.logic";
5461
62/** One reviewer, however a host happens to have cased their login this time. */
63function reviewerKey(login: string): string {
64 return login.toLowerCase();
65}
66
5567/** A host colour only when it is one, so a malformed value falls back to the neutral dot. */
5668function labelDotColor(color: string | null): string | null {
5769 const hex = color?.trim().replace(/^#/, "") ?? "";
......@@ -138,11 +150,14 @@ function CollapsedComment({
138150 comment,
139151 editing,
140152 label,
153 body,
141154 reactionBar,
142155}: {
143156 comment: PullRequestComment;
144157 editing: CommentEditing;
145158 label: string;
159 /** Null where the remark is nothing but its verdict, which a dismissal usually is. */
160 body: string | null;
146161 reactionBar: ReactNode;
147162}) {
148163 const [open, setOpen] = useState(false);
......@@ -181,7 +196,11 @@ function CollapsedComment({
181196 <TooltipPopup side="top">{comment.path}</TooltipPopup>
182197 </Tooltip>
183198 ) : null}
184 <CommentBody className="mt-2" comment={comment} editing={editing} />
199 {/* A dismissal carries no more words than an approval does, and an empty markdown
200 block reads as a card somebody forgot to fill in. */}
201 {body === null && !editing.canEdit(comment) ? null : (
202 <CommentBody className="mt-2" comment={comment} editing={editing} />
203 )}
185204 {reactionBar}
186205 </div>
187206 ) : null}
......@@ -386,6 +405,42 @@ export function PullRequestSummaryTab({
386405 const hiddenCommentCount = detail.comments.length - recentComments.length;
387406 const [commentOrder, setCommentOrder] = useState<"newest" | "oldest">("newest");
388407 const visibleComments = orderPullRequestComments(recentComments, commentOrder);
408 // Read from the whole conversation, not the window shown below it: a verdict older than the
409 // last thirty comments still stands.
410 const reviewOutcomes = latestPullRequestReviewOutcomes(detail.comments, detail.commits);
411 // Hosts do not promise one casing for a login across two fields of the same response, and
412 // none of them lets `Octocat` and `octocat` be two people — so matching on the literal string
413 // would show one reviewer twice and drop the verdict off both.
414 const outcomeByLogin = new Map(
415 reviewOutcomes.flatMap((entry) =>
416 entry.actor ? [[reviewerKey(entry.actor.login), entry] as const] : [],
417 ),
418 );
419 // Everyone whose face belongs on this row: the people a review was asked of, then anyone who
420 // ruled without being on that list. A host drops a reviewer from the requested set once they
421 // have reviewed, and their verdict is the thing this row now exists to show.
422 const reviewerEntries = [
423 ...detail.reviewers.map((actor) => ({
424 key: actor.login,
425 actor,
426 outcome: outcomeByLogin.get(reviewerKey(actor.login))?.outcome ?? null,
427 stale: outcomeByLogin.get(reviewerKey(actor.login))?.stale ?? false,
428 })),
429 ...reviewOutcomes
430 .filter(
431 (entry) =>
432 !detail.reviewers.some(
433 (actor) =>
434 entry.actor !== null && reviewerKey(actor.login) === reviewerKey(entry.actor.login),
435 ),
436 )
437 .map((entry) => ({
438 key: entry.key,
439 actor: entry.actor,
440 outcome: entry.outcome,
441 stale: entry.stale,
442 })),
443 ];
389444
390445 // A comment that already lives on a review thread is that thread: the thread carries the line
391446 // and side the bare comment has lost, and a resolved one is finished work nobody should be
......@@ -464,33 +519,76 @@ export function PullRequestSummaryTab({
464519 <div>
465520 <MetaRow icon={<UsersIcon className="size-3.5" />} label="Reviewers">
466521 <span className="flex min-w-0 flex-wrap items-center gap-1.5">
467 {detail.reviewers.length === 0 ? (
522 {reviewerEntries.length === 0 ? (
468523 <span className="text-muted-foreground">None</span>
469524 ) : (
470525 <span className="flex items-center -space-x-1">
471 {detail.reviewers.map((actor) => (
472 <Tooltip key={actor.login}>
473 <TooltipTrigger
474 render={
475 <span
476 className="relative rounded-full hover:z-10"
477 aria-label={actor.name ?? actor.login}
526 {reviewerEntries.map((entry) => {
527 const login = entry.actor?.login ?? "ghost";
528 const named =
529 entry.actor?.name && entry.actor.name !== login
530 ? `${entry.actor.name} (@${login})`
531 : login;
532 return (
533 <Tooltip key={entry.key}>
534 {/* A verdict rides the face that earned it rather than a row of its own:
535 the ring sits outside the one that separates overlapping avatars, so
536 it reads at a glance without adding anything to scroll past. */}
537 <TooltipTrigger
538 render={
539 <span
540 className={cn(
541 "relative rounded-full hover:z-10",
542 // The verdict replaces the separator rather than ringing it. Both
543 // occupy the same 2px immediately outside a 16px avatar, so the
544 // colour costs no size: anything drawn further out would be a
545 // halo wide enough to eclipse the neighbour this stack overlaps
546 // by 4px. Painted by this wrapper because a child's box-shadow
547 // covers its parent's, never the other way round.
548 entry.outcome
549 ? pullRequestReviewOutcomeRingClassName(
550 entry.outcome,
551 entry.stale,
552 )
553 : undefined,
554 )}
555 />
556 }
557 >
558 <PullRequestActorLabel
559 actor={entry.actor}
560 tooltip={false}
561 className={cn(
562 "gap-0 [&>span:last-child]:sr-only",
563 // Only where the wrapper is not already drawing one, or the opaque
564 // separator would cover the verdict in the band they share.
565 entry.outcome
566 ? undefined
567 : "[&>img]:ring-2 [&>img]:ring-background [&>span:first-child]:ring-2 [&>span:first-child]:ring-background",
568 )}
478569 />
479 }
480 >
481 <PullRequestActorLabel
482 actor={actor}
483 tooltip={false}
484 className="gap-0 [&>img]:ring-2 [&>img]:ring-background [&>span:first-child]:ring-2 [&>span:first-child]:ring-background [&>span:last-child]:sr-only"
485 />
486 </TooltipTrigger>
487 <TooltipPopup side="bottom">
488 {actor.name && actor.name !== actor.login
489 ? `${actor.name} (@${actor.login})`
490 : actor.login}
491 </TooltipPopup>
492 </Tooltip>
493 ))}
570 {/* Colour alone says nothing to a reader who cannot see it, and the
571 login beside this is already in the accessible name. */}
572 {entry.outcome ? (
573 <span className="sr-only">
574 {entry.stale
575 ? pullRequestReviewOutcomeStaleLabel(entry.outcome)
576 : pullRequestReviewOutcomeLabel(entry.outcome)}
577 </span>
578 ) : null}
579 </TooltipTrigger>
580 <TooltipPopup side="bottom">
581 {entry.outcome
582 ? `${named} — ${
583 entry.stale
584 ? pullRequestReviewOutcomeStaleLabel(entry.outcome)
585 : pullRequestReviewOutcomeLabel(entry.outcome)
586 }`
587 : named}
588 </TooltipPopup>
589 </Tooltip>
590 );
591 })}
494592 </span>
495593 )}
496594 {/* Shown wherever the host can take a review request at all, and disabled with the
......@@ -697,14 +795,16 @@ export function PullRequestSummaryTab({
697795 ) : null}
698796 {visibleComments.map((comment) => {
699797 const thread = threadByCommentId.get(comment.id);
700 const reviewState = comment.reviewState?.toLowerCase();
701 if (thread?.isResolved || reviewState === "dismissed") {
798 const body = visibleBody(comment.body);
799 const outcome = pullRequestReviewOutcome(comment.reviewState);
800 if (thread?.isResolved || outcome === "dismissed") {
702801 return (
703802 <CollapsedComment
704803 key={comment.id}
705804 comment={comment}
706805 editing={commentEditing}
707806 label={thread?.isResolved ? "Resolved" : "Approval dismissed"}
807 body={body}
708808 reactionBar={
709809 <PullRequestReactionBar
710810 className="mt-2"
......@@ -722,11 +822,30 @@ export function PullRequestSummaryTab({
722822 // An approval is a verdict, not a finding: there is nothing in it to fix.
723823 const finding: PullRequestFinding | null =
724824 (comment.kind !== "review" && comment.kind !== "review-comment") ||
725 reviewState === "approved"
825 outcome === "approved"
726826 ? null
727827 : thread === undefined
728 ? { kind: "comment", comment }
828 ? // Nor is a remark with nothing in it: offering to hand an empty review
829 // to a thread promises work it does not describe.
830 body === null
831 ? null
832 : { kind: "comment", comment }
729833 : { kind: "thread", thread };
834 // One bar, two homes. Under a card with words in it, it is the row beneath
835 // them. A bodiless verdict has nothing above it, so a row reserved for an add
836 // button nobody can see until they hover is a hole — there it rides the header
837 // line instead, which keeps the affordance every sibling card offers.
838 const reactionBar = (
839 <PullRequestReactionBar
840 reactions={comment.reactions ?? []}
841 canReact={detail.capabilities.reactions === true}
842 subjectId={comment.id}
843 environmentId={environmentId}
844 reference={reference}
845 onRefresh={onRefresh}
846 {...(body === null ? {} : { className: "mt-2" })}
847 />
848 );
730849 return (
731850 <article
732851 key={comment.id}
......@@ -738,9 +857,12 @@ export function PullRequestSummaryTab({
738857 <span className="flex min-w-0 flex-1 items-center gap-2 text-xs text-muted-foreground">
739858 <CommentAuthor actor={comment.author} />
740859 <span>{formatRelativeTimeLabel(comment.createdAt)}</span>
741 {comment.reviewState ? (
860 {outcome ? (
861 <PullRequestReviewOutcomeBadge outcome={outcome} />
862 ) : comment.reviewState ? (
742863 <span>{reviewStateLabel(comment.reviewState)}</span>
743864 ) : null}
865 {body === null ? reactionBar : null}
744866 </span>
745867 {/* Review remarks only. A plain conversation comment is talk, not a finding,
746868 and offering to fix one would promise more than it says. */}
......@@ -771,16 +893,14 @@ export function PullRequestSummaryTab({
771893 <TooltipPopup side="top">{comment.path}</TooltipPopup>
772894 </Tooltip>
773895 ) : null}
774 <CommentBody className="mt-2" comment={comment} editing={commentEditing} />
775 <PullRequestReactionBar
776 className="mt-2"
777 reactions={comment.reactions ?? []}
778 canReact={detail.capabilities.reactions === true}
779 subjectId={comment.id}
780 environmentId={environmentId}
781 reference={reference}
782 onRefresh={onRefresh}
783 />
896 {/* A verdict usually carries no words, and an empty markdown block reads as
897 a card somebody forgot to fill in — the badge above already said it.
898 Kept where this reader may rewrite the remark: the pencil lives in here,
899 and hiding the block would take away the only way back to it. */}
900 {body === null && !commentEditing.canEdit(comment) ? null : (
901 <CommentBody className="mt-2" comment={comment} editing={commentEditing} />
902 )}
903 {body === null ? null : reactionBar}
784904 </article>
785905 );
786906 })}
apps/web/src/components/pullRequest/PullRequestTimelineTab.tsx+118
......@@ -27,9 +27,14 @@ import { formatRelativeTimeLabel } from "~/timestampFormat";
2727import { Button } from "../ui/button";
2828import { Collapsible, CollapsiblePanel, CollapsibleTrigger } from "../ui/collapsible";
2929import { toastManager } from "../ui/toast";
30import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip";
3031import {
3132 buildPullRequestTimeline,
3233 groupPullRequestTimelineConversations,
34 isPullRequestVerdictStale,
35 newestPullRequestCommitAt,
36 pullRequestReviewOutcome,
37 type PullRequestReviewOutcome,
3338 type PullRequestTimelineEvent,
3439} from "./pullRequestDetail.logic";
3540import { canEditPullRequestComment } from "./pullRequestEditing.logic";
......@@ -40,6 +45,10 @@ import {
4045 PullRequestActorAvatar,
4146 PullRequestDiffStat,
4247 PullRequestMetaLine,
48 PullRequestReviewOutcomeIcon,
49 pullRequestReviewOutcomeLabel,
50 pullRequestReviewOutcomeStaleLabel,
51 pullRequestReviewOutcomeToneClassName,
4352} from "./pullRequestPresentation";
4453
4554/** What every comment on the timeline needs to react; only the subject differs between them. */
......@@ -411,6 +420,100 @@ function LifecycleEvent({ event }: { event: PullRequestTimelineEvent }) {
411420 );
412421}
413422
423/**
424 * A verdict, as its own row rather than a line inside a collapsed conversation. It wears the
425 * reviewer's face on the rail and the verdict's own icon beside their name, so "approved" reads
426 * at a glance from the same place a merge or a commit does.
427 */
428function ReviewVerdictEvent({
429 event,
430 outcome,
431 stale,
432 cwd,
433 onOpen,
434 reactions,
435}: {
436 event: PullRequestTimelineEvent;
437 outcome: PullRequestReviewOutcome;
438 /** Commits landed after this verdict, so it speaks for code the branch no longer has. */
439 stale: boolean;
440 cwd: string;
441 onOpen: (url: string) => void;
442 reactions: ReactionSurface;
443}) {
444 return (
445 <div className="group relative mb-5 pl-12 [contain-intrinsic-block-size:48px] [content-visibility:auto]">
446 {/* Pinned rather than centred: this row grows with a body and a reaction bar, and a
447 centred avatar drifts down beside them instead of sitting by the name. */}
448 <ActorTimelineMarker
449 actors={event.actor ? [event.actor] : []}
450 className="top-6"
451 fallback={<PullRequestReviewOutcomeIcon outcome={outcome} />}
452 />
453 <div className="flex min-w-0 items-start gap-2 py-1.5">
454 <div className="min-w-0 flex-1">
455 <div className="flex min-w-0 flex-wrap items-center gap-x-1.5 gap-y-1 text-xs">
456 <ActorName actor={event.actor} />
457 {/* The word alone, in the verdict's own colour — green for an approval, red for a
458 request for changes. A verdict overtaken by later commits keeps its word and
459 loses that colour: it still happened, and it no longer speaks for what is on the
460 branch. Lowercased in the styling rather than the string, so what a screen reader
461 announces stays the label every other surface uses. */}
462 <Tooltip disabled={!stale}>
463 <TooltipTrigger
464 render={
465 <span
466 className={cn(
467 "font-medium lowercase",
468 stale
469 ? "text-muted-foreground opacity-70"
470 : pullRequestReviewOutcomeToneClassName(outcome),
471 )}
472 />
473 }
474 >
475 {pullRequestReviewOutcomeLabel(outcome)}
476 {stale ? <span className="sr-only">, before the latest commits</span> : null}
477 </TooltipTrigger>
478 <TooltipPopup>{pullRequestReviewOutcomeStaleLabel(outcome)}</TooltipPopup>
479 </Tooltip>
480 </div>
481 {/* The reaction bar rides this line rather than taking one of its own. Its add button
482 is invisible until hovered but still occupies `h-6`, and under a verdict — usually a
483 single line with no body — a row of that reserved on its own reads as a hole. */}
484 <div className="mt-0.5 flex min-w-0 flex-wrap items-center gap-x-2 gap-y-1">
485 <PullRequestMetaLine className="flex-wrap text-[11px] text-muted-foreground">
486 <span>{formatRelativeTimeLabel(event.at)}</span>
487 {event.path ? (
488 <span className="inline-flex min-w-0 items-center gap-1">
489 <FileCode2Icon aria-hidden className="size-3 shrink-0" />
490 <span className="truncate">{event.path}</span>
491 </span>
492 ) : null}
493 </PullRequestMetaLine>
494 {reactions.canReact || event.reactions.length > 0 ? (
495 <PullRequestReactionBar
496 reactions={event.reactions}
497 canReact={reactions.canReact}
498 subjectId={event.id}
499 environmentId={reactions.environmentId}
500 reference={reactions.reference}
501 onRefresh={reactions.onRefresh}
502 />
503 ) : null}
504 </div>
505 {/* An approval usually carries no words. When it does they are the review, so they stay
506 visible rather than being folded away with the ordinary conversation. */}
507 {event.body ? (
508 <TimelineBody body={event.body} markdown={event.markdown} cwd={cwd} />
509 ) : null}
510 </div>
511 <OpenOnHostButton url={event.url} onOpen={onOpen} />
512 </div>
513 </div>
514 );
515}
516
414517export function PullRequestTimelineTab({
415518 detail,
416519 environmentId,
......@@ -427,6 +530,7 @@ export function PullRequestTimelineTab({
427530 onRefresh: () => void;
428531}) {
429532 const events = buildPullRequestTimeline(detail);
533 const newestCommitAt = newestPullRequestCommitAt(detail.commits);
430534 const reactions: ReactionSurface = {
431535 canReact: detail.capabilities.reactions === true,
432536 environmentId,
......@@ -468,6 +572,20 @@ export function PullRequestTimelineTab({
468572 if (event.kind === "commit") {
469573 return <CommitEvent key={event.id} event={event} onOpen={onOpenCommit} />;
470574 }
575 const outcome = pullRequestReviewOutcome(event.reviewState);
576 if (outcome !== null) {
577 return (
578 <ReviewVerdictEvent
579 key={event.id}
580 event={event}
581 outcome={outcome}
582 stale={isPullRequestVerdictStale(event.at, newestCommitAt)}
583 cwd={detail.workspaceRoot}
584 onOpen={openOnHost}
585 reactions={reactions}
586 />
587 );
588 }
471589 return <LifecycleEvent key={event.id} event={event} />;
472590 })}
473591 </div>
apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts+210
......@@ -16,7 +16,10 @@ import {
1616 groupPullRequestTimelineConversations,
1717 handoffPrompt,
1818 handoffReviewComments,
19 isPullRequestVerdictStale,
1920 isThreadOwnPullRequest,
21 latestPullRequestReviewOutcomes,
22 newestPullRequestCommitAt,
2023 mergePullRequestThreadComments,
2124 orderPullRequestComments,
2225 pullRequestActionMenuHasGroup,
......@@ -24,6 +27,7 @@ import {
2427 pullRequestComposerTarget,
2528 pullRequestFindingKey,
2629 pullRequestHandoffLabels,
30 pullRequestReviewOutcome,
2731 readableFailure,
2832 shouldRefreshPullRequestActivity,
2933 resolveBaseFreshness,
......@@ -177,6 +181,171 @@ describe("ordering comments", () => {
177181 });
178182});
179183
184describe("review verdicts", () => {
185 it("reads the same three verdicts however a host spells them", () => {
186 expect(pullRequestReviewOutcome("APPROVED")).toBe("approved");
187 expect(pullRequestReviewOutcome("approved")).toBe("approved");
188 expect(pullRequestReviewOutcome("CHANGES_REQUESTED")).toBe("changes-requested");
189 expect(pullRequestReviewOutcome("changes_requested")).toBe("changes-requested");
190 expect(pullRequestReviewOutcome("DISMISSED")).toBe("dismissed");
191 });
192
193 it("is not a verdict where the review only carried remarks", () => {
194 expect(pullRequestReviewOutcome("COMMENTED")).toBeNull();
195 expect(pullRequestReviewOutcome("PENDING")).toBeNull();
196 expect(pullRequestReviewOutcome(null)).toBeNull();
197 });
198
199 it("keeps each reviewer's last word, whatever order the host returned them in", () => {
200 const review = (
201 id: string,
202 login: string,
203 reviewState: string,
204 createdAt: string,
205 ): PullRequestComment => ({
206 id,
207 kind: "review",
208 author: { login, name: null, avatarUrl: null },
209 body: "",
210 createdAt,
211 url: null,
212 path: null,
213 reviewState,
214 });
215
216 expect(
217 latestPullRequestReviewOutcomes([
218 review("r3", "bilal", "APPROVED", "2026-07-03T00:00:00Z"),
219 review("r1", "bilal", "CHANGES_REQUESTED", "2026-07-01T00:00:00Z"),
220 review("r2", "octocat", "CHANGES_REQUESTED", "2026-07-02T00:00:00Z"),
221 // Not a verdict, so it neither adds a reviewer nor overwrites one.
222 review("r4", "octocat", "COMMENTED", "2026-07-04T00:00:00Z"),
223 ]).map((entry) => [entry.actor?.login, entry.outcome]),
224 ).toEqual([
225 ["bilal", "approved"],
226 ["octocat", "changes-requested"],
227 ]);
228 });
229
230 it("keeps two deleted accounts apart rather than counting them as one reviewer", () => {
231 expect(
232 latestPullRequestReviewOutcomes([
233 {
234 ...TIMELINE_SOURCE.comments[0]!,
235 id: "r1",
236 kind: "review",
237 author: null,
238 reviewState: "APPROVED",
239 createdAt: "2026-07-01T00:00:00Z",
240 },
241 {
242 ...TIMELINE_SOURCE.comments[0]!,
243 id: "r2",
244 kind: "review",
245 author: null,
246 reviewState: "APPROVED",
247 createdAt: "2026-07-02T00:00:00Z",
248 },
249 ]),
250 ).toHaveLength(2);
251 });
252
253 it("gives every entry a key that separates the reviewers it kept apart", () => {
254 const entries = latestPullRequestReviewOutcomes([
255 {
256 ...TIMELINE_SOURCE.comments[0]!,
257 id: "r1",
258 kind: "review",
259 author: null,
260 reviewState: "APPROVED",
261 createdAt: "2026-07-01T00:00:00Z",
262 },
263 {
264 ...TIMELINE_SOURCE.comments[0]!,
265 id: "r2",
266 kind: "review",
267 author: null,
268 reviewState: "APPROVED",
269 createdAt: "2026-07-01T00:00:00Z",
270 },
271 ]);
272 // Same author (none) and the same instant, so only the review's own id tells them apart.
273 expect(new Set(entries.map((entry) => entry.key)).size).toBe(2);
274 });
275
276 it("calls a verdict stale once commits land after it, and current before that", () => {
277 const commits = [
278 { oid: "c0ffee", messageHeadline: "later work", committedDate: "2026-07-05T00:00:00Z" },
279 ];
280 const review = (createdAt: string): PullRequestComment => ({
281 ...TIMELINE_SOURCE.comments[0]!,
282 kind: "review",
283 reviewState: "APPROVED",
284 createdAt,
285 });
286
287 expect(
288 latestPullRequestReviewOutcomes([review("2026-07-01T00:00:00Z")], commits)[0]?.stale,
289 ).toBe(true);
290 expect(
291 latestPullRequestReviewOutcomes([review("2026-07-06T00:00:00Z")], commits)[0]?.stale,
292 ).toBe(false);
293 // Nothing to be overtaken by, so nothing is stale.
294 expect(latestPullRequestReviewOutcomes([review("2026-07-01T00:00:00Z")], [])[0]?.stale).toBe(
295 false,
296 );
297 });
298
299 it("measures staleness against the newest commit, not the last one listed", () => {
300 expect(
301 newestPullRequestCommitAt([
302 { oid: "a", messageHeadline: "", committedDate: "2026-07-09T00:00:00Z" },
303 { oid: "b", messageHeadline: "", committedDate: "2026-07-02T00:00:00Z" },
304 ]),
305 ).toBe("2026-07-09T00:00:00Z");
306 expect(newestPullRequestCommitAt([])).toBeNull();
307 });
308
309 it("orders instants rather than their text, so a UTC offset cannot invert them", () => {
310 // 01:00+02:00 is 23:00 the previous day, so as text it sorts after the Z stamp and in time
311 // it falls well before it.
312 expect(
313 newestPullRequestCommitAt([
314 { oid: "a", messageHeadline: "", committedDate: "2026-07-05T00:30:00Z" },
315 { oid: "b", messageHeadline: "", committedDate: "2026-07-05T01:00:00+02:00" },
316 ]),
317 ).toBe("2026-07-05T00:30:00Z");
318 expect(isPullRequestVerdictStale("2026-07-05T00:30:00Z", "2026-07-05T01:00:00+02:00")).toBe(
319 false,
320 );
321 // A timestamp nothing can parse is not a position, so it settles nothing either way.
322 expect(isPullRequestVerdictStale("2026-07-01T00:00:00Z", "not a date")).toBe(false);
323 expect(
324 newestPullRequestCommitAt([{ oid: "a", messageHeadline: "", committedDate: "not a date" }]),
325 ).toBeNull();
326 });
327
328 it("shows nothing for a reviewer whose verdict was dismissed", () => {
329 expect(
330 latestPullRequestReviewOutcomes([
331 {
332 ...TIMELINE_SOURCE.comments[0]!,
333 kind: "review",
334 reviewState: "APPROVED",
335 createdAt: "2026-07-01T00:00:00Z",
336 },
337 {
338 ...TIMELINE_SOURCE.comments[0]!,
339 id: "c2",
340 kind: "review",
341 reviewState: "DISMISSED",
342 createdAt: "2026-07-02T00:00:00Z",
343 },
344 ]),
345 ).toEqual([]);
346 });
347});
348
180349describe("pull request timeline", () => {
181350 it("orders creation, commits and comments newest first", () => {
182351 // What happened last is what the reader opening the tab is asking about.
......@@ -321,6 +490,47 @@ describe("pull request timeline", () => {
321490 ["event", "created"],
322491 ]);
323492 });
493
494 it("keeps a verdict out of the collapsed conversation it was submitted in", () => {
495 const events = buildPullRequestTimeline({
496 ...TIMELINE_SOURCE,
497 comments: [
498 { ...TIMELINE_SOURCE.comments[0]!, id: "chatter-1", createdAt: "2026-07-05T00:00:00Z" },
499 {
500 ...TIMELINE_SOURCE.comments[0]!,
501 id: "approval",
502 kind: "review",
503 body: "",
504 reviewState: "APPROVED",
505 createdAt: "2026-07-04T00:00:00Z",
506 },
507 { ...TIMELINE_SOURCE.comments[0]!, id: "chatter-2", createdAt: "2026-07-03T00:00:00Z" },
508 // A review without a verdict is ordinary conversation and still groups.
509 {
510 ...TIMELINE_SOURCE.comments[0]!,
511 id: "remark",
512 kind: "review",
513 reviewState: "COMMENTED",
514 createdAt: "2026-07-02T12:00:00Z",
515 },
516 ],
517 });
518
519 const rows = groupPullRequestTimelineConversations(events);
520 expect(
521 rows.map((row) =>
522 row.kind === "comments"
523 ? [row.kind, ...row.events.map((event) => event.id)]
524 : [row.kind, row.event.id],
525 ),
526 ).toEqual([
527 ["comments", "chatter-1"],
528 ["event", "approval"],
529 ["comments", "chatter-2", "remark"],
530 ["event", "1baf7bdcafe"],
531 ["event", "created"],
532 ]);
533 });
324534});
325535
326536describe("fix findings handoff", () => {
apps/web/src/components/pullRequest/pullRequestDetail.logic.ts+127-2
......@@ -4,6 +4,7 @@ import type {
44 PullRequestBaseComparison,
55 PullRequestCheck,
66 PullRequestComment,
7 PullRequestCommit,
78 PullRequestDetailView,
89 PullRequestMergeability,
910 PullRequestReaction,
......@@ -117,6 +118,123 @@ export function orderPullRequestComments<T extends { readonly createdAt: string
117118 return order === "newest" ? comments.toReversed() : comments;
118119}
119120
121/** A review that says something about the change itself, rather than only carrying remarks. */
122export type PullRequestReviewOutcome = "approved" | "changes-requested" | "dismissed";
123
124/**
125 * Which review states are a verdict. Hosts spell the same three differently — GitHub reports
126 * `CHANGES_REQUESTED`, Bitbucket `changes_requested` — so case and separator are ignored, and
127 * anything else (GitHub's `COMMENTED`, a state no host here reports yet) is not a verdict.
128 */
129export function pullRequestReviewOutcome(
130 reviewState: string | null,
131): PullRequestReviewOutcome | null {
132 switch (reviewState?.trim().toLowerCase().replaceAll("_", "-")) {
133 case "approved":
134 return "approved";
135 case "changes-requested":
136 return "changes-requested";
137 case "dismissed":
138 return "dismissed";
139 default:
140 return null;
141 }
142}
143
144/**
145 * An instant as a number, because the text is not the order. Every host returns ISO-8601 but not
146 * all of them in UTC, and `2026-07-05T01:00:00+02:00` sorts after `2026-07-05T00:30:00Z` as text
147 * while falling an hour and a half before it in time. NaN for anything unparseable, which every
148 * caller treats as "cannot say" rather than as a position.
149 */
150function instant(iso: string): number {
151 return Date.parse(iso);
152}
153
154/**
155 * The newest commit on the branch, which is what a verdict is current against. Null where the
156 * host reported no commits — or none with a timestamp that parses — since nothing can then be
157 * said to predate them.
158 */
159export function newestPullRequestCommitAt(
160 commits: ReadonlyArray<PullRequestCommit>,
161): string | null {
162 let newest: string | null = null;
163 let newestAt = Number.NEGATIVE_INFINITY;
164 for (const commit of commits) {
165 const at = instant(commit.committedDate);
166 if (Number.isNaN(at) || at <= newestAt) continue;
167 newest = commit.committedDate;
168 newestAt = at;
169 }
170 return newest;
171}
172
173/**
174 * Whether a verdict was given before the code it was given on.
175 *
176 * Measured against commit dates, which is the only thing the detail carries. That is a proxy and
177 * not the question: a commit date says when the work was written, not when it reached this change
178 * request, so pushing a branch of older commits after an approval leaves the approval reading as
179 * current, and a rebase re-dates commits a verdict already covered. Answering it exactly needs
180 * the host's own review-to-commit link — GitHub hangs a commit off every review — which no
181 * adapter reads yet. Until one does, this errs towards leaving a verdict alone: it dims only
182 * where the branch plainly moved on.
183 */
184export function isPullRequestVerdictStale(at: string, newestCommitAt: string | null): boolean {
185 if (newestCommitAt === null) return false;
186 const verdictAt = instant(at);
187 const commitAt = instant(newestCommitAt);
188 return !Number.isNaN(verdictAt) && !Number.isNaN(commitAt) && verdictAt < commitAt;
189}
190
191export interface PullRequestReviewOutcomeEntry {
192 /**
193 * What made this entry its own reviewer. A login where the host reported one, and otherwise the
194 * review's own id — so a surface listing these has a key that separates the same two authorless
195 * verdicts this does, rather than collapsing them back into one row.
196 */
197 readonly key: string;
198 readonly actor: PullRequestActor | null;
199 readonly outcome: PullRequestReviewOutcome;
200 readonly at: string;
201 /** Commits landed after this verdict, so it speaks for code that is no longer on the branch. */
202 readonly stale: boolean;
203}
204
205/**
206 * Where each reviewer landed, which is what "is this approved?" actually asks. One entry per
207 * person and only their last word: a host keeps every review somebody ever submitted, and an
208 * approval later followed by a request for changes is not an approval any more. A dismissal is a
209 * verdict taken back, so it leaves nothing to show rather than showing itself.
210 */
211export function latestPullRequestReviewOutcomes(
212 comments: ReadonlyArray<PullRequestComment>,
213 /** Left empty by a caller with no commits to hand, which makes no verdict stale. */
214 commits: ReadonlyArray<PullRequestCommit> = [],
215): ReadonlyArray<PullRequestReviewOutcomeEntry> {
216 const newestCommitAt = newestPullRequestCommitAt(commits);
217 const latest = new Map<string, PullRequestReviewOutcomeEntry>();
218 for (const comment of comments) {
219 const outcome = pullRequestReviewOutcome(comment.reviewState);
220 if (outcome === null) continue;
221 // Two deleted accounts are two reviewers. Keying both as "ghost" would let one overwrite the
222 // other and undercount the verdicts, so a review with no author identity stands alone.
223 const login = comment.author?.login ?? `ghost:${comment.id}`;
224 const current = latest.get(login);
225 // Not every host returns its reviews in order, so the newest wins rather than the last read.
226 if (current !== undefined && instant(current.at) > instant(comment.createdAt)) continue;
227 latest.set(login, {
228 key: login,
229 actor: comment.author,
230 outcome,
231 at: comment.createdAt,
232 stale: isPullRequestVerdictStale(comment.createdAt, newestCommitAt),
233 });
234 }
235 return [...latest.values()].filter((entry) => entry.outcome !== "dismissed");
236}
237
120238export interface PullRequestTimelineEvent {
121239 readonly id: string;
122240 readonly at: string;
......@@ -146,13 +264,20 @@ export type PullRequestTimelineRow =
146264 * Consecutive comments are one conversation section. Commits and pull-request lifecycle updates
147265 * stay first-class rows and split those sections, so expanding a conversation never hides the
148266 * work that happened between two review rounds.
267 *
268 * A verdict is a first-class row too. Whether the change was approved is the question a reader
269 * opens the timeline with, and folding the answer into a collapsed "9 comments" section hides it
270 * behind a press — the one thing on the page that must be readable without one.
149271 */
150272export function groupPullRequestTimelineConversations(
151273 events: ReadonlyArray<PullRequestTimelineEvent>,
152274): ReadonlyArray<PullRequestTimelineRow> {
153275 const rows: PullRequestTimelineRow[] = [];
154276 for (const event of events) {
155 if (event.kind === "comment" || event.kind === "review") {
277 if (
278 (event.kind === "comment" || event.kind === "review") &&
279 pullRequestReviewOutcome(event.reviewState) === null
280 ) {
156281 const last = rows.at(-1);
157282 if (last?.kind === "comments") {
158283 rows[rows.length - 1] = { kind: "comments", events: [...last.events, event] };
......@@ -172,7 +297,7 @@ export function groupPullRequestTimelineConversations(
172297 * at all. The stripped text decides that and nothing else: the body itself is passed on whole,
173298 * because a comment demonstrating an HTML comment inside a code fence still has to show it.
174299 */
175function visibleBody(body: string): string | null {
300export function visibleBody(body: string): string | null {
176301 return body.replace(/<!--[\s\S]*?-->/gu, "").trim().length === 0 ? null : body.trim();
177302}
178303
apps/web/src/components/pullRequest/pullRequestPresentation.tsx+113
......@@ -22,7 +22,9 @@ import { Children, isValidElement, type ReactNode } from "react";
2222
2323import { cn } from "~/lib/utils";
2424
25import { Badge } from "../ui/badge";
2526import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip";
27import type { PullRequestReviewOutcome } from "./pullRequestDetail.logic";
2628
2729interface StatePresentation {
2830 readonly label: string;
......@@ -191,6 +193,117 @@ export function pullRequestChecksState(
191193 return statuses.includes("success") ? "passing" : null;
192194}
193195
196/**
197 * How a verdict reads, in the one place every surface takes it from. The green is the green a
198 * passing check already wears in the same panel, so "approved" and "all checks passed" cannot
199 * look like two different kinds of good news.
200 *
201 * The ring runs a shade stronger than the text tones. At 16px across it is a thin arc, and the
202 * muted pairing that reads well as a word was barely there as an outline.
203 */
204const REVIEW_OUTCOME_PRESENTATION = {
205 approved: {
206 label: "Approved",
207 Icon: CircleCheckIcon,
208 toneClassName: "text-emerald-600 dark:text-emerald-300/90",
209 ringClassName: "ring-2 ring-emerald-500 dark:ring-emerald-400",
210 staleRingClassName:
211 "ring-2 ring-[color-mix(in_srgb,var(--color-emerald-500)_35%,var(--background))] dark:ring-[color-mix(in_srgb,var(--color-emerald-400)_35%,var(--background))]",
212 badgeVariant: "success",
213 },
214 "changes-requested": {
215 label: "Changes requested",
216 Icon: CircleXIcon,
217 toneClassName: "text-destructive",
218 ringClassName: "ring-2 ring-destructive",
219 staleRingClassName: "ring-2 ring-[color-mix(in_srgb,var(--destructive)_35%,var(--background))]",
220 badgeVariant: "error",
221 },
222 dismissed: {
223 label: "Review dismissed",
224 Icon: CircleDashedIcon,
225 toneClassName: "text-muted-foreground/70",
226 ringClassName: "ring-2 ring-muted-foreground/60",
227 staleRingClassName:
228 "ring-2 ring-[color-mix(in_srgb,var(--muted-foreground)_30%,var(--background))]",
229 badgeVariant: "outline",
230 },
231} as const satisfies Record<
232 PullRequestReviewOutcome,
233 {
234 label: string;
235 Icon: typeof CircleCheckIcon;
236 toneClassName: string;
237 ringClassName: string;
238 staleRingClassName: string;
239 badgeVariant: "success" | "error" | "outline";
240 }
241>;
242
243export function pullRequestReviewOutcomeToneClassName(outcome: PullRequestReviewOutcome): string {
244 return REVIEW_OUTCOME_PRESENTATION[outcome].toneClassName;
245}
246
247/** Worn by whatever wraps a reviewer's avatar, so their verdict reads without a row of its own. */
248/**
249 * A faded verdict is mixed into the background rather than made translucent. The ring is the only
250 * separator an avatar carrying one has — the summary drops the opaque `ring-background` where a
251 * verdict is drawn — and the stack overlaps by 4px, so an alpha ring would let the neighbour show
252 * straight through it and the two faces would merge.
253 */
254export function pullRequestReviewOutcomeRingClassName(
255 outcome: PullRequestReviewOutcome,
256 stale = false,
257): string {
258 const presentation = REVIEW_OUTCOME_PRESENTATION[outcome];
259 return stale ? presentation.staleRingClassName : presentation.ringClassName;
260}
261
262/**
263 * What a superseded verdict says, which is the same word with when it applied added. Commits
264 * landed after it, so it stands for code the branch no longer has.
265 */
266export function pullRequestReviewOutcomeStaleLabel(outcome: PullRequestReviewOutcome): string {
267 return `${REVIEW_OUTCOME_PRESENTATION[outcome].label} earlier changes`;
268}
269
270/** Decorative: every caller says which verdict this is in words beside it. */
271export function PullRequestReviewOutcomeIcon({
272 outcome,
273 className,
274}: {
275 outcome: PullRequestReviewOutcome;
276 className?: string;
277}) {
278 const presentation = REVIEW_OUTCOME_PRESENTATION[outcome];
279 return (
280 <presentation.Icon
281 aria-hidden
282 className={cn("size-3.5 shrink-0", presentation.toneClassName, className)}
283 />
284 );
285}
286
287export function pullRequestReviewOutcomeLabel(outcome: PullRequestReviewOutcome): string {
288 return REVIEW_OUTCOME_PRESENTATION[outcome].label;
289}
290
291export function PullRequestReviewOutcomeBadge({
292 outcome,
293 className,
294}: {
295 outcome: PullRequestReviewOutcome;
296 className?: string;
297}) {
298 const presentation = REVIEW_OUTCOME_PRESENTATION[outcome];
299 return (
300 <Badge size="sm" variant={presentation.badgeVariant} className={cn("gap-1", className)}>
301 <presentation.Icon aria-hidden className="size-3" />
302 {presentation.label}
303 </Badge>
304 );
305}
306
194307export function PullRequestActorAvatar({
195308 actor,
196309 className,