Cs 10504/pr card should add approved or request changed by which reviewer by lucaslyl · Pull Request #4254 · cardstack/boxel

lucaslyl · 2026-03-26T12:59:16Z

linear: https://linear.app/cardstack/issue/CS-10504/pr-card-should-add-approved-or-request-changed-by-which-reviewer

Show reviewer name and comment for both approved and changes_requested reviews
Display "by {reviewer}" on the review state badge for PR cards.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4c5874316

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-26T13:01:26Z

packages/catalog-realm/pr-card/pr-card.gts

+    if (state === 'changes_requested') {
+      return findLatestChangesRequestedEvent(this.latestReviewByReviewer);
+    }


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 👍 / 👎.

github-actions · 2026-03-26T13:34:18Z

Host Test Results

1 files ±0 1 suites ±0 2h 8m 14s ⏱️ - 2m 13s
2 056 tests +5 2 041 ✅ +5 15 💤 ±0 0 ❌ ±0
2 071 runs +5 2 056 ✅ +5 15 💤 ±0 0 ❌ ±0

Results for commit 8a29db0. ± Comparison against base commit 724fc8b.

♻️ This comment has been updated with latest results.

Copilot

Pull request overview

Updates the catalog realm PR card review UI so it can attribute “Approved” / “Changes Requested” states to a specific reviewer and show the associated review details.

Changes:

Add support for selecting the latest approved review event (in addition to changes requested).
Update the PR card to surface reviewer name/comment/link for both approval and changes-requested states.
Update the review state badge to append “by {reviewer}” when available.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
`packages/catalog-realm/pr-card/utils.ts`	Adds helper to find the latest approved review event; adjusts changes-requested lookup.
`packages/catalog-realm/pr-card/pr-card.gts`	Chooses the latest relevant review event based on overall review state and passes reviewer/comment/link into the review section.
`packages/catalog-realm/pr-card/components/isolated/review-section.gts`	Extends badge args to include reviewer name and renders “by {reviewer}”.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-26T22:08:46Z

packages/catalog-realm/pr-card/components/isolated/review-section.gts

+        <ReviewStateBadge
+          @state={{@reviewState}}
+          @reviewerName={{@reviewerName}}
+        />


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").

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

Copilot · 2026-03-26T22:08:47Z

packages/catalog-realm/pr-card/utils.ts

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

+export function findLatestApprovedEvent(
+  latestReviewByReviewer: Map<string, any>,
+): any | null {
+  let approvedEvents = [...latestReviewByReviewer.values()].filter(
+    (event: any) =>
+      normalizeReviewState(event?.payload?.review?.state) === 'approved',
+  );
+  return approvedEvents[0] ?? null;
+}


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 · 2026-03-26T22:08:47Z

packages/catalog-realm/pr-card/pr-card.gts

+  get latestReviewerName() {
+    return (
+      this.latestPrReviewCommentEventInstance?.payload?.review?.user?.login ??
+      null


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'

same for this

Copilot · 2026-03-26T22:08:47Z

packages/catalog-realm/pr-card/pr-card.gts

  get latestPrReviewCommentEventInstance() {
-    return findLatestChangesRequestedEvent(this.latestReviewByReviewer);
-  }
-
-  get latestChangesRequestedReviewerName() {
-    return (
-      this.latestPrReviewCommentEventInstance?.payload?.review?.user?.login ??
-      'Unknown reviewer'
-    );
+    let state = this.latestReviewState;
+    if (state === 'changes_requested') {
+      return findLatestChangesRequestedEvent(this.latestReviewByReviewer);
+    }
+    if (state === 'approved') {
+      return findLatestApprovedEvent(this.latestReviewByReviewer);
+    }
+    return null;
  }


latestPrReviewCommentEventInstance can now return an approved review event as well as a changes-requested one. The name is now misleading and increases the chance of future mistakes (e.g., someone assuming it only exists when there’s a comment). Consider renaming this getter (and the dependent getters) to something like latestPrReviewEventInstance or similar.

tintinthong

test coverage pls

lucaslyl added 3 commits March 25, 2026 18:13

add reviewer on review section

874a877

show approve comment too

45cb177

update css

d4c5874

lucaslyl requested a review from a team March 26, 2026 12:59

chatgpt-codex-connector bot reviewed Mar 26, 2026

View reviewed changes

fix codex feedback

58f84ae

habdelra requested a review from Copilot March 26, 2026 22:03

Copilot started reviewing on behalf of habdelra March 26, 2026 22:04 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

fix copilot feedback

8a29db0

tintinthong requested changes Mar 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Cs 10504/pr card should add approved or request changed by which reviewer#4254
lucaslyl wants to merge 5 commits intomainfrom
CS-10504/pr-card-should-add-approved-or-request-changed-by-which-reviewer

lucaslyl commented Mar 26, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 26, 2026

Uh oh!

github-actions bot commented Mar 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

lucaslyl Mar 27, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

lucaslyl Mar 27, 2026

Uh oh!

Copilot AI Mar 26, 2026

Uh oh!

tintinthong left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lucaslyl commented Mar 26, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Host Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

lucaslyl Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

lucaslyl Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

tintinthong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 26, 2026 •

edited

Loading