From ef2167f9c43611f06f26c17dfdc450535fc22968 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:30:30 -0700 Subject: [PATCH 1/5] Redesign PR dashboard status comment layout Replace the Waiting on/Next step label stack with a bold status headline plus inline freshness, state the ask as a sentence, and move the reroute/report/staleness guidance into a collapsible details footer. Bumps STATUS_COMMENT_REVISION to 14 to roll the new layout out to open PRs. --- .../pr_status_comment.py | 280 ++++++++++-------- .../route_presentation.py | 11 + .../test_pr_status_comment.py | 120 ++++---- 3 files changed, 219 insertions(+), 192 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 62b1dd6ff96..e303776d25e 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -12,8 +12,8 @@ gh_api, run_gh, ) -from dashboard_override import PRE_REVIEW_ROUTES, author_override_guidance -from route_presentation import route_status_summary +from dashboard_override import PRE_REVIEW_ROUTES +from route_presentation import route_status_summary, status_headline from state import ( load_dashboard_state_cache, load_status_comment_rollout_state, @@ -32,7 +32,7 @@ ) # Increment whenever render_status_comment changes in a way existing comments # need to adopt. Hourly runs durably roll the revision out to all open PRs. -STATUS_COMMENT_REVISION = 13 +STATUS_COMMENT_REVISION = 14 STATUS_COMMENT_ROLLOUT_BATCH_SIZE = 50 AUTHOR_ACTION_FEEDBACK_LINK_LIMIT = 20 NON_BLOCKING_CHECK_FAILURE_LIMIT = 20 @@ -44,8 +44,8 @@ "[Status comment truncated to keep this report link usable.]" ) AUTHOR_GUIDANCE = ( - "For each item, reply to move the discussion forward, e.g. link to the commit " - "that addresses it, explain why no change is needed, or ask a follow-up question." + "Reply to move it forward \u2014 for example, link the fixing commit, explain " + "why no change is needed, or ask a follow-up." ) DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" # Remove after migrating open PRs as described by the post-rollout @@ -97,44 +97,131 @@ def status_report_url(pr: dict[str, Any], status_comment: str) -> str: return f"{STATUS_REPORT_ISSUE_URL}?{query}" -def accuracy_note( +def _bounded_report_url(pr: dict[str, Any], status_comment: str) -> str: + report_url = status_report_url(pr, status_comment) + if len(report_url) <= STATUS_REPORT_URL_MAX_CHARS: + return report_url + lower_bound = 0 + upper_bound = len(status_comment) + while lower_bound <= upper_bound: + midpoint = (lower_bound + upper_bound) // 2 + truncated_status_comment = ( + f"{status_comment[:midpoint]}\n{STATUS_REPORT_TRUNCATION_NOTICE}" + ) + candidate_url = status_report_url(pr, truncated_status_comment) + if len(candidate_url) <= STATUS_REPORT_URL_MAX_CHARS: + report_url = candidate_url + lower_bound = midpoint + 1 + else: + upper_bound = midpoint - 1 + return report_url + + +def status_footer( pr: dict[str, Any], status_comment: str, + *, override_route: str = "", -) -> str: - report_url = status_report_url(pr, status_comment) - if len(report_url) > STATUS_REPORT_URL_MAX_CHARS: - lower_bound = 0 - upper_bound = len(status_comment) - while lower_bound <= upper_bound: - midpoint = (lower_bound + upper_bound) // 2 - truncated_status_comment = ( - f"{status_comment[:midpoint]}\n{STATUS_REPORT_TRUNCATION_NOTICE}" - ) - candidate_url = status_report_url(pr, truncated_status_comment) - if len(candidate_url) <= STATUS_REPORT_URL_MAX_CHARS: - report_url = candidate_url - lower_bound = midpoint + 1 - else: - upper_bound = midpoint - 1 - note = ( - "This automated status or its linked feedback items may be incorrect. " - f"If something looks wrong, please [report it]({report_url}) with the result you expected." - ) + terminal: bool = False, +) -> list[str]: + report_url = _bounded_report_url(pr, status_comment) + lines = [ + "
", + "Doesn't look right?", + "", + ] + if not terminal: + lines.append( + "- **Just replied or pushed?** Anything after the refresh time above " + "hasn't been picked up yet \u2014 give it a few minutes." + ) if override_route: - note += " " + author_override_guidance( - "If the last refreshed time above predates your latest reply or " - "push, the dashboard hasn't processed it yet.", - route=override_route, + lines.append( + "- **Should this be with reviewers?** Comment " + "`/dashboard route:reviewers` to route it to them." + ) + report_lead = ( + "Anything wrong \u2014 including the routing?" + if override_route + else "Anything look wrong?" + ) + lines.append( + f"- **{report_lead}** [Report it]({report_url}) with what you expected; " + "it helps us improve the dashboard." + ) + lines.extend(["", "
"]) + return lines + + +def non_blocking_failure_summary(non_blocking_check_failures: list[str]) -> str: + if not non_blocking_check_failures: + return "" + displayed_failures = non_blocking_check_failures[:NON_BLOCKING_CHECK_FAILURE_LIMIT] + names = format_list([ + markdown_escape(truncate(name, NON_BLOCKING_CHECK_FAILURE_NAME_LIMIT)) + for name in displayed_failures + ]) + if len(non_blocking_check_failures) == 1: + note = f"{names} is failing but is not a required check." + else: + note = f"{names} are failing but are not required checks." + omitted_count = len(non_blocking_check_failures) - len(displayed_failures) + if omitted_count: + noun = "failure" if omitted_count == 1 else "failures" + omitted_verb = "is" if omitted_count == 1 else "are" + note += ( + f" {omitted_count} additional non-blocking check {noun} " + f"{omitted_verb} not shown." + ) + return note + + +def author_body( + *, + feedback_count: int, + failing_count: int, + non_blocking_failure_note: str, + review_thread_urls: list[str], + top_level_feedback_urls: list[str], +) -> list[str]: + noun = "item" if feedback_count == 1 else "items" + if failing_count and feedback_count: + checks_bullet = "- **Required checks are failing** \u2014 investigate the failures." + if non_blocking_failure_note: + checks_bullet += f" Note: {non_blocking_failure_note}" + body = [ + "Two things need attention:", + checks_bullet, + f"- **{feedback_count} review {noun}** \u2014 address or respond:", + ] + body.extend( + feedback_breakdown_lines( + review_thread_urls, top_level_feedback_urls, indent=" " + ) + ) + body.extend(["", f"_{AUTHOR_GUIDANCE}_"]) + return body + if feedback_count: + body = [f"Address or respond to {feedback_count} review {noun}:"] + body.extend( + feedback_breakdown_lines(review_thread_urls, top_level_feedback_urls) ) - return f"_{note}_" + body.extend(["", f"_{AUTHOR_GUIDANCE}_"]) + return body + if failing_count: + sentence = "Investigate required status check failures." + if non_blocking_failure_note: + sentence += f" Note: {non_blocking_failure_note}" + return [sentence] + _, fallback_next_step = route_status_summary("author") + return [fallback_next_step] def render_status_comment( pr: dict[str, Any], result: dict[str, Any] | None, ) -> str: - last_updated = utc_now().strftime("%Y-%m-%d %H:%M:%S UTC") + last_updated = utc_now().strftime("%Y-%m-%d %H:%M UTC") state = (pr.get("state") or "").lower() facts = (result or {}).get("facts") or {} review_thread_urls = facts.get("author_action_review_thread_urls") or [] @@ -142,132 +229,80 @@ def render_status_comment( feedback_count = len(review_thread_urls) + len(top_level_feedback_urls) failing_count = facts.get("ci_failing_count", 0) non_blocking_check_failures = facts.get("non_blocking_check_failures") or [] - non_blocking_failure_note = "" - if non_blocking_check_failures: - displayed_failures = non_blocking_check_failures[ - :NON_BLOCKING_CHECK_FAILURE_LIMIT - ] - names = format_list([ - markdown_escape(truncate(name, NON_BLOCKING_CHECK_FAILURE_NAME_LIMIT)) - for name in displayed_failures - ]) - if len(non_blocking_check_failures) == 1: - non_blocking_failure_note = ( - f"{names} is failing but is not a required check." - ) - else: - non_blocking_failure_note = ( - f"{names} are failing but are not required checks." - ) - omitted_count = len(non_blocking_check_failures) - len(displayed_failures) - if omitted_count: - noun = "failure" if omitted_count == 1 else "failures" - omitted_verb = "is" if omitted_count == 1 else "are" - non_blocking_failure_note += ( - f" {omitted_count} additional non-blocking check {noun} " - f"{omitted_verb} not shown." - ) + non_blocking_failure_note = non_blocking_failure_summary(non_blocking_check_failures) - feedback_indent: str | None = None override_route = "" + terminal = False + body: list[str] = [] if pr.get("merged"): - summary = ["- **Status:** Merged."] + headline = "Merged" + terminal = True elif state == "closed": - summary = ["- **Status:** Closed."] + headline = "Closed" + terminal = True elif pr.get("draft"): - summary = [ - "- **Waiting on:** Author", - "- **Next step:** Move out of draft to request review.", - ] + headline = status_headline("author") + body = ["Move out of draft to request review."] elif result is None: - summary = [ - "- **Waiting on:** Pull request dashboard", - "- **Next step:** Finish refreshing this pull request.", - ] + headline = "Waiting on the pull request dashboard" + body = ["Finish refreshing this pull request."] else: route = result.get("route") or "unknown" if route in PRE_REVIEW_ROUTES: override_route = route + headline = status_headline(route) if route == "author": - waiting_on, fallback_next_step = route_status_summary(route) - check_action = None - if failing_count: - # One required aggregate check can represent multiple failing jobs. - check_action = "Investigate required status check failures." - if non_blocking_failure_note: - check_action += f" Note: {non_blocking_failure_note}" - noun = "item" if feedback_count == 1 else "items" - feedback_action = f"Address or respond to {feedback_count} review feedback {noun}:" - if check_action and feedback_count: - summary = [ - f"- **Waiting on:** {waiting_on}", - "- **Next steps:**", - f" - {check_action}", - f" - {feedback_action}", - ] - feedback_indent = " " - elif feedback_count: - summary = [ - f"- **Waiting on:** {waiting_on}", - f"- **Next step:** {feedback_action}", - ] - feedback_indent = " " - elif check_action: - summary = [ - f"- **Waiting on:** {waiting_on}", - f"- **Next step:** {check_action}", - ] - else: - summary = [ - f"- **Waiting on:** {waiting_on}", - f"- **Next step:** {fallback_next_step}", - ] + body = author_body( + feedback_count=feedback_count, + failing_count=failing_count, + non_blocking_failure_note=non_blocking_failure_note, + review_thread_urls=review_thread_urls, + top_level_feedback_urls=top_level_feedback_urls, + ) else: - waiting_on, next_step = route_status_summary(route) - summary = [ - f"- **Waiting on:** {waiting_on}", - f"- **Next step:** {next_step}", - ] + _, next_step = route_status_summary(route) + body = [next_step] if failing_count: check_summary = ( "1 required status check is failing." if failing_count == 1 else f"{failing_count} required status checks are failing." ) - summary.append(f"- **Also blocked by:** {check_summary}") + body.extend(["", f"**Also blocked by:** {check_summary}"]) if non_blocking_failure_note: label = ( "Non-blocking check failure" if len(non_blocking_check_failures) == 1 else "Non-blocking check failures" ) - summary.append(f"- **{label}:** {non_blocking_failure_note}") + body.extend(["", f"**{label}:** {non_blocking_failure_note}"]) lines = [ STATUS_MARKER, f"", "## Pull request dashboard status", "", - f"_Status last refreshed: {last_updated}._", - "", - *summary, + f"**{headline}** \u00b7 refreshed {last_updated}", ] episode_id = str(facts.get("author_nudge_episode_id") or "") if (result or {}).get("route") == "author" and episode_id: lines.insert(2, author_nudge_episode_marker(episode_id)) - if feedback_indent is not None and feedback_count: - lines.extend( - feedback_breakdown_lines( - review_thread_urls, - top_level_feedback_urls, - feedback_indent, - ) - ) + if body: + lines.append("") + lines.extend(body) + status_comment = "\n".join(lines) lines.append("") - lines.append(accuracy_note(pr, status_comment, override_route)) + lines.extend( + status_footer( + pr, + status_comment, + override_route=override_route, + terminal=terminal, + ) + ) lines.append("") return "\n".join(lines) @@ -283,12 +318,12 @@ def format_list(values: list[str]) -> str: def feedback_breakdown_lines( review_thread_urls: list[str], top_level_feedback_urls: list[str], - indent: str, + indent: str = "", ) -> list[str]: feedback_count = len(review_thread_urls) + len(top_level_feedback_urls) sections = ( ("Inline threads", review_thread_urls), - ("Top-level feedback", top_level_feedback_urls), + ("Top-level threads", top_level_feedback_urls), ) lines: list[str] = [] remaining_limit = AUTHOR_ACTION_FEEDBACK_LINK_LIMIT @@ -309,7 +344,6 @@ def feedback_breakdown_lines( f"{indent}- _Showing {shown} of {feedback_count} feedback links; " "resolve the remaining items from the pull request's conversation._" ) - lines.append(f"{indent}- _{AUTHOR_GUIDANCE}_") return lines diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 20b204aa06d..e3068c3830c 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -4,36 +4,43 @@ ROUTE_PRESENTATION = { "maintainer": { "dashboard_label": "Waiting on maintainers", + "status_headline": "Waiting on maintainers", "status_waiting_on": "Maintainers", "status_next_step": "Merge when ready.", }, "copilot": { "dashboard_label": "Waiting on Copilot", + "status_headline": "Waiting on Copilot", "status_waiting_on": "Copilot", "status_next_step": "Wait for the pending review to complete.", }, "approver": { "dashboard_label": "Waiting on reviewers", + "status_headline": "Waiting on reviewers", "status_waiting_on": "Reviewers", "status_next_step": "Review the latest changes.", }, "external": { "dashboard_label": "Waiting on external", + "status_headline": "Waiting on an external dependency or decision", "status_waiting_on": "An external dependency or decision", "status_next_step": "Resolve it before work can continue.", }, "author": { "dashboard_label": "Waiting on authors", + "status_headline": "Waiting on the author", "status_waiting_on": "Author", "status_next_step": "Address or respond to review feedback.", }, "transient-failure": { "dashboard_label": "Transient GitHub failure retrieving PR data", + "status_headline": "Waiting on the pull request dashboard maintainers", "status_waiting_on": "Pull request dashboard maintainers", "status_next_step": "Determine the next action.", }, "unknown": { "dashboard_label": "Unknown", + "status_headline": "Waiting on the pull request dashboard maintainers", "status_waiting_on": "Pull request dashboard maintainers", "status_next_step": "Determine the next action.", }, @@ -51,3 +58,7 @@ def route_status_summary(route: str) -> tuple[str, str]: presentation["status_waiting_on"], presentation["status_next_step"], ) + + +def status_headline(route: str) -> str: + return ROUTE_PRESENTATION.get(route, ROUTE_PRESENTATION["unknown"])["status_headline"] diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index ca1771c555d..99683d06f2a 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -38,11 +38,8 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: }, ) - self.assertIn( - "- **Waiting on:** Author", - body, - ) - self.assertIn("- **Next step:** Address or respond to 2 review feedback items:", body) + self.assertIn("**Waiting on the author** · refreshed ", body) + self.assertIn("Address or respond to 2 review items:", body) self.assertIn( f"", body, @@ -52,15 +49,12 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: body, ) self.assertNotIn("### Review feedback", body) - self.assertIn(" - **Inline threads:** [1]", body) - self.assertIn(" - **Top-level feedback:** [2]", body) - self.assertIn(f" - _{pr_status_comment.AUTHOR_GUIDANCE}_", body) + self.assertIn("- **Inline threads:** [1]", body) + self.assertIn("- **Top-level threads:** [2]", body) + self.assertIn(f"_{pr_status_comment.AUTHOR_GUIDANCE}_", body) self.assertIn( - "If you believe this pull request is incorrectly routed as waiting " - "on the author, comment `/dashboard route:reviewers` to route it from " - "waiting on the author to waiting on reviewers. If the last refreshed " - "time above predates your latest reply or push, the dashboard hasn't " - "processed it yet.", + "- **Should this be with reviewers?** Comment " + "`/dashboard route:reviewers` to route it to them.", body, ) @@ -128,8 +122,8 @@ def test_status_last_refreshed_changes_for_identical_status(self, _utc_now: Mock first_body = pr_status_comment.render_status_comment(self.pr(), {"route": "approver"}) second_body = pr_status_comment.render_status_comment(self.pr(), {"route": "approver"}) - self.assertIn("_Status last refreshed: 2026-07-18 12:34:56 UTC.", first_body) - self.assertIn("_Status last refreshed: 2026-07-18 12:35:01 UTC.", second_body) + self.assertIn("**Waiting on reviewers** · refreshed 2026-07-18 12:34 UTC", first_body) + self.assertIn("**Waiting on reviewers** · refreshed 2026-07-18 12:35 UTC", second_body) self.assertNotEqual(first_body, second_body) def test_accuracy_note_prefills_central_issue_for_every_status(self) -> None: @@ -145,11 +139,8 @@ def test_accuracy_note_prefills_central_issue_for_every_status(self) -> None: with self.subTest(pr=pr, result=result): body = pr_status_comment.render_status_comment(pr, result) - self.assertIn( - "This automated status or its linked feedback items may be incorrect", - body, - ) - self.assertIn("with the result you expected", body) + self.assertIn("[Report it](", body) + self.assertIn("with what you expected", body) self.assertIn( "https://github.com/open-telemetry/shared-workflows/issues/new?", body, @@ -168,11 +159,8 @@ def test_accuracy_note_prefills_quoted_live_status_comment(self) -> None: {"route": "approver", "facts": {}}, ) - status_comment, accuracy_note = body.split( - "\n\n_This automated status or its linked feedback items may be incorrect.", - maxsplit=1, - ) - report_url = accuracy_note.split("[report it](", maxsplit=1)[1].split( + status_comment, footer = body.split("\n\n
", maxsplit=1) + report_url = footer.split("[Report it](", maxsplit=1)[1].split( ")", maxsplit=1 )[0] issue_body = parse_qs(urlparse(report_url).query)["body"][0] @@ -211,7 +199,7 @@ def test_accuracy_note_bounds_report_url_for_large_status(self) -> None: }, ) - report_url = body.split("[report it](", maxsplit=1)[1].split( + report_url = body.split("[Report it](", maxsplit=1)[1].split( ")", maxsplit=1 )[0] issue_body = parse_qs(urlparse(report_url).query)["body"][0] @@ -234,11 +222,8 @@ def test_waiting_on_author_names_required_ci_failure(self) -> None: }, ) - self.assertIn( - "- **Waiting on:** Author", - body, - ) - self.assertIn("- **Next step:** Investigate required status check failures.", body) + self.assertIn("**Waiting on the author** · refreshed ", body) + self.assertIn("Investigate required status check failures.", body) self.assertNotIn("### Review feedback", body) self.assertNotIn(pr_status_comment.AUTHOR_GUIDANCE, body) @@ -257,10 +242,10 @@ def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None }, ) - self.assertIn("- **Next steps:**", body) - self.assertIn(" - Investigate required status check failures.", body) - self.assertIn(" - Address or respond to 1 review feedback item:", body) - self.assertIn(" - **Inline threads:** [1]", body) + self.assertIn("Two things need attention:", body) + self.assertIn("- **Required checks are failing** — investigate the failures.", body) + self.assertIn("- **1 review item** — address or respond:", body) + self.assertIn(" - **Inline threads:** [1]", body) def test_required_ci_action_notes_configured_non_blocking_failures(self) -> None: body = pr_status_comment.render_status_comment( @@ -278,7 +263,7 @@ def test_required_ci_action_notes_configured_non_blocking_failures(self) -> None ) self.assertIn( - "- **Next step:** Investigate required status check failures. " + "Investigate required status check failures. " "Note: CodeQL and workflow-notification are failing but are not required checks.", body, ) @@ -345,9 +330,9 @@ def test_non_author_route_names_non_blocking_failure(self) -> None: }, ) - self.assertIn("- **Waiting on:** Reviewers", body) + self.assertIn("**Waiting on reviewers** · refreshed ", body) self.assertIn( - "- **Non-blocking check failure:** codecov/patch is failing but is not a required check.", + "**Non-blocking check failure:** codecov/patch is failing but is not a required check.", body, ) @@ -356,18 +341,18 @@ def test_non_author_routes_also_name_required_ci_failures(self) -> None: ( "maintainer", 1, - "Maintainers", + "Waiting on maintainers", ["CodeQL"], "1 required status check is failing.", - "- **Non-blocking check failure:** CodeQL is failing but is not a required check.", + "**Non-blocking check failure:** CodeQL is failing but is not a required check.", ), ( "approver", 2, - "Reviewers", + "Waiting on reviewers", ["CodeQL", "workflow-notification"], "2 required status checks are failing.", - "- **Non-blocking check failures:** CodeQL and workflow-notification are failing but are not required checks.", + "**Non-blocking check failures:** CodeQL and workflow-notification are failing but are not required checks.", ), ) for ( @@ -390,8 +375,8 @@ def test_non_author_routes_also_name_required_ci_failures(self) -> None: }, ) - self.assertIn(f"- **Waiting on:** {waiting_on}", body) - self.assertIn(f"- **Also blocked by:** {blocked_by}", body) + self.assertIn(f"**{waiting_on}** · refreshed ", body) + self.assertIn(f"**Also blocked by:** {blocked_by}", body) self.assertIn(non_blocking_line, body) def test_waiting_on_author_caps_feedback_links_across_sections(self) -> None: @@ -416,10 +401,10 @@ def test_waiting_on_author_caps_feedback_links_across_sections(self) -> None: }, ) - self.assertIn(" - **Inline threads:**", body) - self.assertIn(" - **Top-level feedback:** [20]", body) + self.assertIn("- **Inline threads:**", body) + self.assertIn("- **Top-level threads:** [20]", body) self.assertIn( - " - _Showing 20 of 22 feedback links; " + "- _Showing 20 of 22 feedback links; " "resolve the remaining items from the pull request's conversation._", body, ) @@ -444,9 +429,9 @@ def test_feedback_group_with_no_remaining_link_slots_still_reads_cleanly(self) - }, ) - self.assertNotIn("Top-level feedback", body) + self.assertNotIn("Top-level threads", body) self.assertIn( - " - _Showing 20 of 21 feedback links; " + "- _Showing 20 of 21 feedback links; " "resolve the remaining items from the pull request's conversation._", body, ) @@ -454,11 +439,8 @@ def test_feedback_group_with_no_remaining_link_slots_still_reads_cleanly(self) - def test_draft_waits_on_author(self) -> None: body = pr_status_comment.render_status_comment(self.pr(draft=True), None) - self.assertIn("- **Waiting on:** Author", body) - self.assertIn( - "- **Next step:** Move out of draft to request review.", - body, - ) + self.assertIn("**Waiting on the author** · refreshed ", body) + self.assertIn("Move out of draft to request review.", body) def test_merged_pr_has_no_author_guidance(self) -> None: body = pr_status_comment.render_status_comment( @@ -466,7 +448,7 @@ def test_merged_pr_has_no_author_guidance(self) -> None: None, ) - self.assertIn("**Status:** Merged.", body) + self.assertIn("**Merged** · refreshed ", body) self.assertNotIn(pr_status_comment.AUTHOR_GUIDANCE, body) def test_terminal_pr_has_no_author_feedback_links(self) -> None: @@ -489,7 +471,7 @@ def test_terminal_pr_has_no_author_feedback_links(self) -> None: self.assertNotIn("### Review feedback", body) self.assertNotIn("- **Inline threads", body) - self.assertNotIn("- **Top-level feedback", body) + self.assertNotIn("- **Top-level threads", body) def test_author_login_is_not_mentioned(self) -> None: body = pr_status_comment.render_status_comment( @@ -497,7 +479,7 @@ def test_author_login_is_not_mentioned(self) -> None: {"route": "author", "facts": {"author": "alice"}}, ) - self.assertIn("- **Waiting on:** Author", body) + self.assertIn("**Waiting on the author** · refreshed ", body) self.assertNotIn("@alice", body) def test_external_route_advertises_reviewer_override(self) -> None: @@ -506,32 +488,32 @@ def test_external_route_advertises_reviewer_override(self) -> None: {"route": "external", "facts": {}}, ) + self.assertIn("**Waiting on an external dependency or decision** · refreshed ", body) self.assertIn( - "waiting on an external dependency or decision, comment " - "`/dashboard route:reviewers` to route it from waiting on an " - "external dependency or decision to waiting on reviewers", + "- **Should this be with reviewers?** Comment " + "`/dashboard route:reviewers` to route it to them.", body, ) def test_routes_render_one_status_sentence(self) -> None: expected_summaries = { - "approver": ("Reviewers", "Review the latest changes."), - "maintainer": ("Maintainers", "Merge when ready."), - "copilot": ("Copilot", "Wait for the pending review to complete."), - "external": ("An external dependency or decision", "Resolve it before work can continue."), - "transient-failure": ("Pull request dashboard maintainers", "Determine the next action."), - "unknown": ("Pull request dashboard maintainers", "Determine the next action."), + "approver": ("Waiting on reviewers", "Review the latest changes."), + "maintainer": ("Waiting on maintainers", "Merge when ready."), + "copilot": ("Waiting on Copilot", "Wait for the pending review to complete."), + "external": ("Waiting on an external dependency or decision", "Resolve it before work can continue."), + "transient-failure": ("Waiting on the pull request dashboard maintainers", "Determine the next action."), + "unknown": ("Waiting on the pull request dashboard maintainers", "Determine the next action."), } - for route, (waiting_on, next_step) in expected_summaries.items(): + for route, (headline, next_step) in expected_summaries.items(): with self.subTest(route=route): body = pr_status_comment.render_status_comment( self.pr(), {"route": route, "facts": {}}, ) - self.assertIn(f"- **Waiting on:** {waiting_on}", body) - self.assertIn(f"- **Next step:** {next_step}", body) + self.assertIn(f"**{headline}** · refreshed ", body) + self.assertIn(next_step, body) self.assertNotIn("**Status:**", body) self.assertNotIn(pr_status_comment.AUTHOR_GUIDANCE, body) From f1292cc203b1cd776a9d29673b3c85e08efd4d43 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:36:12 -0700 Subject: [PATCH 2/5] Add blank line below status comment footer summary --- .github/scripts/pull-request-dashboard/pr_status_comment.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index e303776d25e..4970827fd34 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -129,6 +129,8 @@ def status_footer( "
", "Doesn't look right?", "", + "
", + "", ] if not terminal: lines.append( From e805a894a49d870d6008650ffe89daa1306850dc Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:41:57 -0700 Subject: [PATCH 3/5] Address review comment from copilot-pull-request-reviewer: clarify staleness guidance for minute-granular timestamp --- .github/scripts/pull-request-dashboard/pr_status_comment.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 4970827fd34..73a81822df7 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -134,8 +134,8 @@ def status_footer( ] if not terminal: lines.append( - "- **Just replied or pushed?** Anything after the refresh time above " - "hasn't been picked up yet \u2014 give it a few minutes." + "- **Just replied or pushed?** Anything around or after the refresh " + "time above may not be picked up yet \u2014 give it a few minutes." ) if override_route: lines.append( From d89cf59bb5f3c6748ab720181308c4598d6fe3e3 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 15:48:15 -0700 Subject: [PATCH 4/5] Reword author ask from address-or-respond to respond --- .github/scripts/pull-request-dashboard/pr_status_comment.py | 4 ++-- .../scripts/pull-request-dashboard/test_pr_status_comment.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 73a81822df7..741f054cacf 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -194,7 +194,7 @@ def author_body( body = [ "Two things need attention:", checks_bullet, - f"- **{feedback_count} review {noun}** \u2014 address or respond:", + f"- **{feedback_count} review {noun}** — respond:", ] body.extend( feedback_breakdown_lines( @@ -204,7 +204,7 @@ def author_body( body.extend(["", f"_{AUTHOR_GUIDANCE}_"]) return body if feedback_count: - body = [f"Address or respond to {feedback_count} review {noun}:"] + body = [f"Respond to {feedback_count} review {noun}:"] body.extend( feedback_breakdown_lines(review_thread_urls, top_level_feedback_urls) ) diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index 99683d06f2a..b30266544f6 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -39,7 +39,7 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: ) self.assertIn("**Waiting on the author** · refreshed ", body) - self.assertIn("Address or respond to 2 review items:", body) + self.assertIn("Respond to 2 review items:", body) self.assertIn( f"", body, @@ -244,7 +244,7 @@ def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None self.assertIn("Two things need attention:", body) self.assertIn("- **Required checks are failing** — investigate the failures.", body) - self.assertIn("- **1 review item** — address or respond:", body) + self.assertIn("- **1 review item** — respond:", body) self.assertIn(" - **Inline threads:** [1]", body) def test_required_ci_action_notes_configured_non_blocking_failures(self) -> None: From 334967a5b589d11a6d2d63bea5b9a4240da2d6be Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Wed, 22 Jul 2026 16:14:45 -0700 Subject: [PATCH 5/5] Fold response examples into the author review-feedback ask --- .../pull-request-dashboard/pr_status_comment.py | 11 +++-------- .../pull-request-dashboard/test_pr_status_comment.py | 11 +++++------ 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 741f054cacf..2bd152679e6 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -43,10 +43,7 @@ STATUS_REPORT_TRUNCATION_NOTICE = ( "[Status comment truncated to keep this report link usable.]" ) -AUTHOR_GUIDANCE = ( - "Reply to move it forward \u2014 for example, link the fixing commit, explain " - "why no change is needed, or ask a follow-up." -) +RESPONSE_EXAMPLES = "(e.g. link a commit, explain why not, ask a follow-up)" DASHBOARD_APP_SLUG = "opentelemetry-pr-dashboard" # Remove after migrating open PRs as described by the post-rollout # compatibility cleanup in WEBHOOK_SETUP.md. @@ -194,21 +191,19 @@ def author_body( body = [ "Two things need attention:", checks_bullet, - f"- **{feedback_count} review {noun}** — respond:", + f"- **{feedback_count} review {noun}** — respond to each {RESPONSE_EXAMPLES}:", ] body.extend( feedback_breakdown_lines( review_thread_urls, top_level_feedback_urls, indent=" " ) ) - body.extend(["", f"_{AUTHOR_GUIDANCE}_"]) return body if feedback_count: - body = [f"Respond to {feedback_count} review {noun}:"] + body = [f"Respond to {feedback_count} review {noun} {RESPONSE_EXAMPLES}:"] body.extend( feedback_breakdown_lines(review_thread_urls, top_level_feedback_urls) ) - body.extend(["", f"_{AUTHOR_GUIDANCE}_"]) return body if failing_count: sentence = "Investigate required status check failures." diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index b30266544f6..93e84799617 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -39,7 +39,7 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: ) self.assertIn("**Waiting on the author** · refreshed ", body) - self.assertIn("Respond to 2 review items:", body) + self.assertIn(f"Respond to 2 review items {pr_status_comment.RESPONSE_EXAMPLES}:", body) self.assertIn( f"", body, @@ -51,7 +51,6 @@ def test_waiting_on_author_splits_review_feedback_links(self) -> None: self.assertNotIn("### Review feedback", body) self.assertIn("- **Inline threads:** [1]", body) self.assertIn("- **Top-level threads:** [2]", body) - self.assertIn(f"_{pr_status_comment.AUTHOR_GUIDANCE}_", body) self.assertIn( "- **Should this be with reviewers?** Comment " "`/dashboard route:reviewers` to route it to them.", @@ -225,7 +224,7 @@ def test_waiting_on_author_names_required_ci_failure(self) -> None: self.assertIn("**Waiting on the author** · refreshed ", body) self.assertIn("Investigate required status check failures.", body) self.assertNotIn("### Review feedback", body) - self.assertNotIn(pr_status_comment.AUTHOR_GUIDANCE, body) + self.assertNotIn(pr_status_comment.RESPONSE_EXAMPLES, body) def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None: body = pr_status_comment.render_status_comment( @@ -244,7 +243,7 @@ def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None self.assertIn("Two things need attention:", body) self.assertIn("- **Required checks are failing** — investigate the failures.", body) - self.assertIn("- **1 review item** — respond:", body) + self.assertIn("- **1 review item** — respond to each {}:".format(pr_status_comment.RESPONSE_EXAMPLES), body) self.assertIn(" - **Inline threads:** [1]", body) def test_required_ci_action_notes_configured_non_blocking_failures(self) -> None: @@ -449,7 +448,7 @@ def test_merged_pr_has_no_author_guidance(self) -> None: ) self.assertIn("**Merged** · refreshed ", body) - self.assertNotIn(pr_status_comment.AUTHOR_GUIDANCE, body) + self.assertNotIn(pr_status_comment.RESPONSE_EXAMPLES, body) def test_terminal_pr_has_no_author_feedback_links(self) -> None: result = { @@ -515,7 +514,7 @@ def test_routes_render_one_status_sentence(self) -> None: self.assertIn(f"**{headline}** · refreshed ", body) self.assertIn(next_step, body) self.assertNotIn("**Status:**", body) - self.assertNotIn(pr_status_comment.AUTHOR_GUIDANCE, body) + self.assertNotIn(pr_status_comment.RESPONSE_EXAMPLES, body) class UpsertStatusCommentTest(unittest.TestCase):