Summarize pull request checks and group them by status (#3483)
* fix(app): keep check rows the same height with and without "Add to chat" A check row was minHeight 32 plus 8pt of vertical padding, but the xs Button in its trailing slot is 28pt tall. The button stacked on the padding rather than fitting inside the row, so a row grew to 44pt the moment "Add to chat" appeared and the list visibly shifted as rows gained the button. Rows are single-line by construction, so the row now declares a fixed height instead of a floor a child can outgrow. That height comes from CONTROL_HEIGHTS, the same source the button sizes from, so the two cannot drift apart later. GitLab pipeline rows share this style and get the fix too. * feat(app): summarize pull request checks and group them by status The checks section listed every check inline under a bare "Checks" header with three count pills. On a repo with a real matrix that meant a wall of undifferentiated rows that pushed the activity timeline off the pane, and the pills made the reader add up numbers to work out whether the run was actually in trouble. The section now leads with the answer — a headline that names the outcome, a one-line count breakdown, and a ring showing the statuses in proportion — then groups the rows by status with failures first, and scrolls in place past roughly eight rows instead of growing without bound. Groups collapse individually. The derivation lives in checks-summary.ts as one pure function, so the headline, the ring, and the group labels cannot disagree about the same run. The rendering, the row, and the check identity move out of pane.tsx into an owned module; the pane now wires it and nothing more. Running checks also gain a timing. formatCheckRunDuration required both a start and a completion, so anything still going showed nothing at all. It now measures an unfinished run to now. The wire deliberately carries formatted strings rather than raw timestamps — there is a test asserting check timestamps are stripped — so the daemon does the measuring and the client only decides how to say it: a bare "2m" on a running row would read as "took 2m", so those rows say "running 2m".
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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<string, PullRequestCheck & { recency: number }>();
|
||||
@@ -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<typeof PullRequestStatusCheckRollupNodeSchema>,
|
||||
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) };
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user