diff --git a/packages/app/src/components/ui/control-geometry.ts b/packages/app/src/components/ui/control-geometry.ts index 9ed605408..f6f3800ec 100644 --- a/packages/app/src/components/ui/control-geometry.ts +++ b/packages/app/src/components/ui/control-geometry.ts @@ -36,7 +36,12 @@ const CONTROL_FOCUS_RING_OFFSET = 1; const CONTROL_CENTER_JUSTIFY_CONTENT = "center"; const FIELD_TEXT_LINE_HEIGHT_RATIO = 1.4; -const controlHeights = { +/** + * The three control heights every button, field, and segmented control is built from. + * Exported so a row that hosts one of those controls can size itself from the same + * numbers instead of guessing a height the control then outgrows. + */ +export const CONTROL_HEIGHTS = { tight: TIGHT_CONTROL_HEIGHT, compact: COMPACT_CONTROL_HEIGHT, field: FIELD_CONTROL_HEIGHT, @@ -102,15 +107,15 @@ export function createControlGeometry(theme: Theme) { const fieldTextSmLineHeight = fieldLineHeight(theme.fontSize.sm); const fieldTextMdLineHeight = fieldLineHeight(theme.fontSize.base); const fieldControlSm = { - minHeight: controlHeights.compact, + minHeight: CONTROL_HEIGHTS.compact, paddingHorizontal: theme.spacing[3], - paddingVertical: fieldVerticalPadding(controlHeights.compact, fieldTextSmLineHeight), + paddingVertical: fieldVerticalPadding(CONTROL_HEIGHTS.compact, fieldTextSmLineHeight), borderRadius: theme.borderRadius.md, }; const fieldControlMd = { - minHeight: controlHeights.field, + minHeight: CONTROL_HEIGHTS.field, paddingHorizontal: theme.spacing[4], - paddingVertical: fieldVerticalPadding(controlHeights.field, fieldTextMdLineHeight), + paddingVertical: fieldVerticalPadding(CONTROL_HEIGHTS.field, fieldTextMdLineHeight), borderRadius: theme.borderRadius.lg, }; const fieldTextSm = { @@ -122,28 +127,28 @@ export function createControlGeometry(theme: Theme) { lineHeight: fieldTextMdLineHeight, }; const switchControl = { - minHeight: controlHeights.compact, + minHeight: CONTROL_HEIGHTS.compact, justifyContent: CONTROL_CENTER_JUSTIFY_CONTENT, } satisfies { minHeight: number; justifyContent: "center" }; return { buttonXs: { - minHeight: controlHeights.tight, + minHeight: CONTROL_HEIGHTS.tight, paddingHorizontal: theme.spacing[3], borderRadius: theme.borderRadius.md, }, buttonSm: { - minHeight: controlHeights.compact, + minHeight: CONTROL_HEIGHTS.compact, paddingHorizontal: theme.spacing[3], borderRadius: theme.borderRadius.md, }, buttonMd: { - minHeight: controlHeights.field, + minHeight: CONTROL_HEIGHTS.field, paddingHorizontal: theme.spacing[4], borderRadius: theme.borderRadius.lg, }, buttonLg: { - minHeight: controlHeights.field, + minHeight: CONTROL_HEIGHTS.field, paddingHorizontal: theme.spacing[6], borderRadius: theme.borderRadius.xl, }, @@ -193,29 +198,29 @@ export function createControlGeometry(theme: Theme) { }, switchControl, segmentedContainerXs: { - minHeight: controlHeights.tight, + minHeight: CONTROL_HEIGHTS.tight, padding: 0, }, segmentedContainerSm: { - minHeight: controlHeights.compact, + minHeight: CONTROL_HEIGHTS.compact, padding: 0, }, segmentedContainerMd: { - minHeight: controlHeights.field, + minHeight: CONTROL_HEIGHTS.field, padding: 0, }, segmentedSegmentXs: { - minHeight: controlHeights.tight - SEGMENTED_TIGHT_INSET * 2, + minHeight: CONTROL_HEIGHTS.tight - SEGMENTED_TIGHT_INSET * 2, paddingHorizontal: theme.spacing[3], borderRadius: theme.borderRadius.full, }, segmentedSegmentSm: { - minHeight: controlHeights.compact - SEGMENTED_COMPACT_INSET * 2, + minHeight: CONTROL_HEIGHTS.compact - SEGMENTED_COMPACT_INSET * 2, paddingHorizontal: theme.spacing[3], borderRadius: theme.borderRadius.full, }, segmentedSegmentMd: { - minHeight: controlHeights.field - SEGMENTED_FIELD_INSET * 2, + minHeight: CONTROL_HEIGHTS.field - SEGMENTED_FIELD_INSET * 2, paddingHorizontal: theme.spacing[4], borderRadius: theme.borderRadius.full, }, diff --git a/packages/app/src/git/pull-request-panel/checks-ring.tsx b/packages/app/src/git/pull-request-panel/checks-ring.tsx new file mode 100644 index 000000000..65d69e139 --- /dev/null +++ b/packages/app/src/git/pull-request-panel/checks-ring.tsx @@ -0,0 +1,115 @@ +import Svg, { Circle } from "react-native-svg"; +import { StyleSheet, withUnistyles } from "react-native-unistyles"; +import type { Theme } from "@/styles/theme"; +import type { CheckStatus } from "./check-status"; +import type { ChecksSummary } from "./checks-summary"; + +/** + * Geometry in a 20-unit box scaled to the requested size, so the ring keeps its stroke + * weight next to the header text at any size. + */ +const VIEW_BOX = 20; +const STROKE_WIDTH = 3; +const CENTER = VIEW_BOX / 2; +const RADIUS = (VIEW_BOX - STROKE_WIDTH) / 2; +const CIRCUMFERENCE = 2 * Math.PI * RADIUS; +/** Blank arc between segments so two adjacent colours stay legible at 20px. */ +const SEGMENT_GAP = 2; +/** Floor for a segment, so one failure among twenty checks is still a visible mark. */ +const MIN_SEGMENT = 1.5; + +type ChecksPalette = Record & { track: string }; + +const paletteMapping = (theme: Theme): { palette: ChecksPalette } => ({ + palette: { + failure: theme.colors.statusDanger, + pending: theme.colors.statusWarning, + success: theme.colors.statusSuccess, + skipped: theme.colors.foregroundExtraMuted, + track: theme.colors.surface3, + }, +}); + +interface RingSegment { + status: CheckStatus; + /** Painted arc length. */ + dash: number; + /** Distance from twelve o'clock to where this segment starts. */ + offset: number; +} + +/** + * Each status gets a slice of the circle proportional to its share of the run, laid out + * clockwise from twelve. Gaps come out of the painted arc rather than the layout, so the + * slices still add up to the whole circle and the last one lands back at twelve. + */ +function buildSegments(summary: ChecksSummary): RingSegment[] { + const segments: RingSegment[] = []; + let offset = 0; + for (const part of summary.parts) { + const arc = (part.count / summary.total) * CIRCUMFERENCE; + // A single status owns the whole circle, and a gap there would read as a stray notch. + const dash = + summary.parts.length === 1 ? CIRCUMFERENCE : Math.max(arc - SEGMENT_GAP, MIN_SEGMENT); + segments.push({ status: part.status, dash, offset }); + offset += arc; + } + return segments; +} + +function ChecksRingSvg({ + summary, + size, + palette, +}: { + summary: ChecksSummary; + size: number; + palette: ChecksPalette; +}) { + return ( + + + {buildSegments(summary).map((segment) => ( + + ))} + + ); +} + +const ThemedChecksRingSvg = withUnistyles(ChecksRingSvg); + +/** A change request's CI as one glyph: the run's statuses in proportion, worst first. */ +export function ChecksRing({ summary, size }: { summary: ChecksSummary; size: number }) { + return ; +} + +const ringStyles = StyleSheet.create(() => ({ + svg: { + // SVG strokes start at three o'clock; the ring reads clockwise from twelve. + transform: [{ rotate: "-90deg" }], + }, +})); diff --git a/packages/app/src/git/pull-request-panel/checks-section.tsx b/packages/app/src/git/pull-request-panel/checks-section.tsx new file mode 100644 index 000000000..142fc4650 --- /dev/null +++ b/packages/app/src/git/pull-request-panel/checks-section.tsx @@ -0,0 +1,289 @@ +import { useCallback, useMemo, useState } from "react"; +import { Pressable, ScrollView, Text, View, type GestureResponderEvent } from "react-native"; +import { StyleSheet, withUnistyles } from "react-native-unistyles"; +import { ChevronDown, ChevronRight, MessageSquarePlus } from "lucide-react-native"; +import { Button } from "@/components/ui/button"; +import { openExternalUrl } from "@/utils/open-external-url"; +import { ICON_SIZE } from "@/styles/theme"; +import type { CheckStatus } from "./check-status"; +import { ChecksRing } from "./checks-ring"; +import { summarizeChecks, type ChecksGroup } from "./checks-summary"; +import { canAddPullRequestCheckLogsToChat } from "./context-attachment"; +import type { PrPaneCheck } from "./data"; +import { CheckStatusIcon, foregroundMutedColorMapping, sectionKitStyles } from "./section-kit"; + +const ThemedChevronDown = withUnistyles(ChevronDown); +const ThemedChevronRight = withUnistyles(ChevronRight); + +/** + * Roughly eight rows. A repo with thirty checks would otherwise push the activity + * timeline off the pane entirely, so past this height the list scrolls in place. + */ +const LIST_MAX_HEIGHT = 268; + +/** + * The three statuses the pane has always labelled for tests. Skipped has no id because + * nothing asserts on it. + */ +const PART_TEST_ID: Partial> = { + success: "pr-pane-check-passed", + failure: "pr-pane-check-failed", + pending: "pr-pane-check-pending", +}; + +/** + * Identifies a check across refreshes. The forge's run id is stable where a forge exposes + * one; the name/url pair is the fallback for forges that don't. + */ +export function getCheckIdentity(check: PrPaneCheck): string { + if (check.detailRef?.checkRunId !== undefined) { + return `${check.provider}:check-run:${check.detailRef.checkRunId}`; + } + if (check.detailRef?.workflowRunId !== undefined) { + return `${check.provider}:workflow-run:${check.detailRef.workflowRunId}`; + } + return `${check.provider}:${check.name}:${check.url}`; +} + +interface ChecksSectionProps { + checks: readonly PrPaneCheck[]; + open: boolean; + onToggle: () => void; + attachEnabled: boolean; + loadingCheckKeys: ReadonlySet; + onAddLogsToChat: (check: PrPaneCheck) => void; +} + +/** + * The CI surface of the PR pane: one line saying how the run went, and the checks behind + * it grouped by status so a failure never needs scrolling to find. + */ +export function ChecksSection({ + checks, + open, + onToggle, + attachEnabled, + loadingCheckKeys, + onAddLogsToChat, +}: ChecksSectionProps) { + const summary = useMemo(() => summarizeChecks(checks), [checks]); + const [collapsedGroups, setCollapsedGroups] = useState>(() => new Set()); + + const handleToggleGroup = useCallback((status: CheckStatus) => { + setCollapsedGroups((current) => { + const next = new Set(current); + if (!next.delete(status)) { + next.add(status); + } + return next; + }); + }, []); + + return ( + + + + + + {summary.headline} + + {summary.parts.length > 0 ? ( + + {summary.parts.map((part, index) => ( + + {index > 0 ? ", " : ""} + {part.text} + + ))} + {` ${summary.countNoun}`} + + ) : null} + + {open ? ( + + ) : ( + + )} + + + {open && summary.groups.length > 0 ? ( + + {summary.groups.map((group) => ( + + ))} + + ) : null} + + ); +} + +function CheckGroup({ + group, + collapsed, + onToggle, + attachEnabled, + loadingCheckKeys, + onAddLogsToChat, +}: { + group: ChecksGroup; + collapsed: boolean; + onToggle: (status: CheckStatus) => void; + attachEnabled: boolean; + loadingCheckKeys: ReadonlySet; + onAddLogsToChat: (check: PrPaneCheck) => void; +}) { + const handlePress = useCallback(() => onToggle(group.status), [group.status, onToggle]); + return ( + + + {group.label} + {collapsed ? ( + + ) : ( + + )} + + {collapsed + ? null + : group.checks.map((check) => { + const checkKey = getCheckIdentity(check); + return ( + + ); + })} + + ); +} + +function CheckRow({ + check, + attachEnabled, + isAddingLogsToChat, + onAddLogsToChat, +}: { + check: PrPaneCheck; + attachEnabled: boolean; + isAddingLogsToChat: boolean; + onAddLogsToChat: (check: PrPaneCheck) => void; +}) { + const handlePress = useCallback(() => { + void openExternalUrl(check.url); + }, [check.url]); + const handleAddLogsToChat = useCallback( + (event: GestureResponderEvent) => { + event.stopPropagation(); + onAddLogsToChat(check); + }, + [check, onAddLogsToChat], + ); + return ( + + + + {check.name} + + {check.workflow && ( + + {check.workflow} + + )} + + {attachEnabled && canAddPullRequestCheckLogsToChat(check) ? ( + + ) : null} + {check.timing && {check.timing}} + + + ); +} + +function headerPressableStyle({ hovered }: { hovered?: boolean }) { + return [styles.header, Boolean(hovered) && styles.hoverable]; +} + +function rowPressableStyle({ hovered }: { hovered?: boolean }) { + return [sectionKitStyles.checkRow, Boolean(hovered) && styles.hoverable]; +} + +const styles = StyleSheet.create((theme) => ({ + header: { + flexDirection: "row", + alignItems: "center", + gap: theme.spacing[2], + paddingHorizontal: theme.spacing[3], + paddingVertical: theme.spacing[3], + }, + headerText: { + flex: 1, + minWidth: 0, + gap: theme.spacing[0.5], + }, + headline: { + fontSize: theme.fontSize.sm, + fontWeight: theme.fontWeight.medium, + color: theme.colors.foreground, + }, + detail: { + fontSize: theme.fontSize.xs, + color: theme.colors.foregroundMuted, + }, + list: { + maxHeight: LIST_MAX_HEIGHT, + }, + listContent: { + paddingBottom: theme.spacing[2], + }, + groupHeader: { + flexDirection: "row", + alignItems: "center", + gap: theme.spacing[1], + paddingHorizontal: theme.spacing[3], + paddingTop: theme.spacing[2], + paddingBottom: theme.spacing[1], + }, + groupLabel: { + fontSize: theme.fontSize.xs, + fontWeight: theme.fontWeight.medium, + color: theme.colors.foregroundMuted, + }, + hoverable: { + backgroundColor: theme.colors.surfaceSidebarHover, + }, + addButton: { + paddingHorizontal: theme.spacing[1], + }, +})); diff --git a/packages/app/src/git/pull-request-panel/checks-summary.test.ts b/packages/app/src/git/pull-request-panel/checks-summary.test.ts new file mode 100644 index 000000000..f01450075 --- /dev/null +++ b/packages/app/src/git/pull-request-panel/checks-summary.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from "vitest"; +import type { CheckStatus } from "./check-status"; +import { summarizeChecks } from "./checks-summary"; +import type { PrPaneCheck } from "./data"; + +function check(name: string, status: CheckStatus): PrPaneCheck { + return { provider: "github", name, status, url: `https://example.test/${name}` }; +} + +describe("summarizeChecks", () => { + it("reports no checks without claiming the run passed", () => { + const summary = summarizeChecks([]); + + expect(summary.outcome).toBe("none"); + expect(summary.headline).toBe("No checks"); + expect(summary.detail).toBe(""); + expect(summary.groups).toEqual([]); + }); + + it("leads with failures when anything failed", () => { + const summary = summarizeChecks([ + check("build", "success"), + check("lint", "failure"), + check("e2e", "pending"), + ]); + + expect(summary.outcome).toBe("failure"); + expect(summary.headline).toBe("Some checks were not successful"); + expect(summary.detail).toBe("1 failing, 1 in progress, 1 successful checks"); + expect(summary.groups.map((group) => group.status)).toEqual(["failure", "pending", "success"]); + }); + + it("reports an unfinished run as in progress when nothing failed", () => { + const summary = summarizeChecks([check("build", "success"), check("e2e", "pending")]); + + expect(summary.outcome).toBe("pending"); + expect(summary.headline).toBe("Some checks haven't completed yet"); + expect(summary.detail).toBe("1 in progress, 1 successful checks"); + }); + + it("treats a run of only successes and skips as passed", () => { + const summary = summarizeChecks([check("build", "success"), check("deploy", "skipped")]); + + expect(summary.outcome).toBe("success"); + expect(summary.headline).toBe("All checks have passed"); + expect(summary.detail).toBe("1 successful, 1 skipped checks"); + }); + + it("keeps the checks of each group together under a counted label", () => { + const summary = summarizeChecks([ + check("build", "failure"), + check("lint", "success"), + check("test", "failure"), + ]); + + const [failing, successful] = summary.groups; + expect(failing?.label).toBe("2 failing checks"); + expect(failing?.checks.map((item) => item.name)).toEqual(["build", "test"]); + expect(successful?.label).toBe("1 successful check"); + }); + + it("says check rather than checks for a run of one", () => { + const summary = summarizeChecks([check("build", "failure")]); + + expect(summary.countNoun).toBe("check"); + expect(summary.detail).toBe("1 failing check"); + }); +}); diff --git a/packages/app/src/git/pull-request-panel/checks-summary.ts b/packages/app/src/git/pull-request-panel/checks-summary.ts new file mode 100644 index 000000000..3351cfb19 --- /dev/null +++ b/packages/app/src/git/pull-request-panel/checks-summary.ts @@ -0,0 +1,121 @@ +import type { CheckStatus } from "./check-status"; +import type { PrPaneCheck } from "./data"; + +/** + * The order the checks section reads in: what the user can act on, then what they are + * waiting on, then what needs no attention. Skipped trails everything because it is the + * only status that says nothing about whether the run is going well. + */ +const STATUS_ORDER = [ + "failure", + "pending", + "success", + "skipped", +] as const satisfies readonly CheckStatus[]; + +/** How each status reads inside a count phrase: "2 failing", "4 in progress". */ +const STATUS_NOUN: Record = { + failure: "failing", + pending: "in progress", + success: "successful", + skipped: "skipped", +}; + +/** The worst thing happening in the run, which is what the headline and the ring report. */ +export type ChecksOutcome = "failure" | "pending" | "success" | "none"; + +const OUTCOME_HEADLINE: Record = { + failure: "Some checks were not successful", + pending: "Some checks haven't completed yet", + success: "All checks have passed", + none: "No checks", +}; + +export interface ChecksCountPart { + status: CheckStatus; + count: number; + /** e.g. "2 failing" */ + text: string; +} + +export interface ChecksGroup { + status: CheckStatus; + /** e.g. "2 failing checks" */ + label: string; + checks: readonly PrPaneCheck[]; +} + +export interface ChecksSummary { + outcome: ChecksOutcome; + /** e.g. "Some checks were not successful" */ + headline: string; + /** The count phrases behind `detail`, kept apart so the header can label each one. */ + parts: readonly ChecksCountPart[]; + /** The noun closing the detail line — "checks", or "check" for a run of one. */ + countNoun: string; + /** The whole line: "2 failing, 4 in progress, 1 successful checks". Empty with no checks. */ + detail: string; + total: number; + /** Non-empty groups only, in `STATUS_ORDER`. */ + groups: readonly ChecksGroup[]; +} + +/** + * Reduces a change request's checks to everything the checks section renders, so the + * header, the ring, and the grouped list all read from one derivation instead of each + * filtering the array again with its own idea of what counts. + */ +export function summarizeChecks(checks: readonly PrPaneCheck[]): ChecksSummary { + const groups: ChecksGroup[] = []; + const parts: ChecksCountPart[] = []; + + for (const status of STATUS_ORDER) { + const matching = checks.filter((check) => check.status === status); + if (matching.length === 0) { + continue; + } + groups.push({ + status, + label: `${matching.length} ${STATUS_NOUN[status]} ${countNoun(matching.length)}`, + checks: matching, + }); + parts.push({ + status, + count: matching.length, + text: `${matching.length} ${STATUS_NOUN[status]}`, + }); + } + + const outcome = selectOutcome(checks); + const noun = countNoun(checks.length); + return { + outcome, + headline: OUTCOME_HEADLINE[outcome], + parts, + countNoun: noun, + detail: parts.length === 0 ? "" : `${parts.map((part) => part.text).join(", ")} ${noun}`, + total: checks.length, + groups, + }; +} + +/** + * A failure outranks anything still running: a run that is half done with one failure + * already needs the user, and reporting it as in progress buries that. + */ +function selectOutcome(checks: readonly PrPaneCheck[]): ChecksOutcome { + if (checks.length === 0) { + return "none"; + } + if (checks.some((check) => check.status === "failure")) { + return "failure"; + } + if (checks.some((check) => check.status === "pending")) { + return "pending"; + } + return "success"; +} + +function countNoun(count: number): string { + return count === 1 ? "check" : "checks"; +} diff --git a/packages/app/src/git/pull-request-panel/data.test.ts b/packages/app/src/git/pull-request-panel/data.test.ts index 868daaaa3..b28081651 100644 --- a/packages/app/src/git/pull-request-panel/data.test.ts +++ b/packages/app/src/git/pull-request-panel/data.test.ts @@ -155,7 +155,7 @@ describe("mapPrPaneData", () => { name: "success", workflow: "CI", status: "success", - duration: "1m", + timing: "1m", url: "https://example.com/1", }, { provider: "github", name: "failure", status: "failure", url: "https://example.com/2" }, @@ -165,6 +165,27 @@ describe("mapPrPaneData", () => { ]); }); + it("marks a running check's elapsed time so it does not read as a finished duration", () => { + const data = mapPrPaneData( + status({ + checks: [ + { name: "running", status: "pending", url: "https://example.com/1", duration: "7m" }, + { name: "done", status: "success", url: "https://example.com/2", duration: "1m 4s" }, + { name: "broke", status: "failure", url: "https://example.com/3", duration: "12s" }, + { name: "untimed", status: "pending", url: "https://example.com/4" }, + ], + }), + baseTimeline, + ); + + expect(data?.checks.map((check) => check.timing)).toEqual([ + "running 7m", + "1m 4s", + "12s", + undefined, + ]); + }); + it("maps GitHub check identifiers into provider refs", () => { const data = mapPrPaneData( status({ diff --git a/packages/app/src/git/pull-request-panel/data.ts b/packages/app/src/git/pull-request-panel/data.ts index 4b16968b9..84019ea01 100644 --- a/packages/app/src/git/pull-request-panel/data.ts +++ b/packages/app/src/git/pull-request-panel/data.ts @@ -27,7 +27,11 @@ export interface PrPaneCheck { name: string; workflow?: string; status: CheckStatus; - duration?: string; + /** + * What the row says about time: how long a finished check took ("2m"), or how long a + * running one has been going ("running 2m"). Absent when the forge reports neither. + */ + timing?: string; url: string; /** * Forge-neutral reference for fetching this check's detail/logs on demand. Any @@ -212,14 +216,16 @@ function mapCheck( return []; } + const status = mapCheckStatus(check.status); + const timing = formatCheckTiming(check.duration, status); return [ { provider: forge, name: check.name, - status: mapCheckStatus(check.status), + status, url: check.url, ...(check.workflow ? { workflow: check.workflow } : {}), - ...(check.duration ? { duration: check.duration } : {}), + ...(timing ? { timing } : {}), ...(check.checkRunId !== undefined || check.workflowRunId !== undefined ? { detailRef: { @@ -232,6 +238,18 @@ function mapCheck( ]; } +/** + * The forge measures how long a check ran, whether or not it has finished. Which one it + * is comes from the status: on a running check a bare "2m" would read as "took 2m", so + * the row says the run is still going. + */ +function formatCheckTiming(duration: string | undefined, status: CheckStatus): string | null { + if (!duration) { + return null; + } + return status === "pending" ? `running ${duration}` : duration; +} + function mapActivity(item: PullRequestTimelineItem, nowMs: number, forge: Forge): PrPaneActivity[] { if (item.kind === "comment") { if (item.body.trim() === "") { diff --git a/packages/app/src/git/pull-request-panel/pane.tsx b/packages/app/src/git/pull-request-panel/pane.tsx index 582d9543a..ed6434084 100644 --- a/packages/app/src/git/pull-request-panel/pane.tsx +++ b/packages/app/src/git/pull-request-panel/pane.tsx @@ -1,13 +1,5 @@ import { useCallback, useMemo, useState } from "react"; -import { - Image, - Pressable, - ScrollView, - Text, - View, - type GestureResponderEvent, - type ViewStyle, -} from "react-native"; +import { Image, Pressable, ScrollView, Text, View, type ViewStyle } from "react-native"; import { StyleSheet, withUnistyles } from "react-native-unistyles"; import { CircleCheck, @@ -64,8 +56,8 @@ import { buildPullRequestReviewContextAttachment, buildPullRequestThreadContextAttachment, canAddPullRequestActivityToChat, - canAddPullRequestCheckLogsToChat, } from "./context-attachment"; +import { ChecksSection, getCheckIdentity } from "./checks-section"; import { getActivityVerb, getStateLabel } from "./data"; import type { PrPaneActivity, PrPaneCheck, PrPaneData, PrState } from "./data"; import type { ForgeSpecificStatusFacts } from "@/git/merge-capability"; @@ -76,11 +68,9 @@ import { type PrTimelineEntry, } from "./timeline"; import { - CheckStatusIcon, Section, SUMMARY_DANGER_ICON, SUMMARY_SUCCESS_ICON, - SUMMARY_WARNING_ICON, SummaryPill, dangerColorMapping, foregroundMutedColorMapping, @@ -145,10 +135,6 @@ function handleMarkdownLinkPress(url: string): boolean { return false; } -function rowPressableStyle({ hovered }: { hovered?: boolean }) { - return [sectionKitStyles.checkRow, Boolean(hovered) && styles.hoverable]; -} - function entryHeaderPressableStyle({ hovered }: { hovered?: boolean }) { return [styles.entryHeaderPressable, Boolean(hovered) && styles.hoverable]; } @@ -175,16 +161,6 @@ function renderKebabTriggerIcon({ hovered }: { hovered?: boolean }) { ); } -function getCheckIdentity(check: PrPaneCheck): string { - if (check.detailRef?.checkRunId !== undefined) { - return `${check.provider}:check-run:${check.detailRef.checkRunId}`; - } - if (check.detailRef?.workflowRunId !== undefined) { - return `${check.provider}:workflow-run:${check.detailRef.workflowRunId}`; - } - return `${check.provider}:${check.name}:${check.url}`; -} - function addLoadingCheck(current: ReadonlySet, checkKey: string): ReadonlySet { if (current.has(checkKey)) { return current; @@ -268,10 +244,6 @@ export function PullRequestPane({ setActivityOpen((open) => !open); }, []); - const passed = data.checks.filter((check) => check.status === "success").length; - const failed = data.checks.filter((check) => check.status === "failure").length; - const pending = data.checks.filter((check) => check.status === "pending").length; - const approvals = data.activity.filter( (item) => item.kind === "review" && item.reviewState === "approved", ).length; @@ -567,50 +539,14 @@ export function PullRequestPane({ {nativeChecksSection ?? ( -
- - - - - } - > - {data.checks.length === 0 ? ( - No checks - ) : ( - data.checks.map((check) => { - const checkKey = getCheckIdentity(check); - return ( - - ); - }) - )} -
+ attachEnabled={attachEnabled} + loadingCheckKeys={loadingCheckKeys} + onAddLogsToChat={handleAddCheckLogsToChat} + /> )} @@ -672,57 +608,6 @@ function stateLabelStyle(state: PrState) { return styles.stateLabelClosed; } -function CheckRow({ - check, - attachEnabled, - isAddingLogsToChat, - onAddLogsToChat, -}: { - check: PrPaneCheck; - attachEnabled: boolean; - isAddingLogsToChat: boolean; - onAddLogsToChat: (check: PrPaneCheck) => void; -}) { - const handlePress = useCallback(() => { - void openExternalUrl(check.url); - }, [check.url]); - const handleAddLogsToChat = useCallback( - (event: GestureResponderEvent) => { - event.stopPropagation(); - void onAddLogsToChat(check); - }, - [check, onAddLogsToChat], - ); - return ( - - - - {check.name} - - {check.workflow && ( - - {check.workflow} - - )} - - {attachEnabled && canAddPullRequestCheckLogsToChat(check) ? ( - - ) : null} - {check.duration && {check.duration}} - - - ); -} - interface TimelineEntryCallbacks { attachEnabled: boolean; brandLabel: string; diff --git a/packages/app/src/git/pull-request-panel/section-kit.tsx b/packages/app/src/git/pull-request-panel/section-kit.tsx index 9c64deb93..702d40865 100644 --- a/packages/app/src/git/pull-request-panel/section-kit.tsx +++ b/packages/app/src/git/pull-request-panel/section-kit.tsx @@ -9,6 +9,7 @@ import { CircleSlash, CircleX, } from "lucide-react-native"; +import { CONTROL_HEIGHTS } from "@/components/ui/control-geometry"; import type { Theme } from "@/styles/theme"; import type { CheckStatus } from "./check-status"; @@ -149,8 +150,11 @@ export const sectionKitStyles = StyleSheet.create((theme) => ({ alignItems: "center", gap: theme.spacing[2], paddingHorizontal: theme.spacing[3], - paddingVertical: theme.spacing[2], - minHeight: 32, + // Fixed, not minHeight plus padding. A row's trailing slot holds an xs Button + // (28pt), which stacked on the vertical padding and made rows with an + // "Add to chat" button taller than rows without one. The row is single-line by + // construction, so it declares a height the button fits inside instead. + height: CONTROL_HEIGHTS.compact, }, checkName: { fontSize: theme.fontSize.sm, diff --git a/packages/protocol/src/messages.ts b/packages/protocol/src/messages.ts index 80c02e16e..3570dc45e 100644 --- a/packages/protocol/src/messages.ts +++ b/packages/protocol/src/messages.ts @@ -4654,6 +4654,10 @@ export const CheckoutPrStatusSchema = z.object({ status: z.string(), url: z.string().nullable(), workflow: z.string().optional(), + /** + * Formatted by the forge adapter: how long a finished check took, or how long a + * running one has been going. Raw timestamps stay off the wire. + */ duration: z.string().optional(), checkRunId: z.number().optional(), workflowRunId: z.number().optional(), diff --git a/packages/server/src/services/forge-service.ts b/packages/server/src/services/forge-service.ts index 7f6d36465..4c50bc495 100644 --- a/packages/server/src/services/forge-service.ts +++ b/packages/server/src/services/forge-service.ts @@ -63,6 +63,7 @@ export interface PullRequestCheck { status: PullRequestCheckStatus; url: string | null; workflow?: string; + /** How long the check took, or how long it has been running. Formatted, never raw. */ duration?: string; checkRunId?: number; workflowRunId?: number; diff --git a/packages/server/src/services/github-service.test.ts b/packages/server/src/services/github-service.test.ts index c6370e84e..52423de4a 100644 --- a/packages/server/src/services/github-service.test.ts +++ b/packages/server/src/services/github-service.test.ts @@ -9,6 +9,7 @@ import { GitHubCommandError, computeGithubNextInterval, createGitHubService, + parseStatusCheckRollup, type GitHubCommandRunner, type GitHubCommandRunnerOptions, type CurrentPullRequestStatus, @@ -2830,6 +2831,49 @@ describe("ForgeService", () => { ).resolves.toBeNull(); }); + it("measures a still-running check to now, so a pending check reports a duration too", () => { + const nowMs = Date.parse("2026-04-02T13:55:00Z"); + + const checks = parseStatusCheckRollup( + [ + { + __typename: "CheckRun", + name: "still-going", + status: "IN_PROGRESS", + conclusion: null, + detailsUrl: "https://github.com/acme/repo/runs/1", + startedAt: "2026-04-02T13:50:00Z", + completedAt: null, + }, + { + __typename: "CheckRun", + name: "queued", + status: "QUEUED", + conclusion: null, + detailsUrl: "https://github.com/acme/repo/runs/2", + startedAt: null, + completedAt: null, + }, + { + __typename: "CheckRun", + name: "finished", + status: "COMPLETED", + conclusion: "SUCCESS", + detailsUrl: "https://github.com/acme/repo/runs/3", + startedAt: "2026-04-02T13:50:00Z", + completedAt: "2026-04-02T13:52:14Z", + }, + ], + nowMs, + ); + + expect(checks.map((check) => [check.name, check.duration])).toEqual([ + ["still-going", "5m"], + ["queued", undefined], + ["finished", "2m 14s"], + ]); + }); + it("keeps S1 PR status schema additions optional and strips internal check timestamps", () => { const oldDaemonResponse = CheckoutPrStatusResponseSchema.parse({ type: "checkout_pr_status_response", diff --git a/packages/server/src/services/github-service.ts b/packages/server/src/services/github-service.ts index d43916cb6..db2d8b56f 100644 --- a/packages/server/src/services/github-service.ts +++ b/packages/server/src/services/github-service.ts @@ -2862,14 +2862,14 @@ function parseGitHubPullRequestRepo(url: string): { owner: string; name: string } } -export function parseStatusCheckRollup(value: unknown): PullRequestCheck[] { +export function parseStatusCheckRollup(value: unknown, nowMs = Date.now()): PullRequestCheck[] { const directContexts = PullRequestStatusCheckRollupArraySchema.safeParse(value); if (!directContexts.success) { const legacyContexts = LegacyPullRequestStatusCheckRollupSchema.safeParse(value); if (!legacyContexts.success) { return []; } - return parseStatusCheckRollup(legacyContexts.data.contexts); + return parseStatusCheckRollup(legacyContexts.data.contexts, nowMs); } const dedupedChecks = new Map(); @@ -2878,7 +2878,7 @@ export function parseStatusCheckRollup(value: unknown): PullRequestCheck[] { if (!parsed.success) { continue; } - const check = buildPullRequestCheck(parsed.data); + const check = buildPullRequestCheck(parsed.data, nowMs); if (!check) { continue; } @@ -2893,6 +2893,7 @@ export function parseStatusCheckRollup(value: unknown): PullRequestCheck[] { function buildPullRequestCheck( context: z.infer, + nowMs: number, ): (PullRequestCheck & { recency: number }) | null { if (context.__typename === "CheckRun") { return { @@ -2906,7 +2907,7 @@ function buildPullRequestCheck( ...(typeof context.checkSuite?.workflowRun?.databaseId === "number" ? { workflowRunId: context.checkSuite.workflowRun.databaseId } : {}), - ...formatCheckRunDuration(context), + ...formatCheckRunDuration(context, nowMs), recency: getCheckRunRecency(context), }; } @@ -2965,13 +2966,26 @@ function getCheckRunRecency(context: PullRequestCheckRunNode): number { return parseOptionalTime(context.completedAt ?? context.startedAt ?? null); } -function formatCheckRunDuration(context: PullRequestCheckRunNode): { duration?: string } { +/** + * How long the check ran for. A finished run measures to its completion; a run still + * going measures to now, so a client can say how long it has been waiting instead of + * showing nothing. Raw timestamps never reach the client, so this is where the choice + * between the two has to be made. + */ +function formatCheckRunDuration( + context: PullRequestCheckRunNode, + nowMs: number, +): { duration?: string } { const startedAt = parseOptionalTime(context.startedAt ?? null); - const completedAt = parseOptionalTime(context.completedAt ?? null); - if (startedAt <= 0 || completedAt <= 0 || completedAt < startedAt) { + if (startedAt <= 0) { return {}; } - const durationSeconds = Math.floor((completedAt - startedAt) / 1_000); + const completedAt = parseOptionalTime(context.completedAt ?? null); + const endedAt = completedAt > 0 ? completedAt : nowMs; + if (endedAt < startedAt) { + return {}; + } + const durationSeconds = Math.floor((endedAt - startedAt) / 1_000); return { duration: formatDurationSeconds(durationSeconds) }; }