Skip to content

Commit 35629e2

Browse files
committed
Simplify PR dashboard thread-classification heuristic and add Approved column
1 parent 6348644 commit 35629e2

1 file changed

Lines changed: 27 additions & 44 deletions

File tree

.github/scripts/pull-request-dashboard.py

Lines changed: 27 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -57,23 +57,22 @@
5757
- unclear: the thread does not contain enough information to decide
5858
5959
Guidance:
60-
- A reviewer saying "this works", "sounds good", or answering the author's
61-
question may still leave the next implementation step with the author.
62-
- An author saying "fixed", pushing a commit after feedback, or answering a
63-
reviewer question usually puts the thread back in the reviewer court.
64-
- If the author's latest comment asks the reviewer a question or requests
65-
reviewer input, classify the thread as reviewer.
66-
- If thread_facts.same_actor_approved_after_thread is true, use that only as
67-
supporting evidence that an optional suggestion or informational comment is
68-
non-blocking. Do not classify required follow-up as none just because the
69-
same actor later approved.
70-
- A reviewer sharing a reference, example, optional suggestion, or "for ideas"
71-
link without an explicit requested change is informational; classify it as
72-
none.
73-
- If the thread is merely informational and does not require action, classify
74-
it as none.
75-
- If the thread is purely social, for example "thanks", "LGTM", or "nice work",
76-
with no follow-up requested or implied, classify it as none.
60+
- Default heuristic: whoever commented last has passed the ball to the other
61+
side. If the latest comment is from a reviewer/approver, the author owes a
62+
response (classify as author). If the latest comment is from the author,
63+
the reviewer owes a response (classify as reviewer).
64+
- This applies even to optional suggestions, "for ideas" links, references,
65+
or links to a reviewer's own pull request / patch with proposed changes.
66+
The author still needs to acknowledge, accept, or push back.
67+
- Exceptions that map to none:
68+
- Purely social comments ("thanks", "LGTM", "nice work") with no follow-up
69+
requested or implied.
70+
- The reviewer's last comment is a clear acknowledgement of the author's
71+
previous reply ("sounds good", "ok thanks") that closes the thread.
72+
- Exception that keeps the ball with the author: if the author's latest
73+
comment is a self-deferral ("still working on it", "WIP", "I'll get to
74+
this", "will fix") rather than a question or completed reply, classify as
75+
author — they have not yet handed the ball back.
7776
7877
Respond with a single JSON object and nothing else:
7978
{{"thread_action": "author" | "reviewer" | "external" | "none" | "unclear", "reason": "short explanation grounded in this thread"}}
@@ -533,35 +532,13 @@ def thread_comment(timestamp: str, actor: str, author: str, reviewers: set[str],
533532
}
534533

535534

536-
def approver_approved_after_thread(raw: dict[str, Any], comments: list[dict[str, Any]]) -> bool:
537-
last_comment_ts = comments[-1]["timestamp"]
538-
thread_approvers = {
539-
c["actor"].lower()
540-
for c in comments
541-
if c["actor_role"] == "approver" and c.get("actor")
542-
}
543-
if not thread_approvers:
544-
return False
545-
for review in raw["reviews"]:
546-
reviewer = actor_login(review.get("user") or {}).lower()
547-
if reviewer not in thread_approvers:
548-
continue
549-
if review.get("state") != "APPROVED":
550-
continue
551-
if (review.get("submitted_at") or "") > last_comment_ts:
552-
return True
553-
return False
554-
555-
556535
def add_thread_facts(
557-
raw: dict[str, Any],
558536
thread: dict[str, Any],
559537
comments: list[dict[str, Any]],
560538
facts: dict[str, Any],
561539
) -> dict[str, Any]:
562540
thread["thread_facts"] = {
563541
"latest_comment_role": comments[-1].get("actor_role"),
564-
"same_actor_approved_after_thread": approver_approved_after_thread(raw, comments),
565542
"current_conflicts": facts.get("conflicts"),
566543
}
567544
return thread
@@ -585,7 +562,7 @@ def group_review_threads(
585562
comments.sort(key=lambda c: c["timestamp"])
586563
if not comments:
587564
continue
588-
threads.append(add_thread_facts(raw, {
565+
threads.append(add_thread_facts({
589566
"thread_id": thread.get("id") or f"review-thread-{len(threads) + 1}",
590567
"thread_kind": "review-comment-thread",
591568
"path": thread.get("path"),
@@ -641,7 +618,7 @@ def group_pr_conversation(
641618
selected = selected[-PR_COMMENT_WINDOW:]
642619
if not selected:
643620
return []
644-
return [add_thread_facts(raw, {
621+
return [add_thread_facts({
645622
"thread_id": "pr-conversation",
646623
"thread_kind": "pr-conversation",
647624
"path": None,
@@ -919,6 +896,12 @@ def conflicts_cell(facts: dict[str, Any]) -> str:
919896
return "?"
920897

921898

899+
def approved_cell(facts: dict[str, Any]) -> str:
900+
if "approved" not in facts:
901+
return "?"
902+
return "✅" if facts.get("approved") else " "
903+
904+
922905
def _html_escape(s: str) -> str:
923906
return (s or "").replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;")
924907

@@ -978,8 +961,8 @@ def render_markdown_compact(
978961
rows.sort(key=lambda p: -p["number"])
979962
out.append(f"## {SIDE_LABELS.get(side, side)}")
980963
out.append("")
981-
out.append("| PR | Author | CI | Conflicts | Activity |")
982-
out.append("|---|---|---|---|---|")
964+
out.append("| PR | Author | CI | Conflicts | Approved | Activity |")
965+
out.append("|---|---|---|---|---|---|")
983966
for pr in rows:
984967
number = pr["number"]
985968
title = _md_escape(pr.get("title", ""))
@@ -991,7 +974,7 @@ def render_markdown_compact(
991974
activity_cell = "?" if activity is None else f"{activity}d"
992975
out.append(
993976
f"| [{title}]({url}) | {author} | {ci_cell(facts)} | "
994-
f"{conflicts_cell(facts)} | {activity_cell} |"
977+
f"{conflicts_cell(facts)} | {approved_cell(facts)} | {activity_cell} |"
995978
)
996979
out.append("")
997980

0 commit comments

Comments
 (0)