From a3d7be9daee27cd94fdab364c68e800022146ca4 Mon Sep 17 00:00:00 2001 From: real-venus Date: Tue, 14 Jul 2026 08:55:08 -0700 Subject: [PATCH] feat(review): synthesize the 'Contributor next steps' collapsible into a prioritized step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collapsible re-listed the same action text already shown verbatim in the Signals table's Action column, so it added nothing a reader hadn't already seen (#5097). Redesign it to lead with the single highest-priority step as a 'Start here' synthesis — the one thing the flat table does not say — and point back to the table for the remainder, so no underlying signal is lost (publicSafeNextSteps still computes the full maintainer-lane note + readiness actions + finding actions, rendered in full in the table above). Both render sites (the converged comment and the legacy panel) route through the shared contributorNextStepsBody so they can never diverge. Closes #5097 --- src/signals/engine.ts | 21 ++++++++++++--- test/unit/unified-comment-parity.test.ts | 34 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 967ff1ef31..e28cc261f8 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -4261,9 +4261,22 @@ function reviewContextBody(args: PublicSafeCollapsibleArgs): string[] { ]; } -/** "Contributor next steps" body — the deduped actionable steps (or a fallback when none). */ -function contributorNextStepsBody(nextSteps: string[]): string[] { - return nextSteps.length > 0 ? [...new Set(nextSteps)].map((step) => `- ${step}`) : ["- Keep the PR focused and include validation evidence before maintainer review."]; +/** "Contributor next steps" body (#5097). The Signals table's own Action column already lists every one of these + * actions verbatim, so a flat re-listing here added nothing a reader hadn't already seen — which is why it read + * as low-value. Instead this leads with the single highest-priority step as a "Start here" synthesis (the one + * thing the flat table does not say) and points back to the table for the remainder, so the underlying signal + * (`publicSafeNextSteps`: the maintainer-lane note, the readiness actions, the finding actions) is preserved in + * full above while this collapsible finally earns its place. Falls back to the generic line when there are no + * steps at all. */ +export function contributorNextStepsBody(nextSteps: string[]): string[] { + const deduped = [...new Set(nextSteps)]; + if (deduped.length === 0) return ["- Keep the PR focused and include validation evidence before maintainer review."]; + const [first, ...rest] = deduped; + if (rest.length === 0) return [`- **Start here:** ${first}`]; + return [ + `- **Start here:** ${first}`, + `- Then work through the remaining ${rest.length} step${rest.length === 1 ? "" : "s"} in the Signals table above.`, + ]; } /** #5096: one reusable convention for EXPERIMENTAL ("beta") collapsibles in the public PR comment, so a reader @@ -4590,7 +4603,7 @@ export function buildPublicPrIntelligenceComment(args: { "
", "Contributor next steps", "", - ...(nextSteps.length > 0 ? [...new Set(nextSteps)].map((step) => `- ${step}`) : ["- Keep the PR focused and include validation evidence before maintainer review."]), + ...contributorNextStepsBody(nextSteps), "", "
", "", diff --git a/test/unit/unified-comment-parity.test.ts b/test/unit/unified-comment-parity.test.ts index bcc5ed9d42..f04907d0e2 100644 --- a/test/unit/unified-comment-parity.test.ts +++ b/test/unit/unified-comment-parity.test.ts @@ -7,6 +7,7 @@ import { buildPublicPrPanelSignalRows, buildPublicSafeCollapsibles, buildQueueHealth, + contributorNextStepsBody, detectGittensorContributor, } from "../../src/signals/engine"; import { buildUnifiedCommentBody } from "../../src/review/unified-comment-bridge"; @@ -170,6 +171,39 @@ describe("converged comment ↔ legacy panel parity (#unified-comment)", () => { expect(nextSteps.body.length).toBeGreaterThan(0); }); + describe("contributorNextStepsBody redesign (#5097)", () => { + it("leads with a prioritized 'Start here' step and points to the table for the rest", () => { + expect(contributorNextStepsBody(["Add a linked issue.", "Fix the failing check.", "Add tests."])).toEqual([ + "- **Start here:** Add a linked issue.", + "- Then work through the remaining 2 steps in the Signals table above.", + ]); + }); + + it("uses the singular 'step' when exactly one remains", () => { + expect(contributorNextStepsBody(["Add a linked issue.", "Fix the failing check."])).toEqual([ + "- **Start here:** Add a linked issue.", + "- Then work through the remaining 1 step in the Signals table above.", + ]); + }); + + it("shows only the single step when there is exactly one", () => { + expect(contributorNextStepsBody(["Add a linked issue."])).toEqual(["- **Start here:** Add a linked issue."]); + }); + + it("dedupes repeated steps before prioritizing", () => { + expect(contributorNextStepsBody(["Add tests.", "Add tests.", "Fix the check."])).toEqual([ + "- **Start here:** Add tests.", + "- Then work through the remaining 1 step in the Signals table above.", + ]); + }); + + it("falls back to the generic line when there are no steps at all", () => { + expect(contributorNextStepsBody([])).toEqual([ + "- Keep the PR focused and include validation evidence before maintainer review.", + ]); + }); + }); + it("the legacy panel still renders 'Maintainer notes' inline (private section is unchanged, just not shared)", () => { const { currentPr, detection, collisions, queueHealth, preflight, profile } = buildFixtures(); const legacy = buildPublicPrIntelligenceComment({env: {}, repo, pr: currentPr, profile, detection, queueHealth, collisions, preflight, settings });