Skip to content

Cs 10504/pr card should add approved or request changed by which reviewer #4254

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

Open
lucaslyl wants to merge 5 commits into
base: main
Choose a base branch
from
+55 −31
Open

Cs 10504/pr card should add approved or request changed by which reviewer #4254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
874a877
add reviewer on review section
lucaslyl Mar 25, 2026
45cb177
show approve comment too
lucaslyl Mar 26, 2026
d4c5874
update css
lucaslyl Mar 26, 2026
58f84ae
fix codex feedback
lucaslyl Mar 26, 2026
8a29db0
fix copilot feedback
lucaslyl Mar 27, 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
16 changes: 12 additions & 4 deletions packages/catalog-realm/pr-card/components/isolated/review-section.gts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import type { ReviewState } from '../../utils';
// ── Sub-components ──────────────────────────────────────────────────────

interface ReviewStateBadgeSignature {
Args: { state: ReviewState };
Args: {
state: ReviewState;
reviewerName: string | null;
};
}

class ReviewStateBadge extends GlimmerComponent<ReviewStateBadgeSignature> {
Expand Down Expand Up @@ -34,7 +37,9 @@ class ReviewStateBadge extends GlimmerComponent<ReviewStateBadgeSignature> {

<template>
{{#if this.hasState}}
<span class='review-state-badge {{this.stateClass}}'>{{this.label}}</span>
<span class='review-state-badge {{this.stateClass}}'>{{this.label}}{{#if
@reviewerName
}} by {{@reviewerName}}{{/if}}</span>
{{/if}}

<style scoped>
Expand Down Expand Up @@ -88,7 +93,7 @@ class ReviewStateBadge extends GlimmerComponent<ReviewStateBadgeSignature> {
interface ReviewSectionSignature {
Args: {
reviewState: ReviewState;
reviewerName: string;
reviewerName: string | null;
comment: string;
reviewUrl: string | undefined;
hasReview: boolean;
Expand All @@ -113,7 +118,10 @@ export class ReviewSection extends GlimmerComponent<ReviewSectionSignature> {
<div class='review-section'>
<div class='review-heading-row'>
<h2 class='section-heading'>Reviews</h2>
<ReviewStateBadge @state={{@reviewState}} />
<ReviewStateBadge
@state={{@reviewState}}
@reviewerName={{@reviewerName}}
/>
Comment on lines +121 to +124
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewerName is now allowed to be null and is passed through to ReviewStateBadge. If null is possible in practice, the badge will omit the "by …" suffix and the rest of the section may also end up showing no author. Consider normalizing reviewerName to a non-empty display string before passing it into the section/badge (e.g. "Unknown reviewer").

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@lucaslyl lucaslyl Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When @reviewerName is null, Handlebars' {{#if null}} evaluates to falsy, so the by ... block doesn't render. The badge will only show "Approved" or "Changes Requested" without the "by" suffix.

Showing "Unknown reviewer" would make users think something is broken or the data is wrong

</div>

{{#if @hasReview}}
Expand Down
42 changes: 25 additions & 17 deletions packages/catalog-realm/pr-card/pr-card.gts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
buildLatestReviewByReviewer,
computeLatestReviewState,
findLatestChangesRequestedEvent,
findLatestApprovedEvent,
buildGithubEventCardRef,
searchEventQuery,
buildRealmHrefs,
Expand Down Expand Up @@ -167,29 +168,36 @@ class IsolatedTemplate extends Component<typeof PrCard> {
return computeLatestReviewState(this.latestReviewByReviewer);
}

get latestPrReviewCommentEventInstance() {
return findLatestChangesRequestedEvent(this.latestReviewByReviewer);
}

get latestChangesRequestedReviewerName() {
return (
this.latestPrReviewCommentEventInstance?.payload?.review?.user?.login ??
'Unknown reviewer'
);
get latestPrReviewEventInstance() {
let state = this.latestReviewState;
if (state === 'changes_requested') {
return findLatestChangesRequestedEvent(this.latestReviewByReviewer);
}
Comment on lines +173 to +175
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Derive reviewer badge from latest review state, not comment event

latestPrReviewCommentEventInstance is used as the source for reviewer attribution, and in the changes_requested branch it calls findLatestChangesRequestedEvent(...), which filters out reviews with empty bodies. In GitHub, a reviewer can request changes without a summary body, so this flow can produce a Changes Requested state while omitting the reviewer name in the new "by {reviewer}" badge (and review header). The reviewer for badge/state should come from the latest matching review state even when the comment body is empty.

Useful? React with 👍 / 👎.

if (state === 'approved') {
return findLatestApprovedEvent(this.latestReviewByReviewer);
}
return null;
}

get latestChangesRequestedComment() {
get latestReviewComment() {
let comment =
this.latestPrReviewCommentEventInstance?.payload?.review?.body?.trim();
this.latestPrReviewEventInstance?.payload?.review?.body?.trim();
return comment || '-';
}

get latestChangesRequestedReviewUrl() {
return this.latestPrReviewCommentEventInstance?.payload?.review?.html_url;
get latestReviewCommentUrl() {
return this.latestPrReviewEventInstance?.payload?.review?.html_url;
}

get hasReview() {
return !!this.latestPrReviewCommentEventInstance;
return !!this.latestPrReviewEventInstance;
}

get latestReviewerName() {
return (
this.latestPrReviewEventInstance?.payload?.review?.user?.login ??
null
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

latestReviewerName now falls back to null when the review payload is missing user.login. That makes the reviewer label/badge render blank (and also prevents showing "by …"), which undermines the goal of showing who approved/requested changes. Consider restoring a string fallback like "Unknown reviewer" (as before) or ensuring the UI renders a placeholder when the name is missing.

Suggested change
null
'Unknown reviewer'

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@lucaslyl lucaslyl Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for this

);
}

// ── Mergeability ──
Expand Down Expand Up @@ -256,9 +264,9 @@ class IsolatedTemplate extends Component<typeof PrCard> {
<hr class='status-divider' />
<ReviewSection
@reviewState={{this.latestReviewState}}
@reviewerName={{this.latestChangesRequestedReviewerName}}
@comment={{this.latestChangesRequestedComment}}
@reviewUrl={{this.latestChangesRequestedReviewUrl}}
@reviewerName={{this.latestReviewerName}}
@comment={{this.latestReviewComment}}
@reviewUrl={{this.latestReviewCommentUrl}}
@hasReview={{this.hasReview}}
/>
</section>
Expand Down
28 changes: 18 additions & 10 deletions packages/catalog-realm/pr-card/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@ export type CiGroup = {
items: CiItem[];
};

export type ReviewState =
| 'changes_requested'
| 'approved'
| 'unknown';
export type ReviewState = 'changes_requested' | 'approved' | 'unknown';

// ── PR State Helpers ─────────────────────────────────────────────────────

Expand Down Expand Up @@ -299,13 +296,24 @@ export function computeLatestReviewState(
export function findLatestChangesRequestedEvent(
latestReviewByReviewer: Map<string, any>,
): any | null {
let activeChangesRequestedEvents = [...latestReviewByReviewer.values()].filter(
(event: any) =>
normalizeReviewState(event?.payload?.review?.state) ===
'changes_requested' &&
(event?.payload?.review?.body ?? '').trim().length > 0,
return (
[...latestReviewByReviewer.values()].find(
(event: any) =>
normalizeReviewState(event?.payload?.review?.state) ===
'changes_requested',
) ?? null
);
}

export function findLatestApprovedEvent(
latestReviewByReviewer: Map<string, any>,
): any | null {
return (
[...latestReviewByReviewer.values()].find(
(event: any) =>
normalizeReviewState(event?.payload?.review?.state) === 'approved',
) ?? null
);
return activeChangesRequestedEvents[0] ?? null;
}
Comment on lines 296 to +317
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both findLatestChangesRequestedEvent and findLatestApprovedEvent build an intermediate array via filter(...)[0]. Since only the first match is used, Array.prototype.find would avoid the extra allocation and work (especially as the number of reviewers grows).

Copilot uses AI. Check for mistakes.

// ── Query Builders ───────────────────────────────────────────────────────
Expand Down
Loading