From 96132271ccab3b4228d6ff9cb165622d5fbe49d3 Mon Sep 17 00:00:00 2001 From: omartelo Date: Sat, 17 Oct 2026 19:52:24 -0300 Subject: [PATCH] feat(native): label the checks that gate a pull request's merge gh pr view --json statusCheckRollup carries no isRequired, so the detail asks GitHub's GraphQL for it once the pull request reports checks, and the native Checks tab marks those rows Required, as GitHub's own list does. --- CHANGELOG.md | 6 ++ frontend/src/lib/api-types.ts | 2 + frontend/src/lib/pulls/pr-handoff.test.ts | 1 + internal/project/pr.go | 9 ++- internal/project/prchecks.go | 89 ++++++++++++++++++++++- internal/project/prchecks_test.go | 60 ++++++++++++++- native/cmd/lich-native/pulls_panels.go | 6 ++ native/i18n/locales/en/pulls.json | 3 +- native/i18n/locales/es/pulls.json | 3 +- native/i18n/locales/ja/pulls.json | 3 +- native/i18n/locales/ko/pulls.json | 3 +- native/i18n/locales/pt-br/pulls.json | 3 +- native/i18n/locales/ru/pulls.json | 3 +- native/i18n/locales/zh-cn/pulls.json | 3 +- native/lichclient/pulls.go | 1 + 15 files changed, 185 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f79883db8..caac9ff0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **Required checks are labelled.** In the native window, a pull request's + Checks tab marks the checks that gate its merge with Required, as GitHub + does. + ### Fixed - **What's new follows a language you switch to.** In the native window, the diff --git a/frontend/src/lib/api-types.ts b/frontend/src/lib/api-types.ts index 5b85b9449..7fd80c9a9 100644 --- a/frontend/src/lib/api-types.ts +++ b/frontend/src/lib/api-types.ts @@ -120,6 +120,8 @@ export interface CheckItem { /** gh's ISO timestamps; "" while a check has not started or finished. */ startedAt: string completedAt: string + /** The check gates the merge, as GitHub's own checks list labels Required. */ + required: boolean } /** internal/project.PRCommit — one commit of the PR, for the Commits tab. */ diff --git a/frontend/src/lib/pulls/pr-handoff.test.ts b/frontend/src/lib/pulls/pr-handoff.test.ts index da59503f8..af49d3629 100644 --- a/frontend/src/lib/pulls/pr-handoff.test.ts +++ b/frontend/src/lib/pulls/pr-handoff.test.ts @@ -33,6 +33,7 @@ const run = (name: string, state: CheckItem["state"] = "failed"): CheckItem => ( url: `https://github.com/o/l/runs/${name}`, startedAt: "", completedAt: "", + required: false, }) const red = (names: string[]): Partial => ({ diff --git a/internal/project/pr.go b/internal/project/pr.go index 3dc20ab06..5b7f2b0cc 100644 --- a/internal/project/pr.go +++ b/internal/project/pr.go @@ -213,7 +213,14 @@ func (s *Service) PullRequestDetail(path string, number int) (*PRDetail, error) // The OPEN-only gate belongs to the branch lookup alone. Naming a number is // asking for that pull request, whatever became of it — the list can show // merged and closed ones, and a row that cannot be opened is not a row. - return parsePRDetail(out, number == 0) + detail, err := parsePRDetail(out, number == 0) + if err != nil || detail == nil || len(detail.CheckRuns) == 0 { + return detail, err + } + if err := s.markRequiredChecks(path, detail.Number, detail.CheckRuns); err != nil { + return nil, err + } + return detail, nil } // parsePRDetail decodes gh's JSON and reduces the check rollup. With openOnly it diff --git a/internal/project/prchecks.go b/internal/project/prchecks.go index fed730a0f..f1df4bb35 100644 --- a/internal/project/prchecks.go +++ b/internal/project/prchecks.go @@ -1,6 +1,10 @@ package project -import "sort" +import ( + "encoding/json" + "fmt" + "sort" +) // gh reports a pull request's CI as a mixed array of two unrelated shapes. This // file is the whole translation from that array into the two things the screen @@ -27,6 +31,9 @@ type CheckItem struct { URL string `json:"url"` // where to read the run; "" when gh reports none StartedAt string `json:"startedAt"` CompletedAt string `json:"completedAt"` + // Required is GitHub's own answer to "does this check gate the merge", the + // same verdict its checks list labels Required. + Required bool `json:"required"` } // checkItem is one statusCheckRollup entry. gh emits two shapes: a CheckRun @@ -128,3 +135,83 @@ func toCheckItems(items []checkItem) []CheckItem { }) return out } + +// requiredChecksQuery asks GitHub which of the head commit's checks gate this +// pull request's merge. gh's `pr view --json statusCheckRollup` does not carry +// isRequired, which takes the pull request as an argument. The aliases fold a +// StatusContext into a CheckRun's field names. +// ponytail: first 100 checks only; a check past that never reads Required. +const requiredChecksQuery = `query($owner:String!,$repo:String!,$number:Int!){ + repository(owner:$owner,name:$repo){ + pullRequest(number:$number){ + commits(last:1){nodes{commit{statusCheckRollup{contexts(first:100){nodes{ + ... on CheckRun{name detailsUrl isRequired(pullRequestNumber:$number)} + ... on StatusContext{name:context detailsUrl:targetUrl isRequired(pullRequestNumber:$number)} + }}}}}} + } + } +}` + +// markRequiredChecks flags the checks that gate the merge of pull request +// number. Checks are matched by name and URL: a name alone repeats across +// workflows ("test" in two of them), and the URL names the run. +func (s *Service) markRequiredChecks(path string, number int, checks []CheckItem) error { + out, err := s.gh(prReadTimeout, path, "api", "graphql", + "-F", "owner={owner}", "-F", "repo={repo}", + "-F", fmt.Sprintf("number=%d", number), + "-f", "query="+requiredChecksQuery) + if err != nil { + return err + } + required, err := parseRequiredChecks(out) + if err != nil { + return err + } + for i := range checks { + checks[i].Required = required[checkKey(checks[i].Name, checks[i].URL)] + } + return nil +} + +func checkKey(name, url string) string { return name + "\n" + url } + +func parseRequiredChecks(out []byte) (map[string]bool, error) { + var payload struct { + Data struct { + Repository struct { + PullRequest struct { + Commits struct { + Nodes []struct { + Commit struct { + StatusCheckRollup *struct { + Contexts struct { + Nodes []struct { + Name string `json:"name"` + DetailsURL string `json:"detailsUrl"` + IsRequired bool `json:"isRequired"` + } `json:"nodes"` + } `json:"contexts"` + } `json:"statusCheckRollup"` + } `json:"commit"` + } `json:"nodes"` + } `json:"commits"` + } `json:"pullRequest"` + } `json:"repository"` + } `json:"data"` + } + if err := json.Unmarshal(out, &payload); err != nil { + return nil, errUnreadableAnswer(err) + } + required := map[string]bool{} + for _, c := range payload.Data.Repository.PullRequest.Commits.Nodes { + if c.Commit.StatusCheckRollup == nil { + continue + } + for _, n := range c.Commit.StatusCheckRollup.Contexts.Nodes { + if n.IsRequired { + required[checkKey(n.Name, n.DetailsURL)] = true + } + } + } + return required, nil +} diff --git a/internal/project/prchecks_test.go b/internal/project/prchecks_test.go index 0975ec673..cf1b1178d 100644 --- a/internal/project/prchecks_test.go +++ b/internal/project/prchecks_test.go @@ -1,6 +1,11 @@ package project -import "testing" +import ( + "errors" + "maps" + "slices" + "testing" +) // TestIsFailureConclusion pins every conclusion gh reports, both halves of the // list. It is an allow-list of failures, so a literal dropped from it does not @@ -31,3 +36,56 @@ func TestIsFailureConclusion(t *testing.T) { }) } } + +// TestPullRequestDetailMarksRequiredChecks pins Required to GitHub's isRequired, +// matched per run: "test" runs in two workflows and only the URL tells them +// apart. +func TestPullRequestDetailMarksRequiredChecks(t *testing.T) { + t.Parallel() + view := []byte(`{"number":818,"state":"OPEN","statusCheckRollup":[ + {"__typename":"CheckRun","name":"test","status":"COMPLETED","conclusion":"SUCCESS","workflowName":"ci","detailsUrl":"https://x/ci"}, + {"__typename":"CheckRun","name":"test","status":"COMPLETED","conclusion":"SUCCESS","workflowName":"native","detailsUrl":"https://x/native"}, + {"__typename":"StatusContext","context":"deploy","state":"SUCCESS","targetUrl":"https://x/deploy"} + ]}`) + required := []byte(`{"data":{"repository":{"pullRequest":{"commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{"nodes":[ + {"name":"test","detailsUrl":"https://x/ci","isRequired":true}, + {"name":"test","detailsUrl":"https://x/native","isRequired":false}, + {"name":"deploy","detailsUrl":"https://x/deploy","isRequired":true} + ]}}}}]}}}}}`) + gh := &fakeGH{outs: [][]byte{view, required}} + pr, err := withGH(gh).PullRequestDetail("/repo", 0) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + got := map[string]bool{} + for _, c := range pr.CheckRuns { + got[c.URL] = c.Required + } + want := map[string]bool{"https://x/ci": true, "https://x/native": false, "https://x/deploy": true} + if !maps.Equal(got, want) { + t.Errorf("required = %v, want %v", got, want) + } + if !slices.Contains(gh.history[1], "number=818") { + t.Errorf("lookup args = %v, want the PR number gh resolved", gh.history[1]) + } +} + +func TestPullRequestDetailRequiredLookupFailure(t *testing.T) { + t.Parallel() + view := []byte(`{"number":7,"state":"OPEN","statusCheckRollup":[{"name":"test","status":"IN_PROGRESS"}]}`) + gh := &fakeGH{outs: [][]byte{view, nil}, errs: []error{nil, errors.New("gh is not signed in to GitHub.")}} + if _, err := withGH(gh).PullRequestDetail("/repo", 0); err == nil { + t.Fatal("expected the lookup's error") + } +} + +func TestPullRequestDetailWithoutChecksSkipsTheLookup(t *testing.T) { + t.Parallel() + gh := &fakeGH{out: []byte(`{"number":7,"state":"OPEN"}`)} + if _, err := withGH(gh).PullRequestDetail("/repo", 0); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gh.calls != 1 { + t.Errorf("gh calls = %d, want 1", gh.calls) + } +} diff --git a/native/cmd/lich-native/pulls_panels.go b/native/cmd/lich-native/pulls_panels.go index 9e46c06cd..29078a68f 100644 --- a/native/cmd/lich-native/pulls_panels.go +++ b/native/cmd/lich-native/pulls_panels.go @@ -375,6 +375,12 @@ func (v *pullView) checkRows(gtx C, th *ui.Theme, d lichclient.PRDetail, act *pu }), ) }), + layout.Rigid(func(gtx C) D { + if !c.Required { + return D{} + } + return layout.Inset{Right: ui.Space(3)}.Layout(gtx, paletteChip(th, i18n.T("pulls.pullsChecks.required"), th.MutedForeground)) + }), layout.Rigid(th.Text(ui.TextXS, checkDuration(c.StartedAt, c.CompletedAt, now)).Muted().Layout), ) }) diff --git a/native/i18n/locales/en/pulls.json b/native/i18n/locales/en/pulls.json index 41312d35a..7e6adb540 100644 --- a/native/i18n/locales/en/pulls.json +++ b/native/i18n/locales/en/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "A ruleset governs {base}: every commit {clauses}." }, "pullsChecks": { - "none": "No checks reported." + "none": "No checks reported.", + "required": "Required" }, "pullsCommits": { "none": "No commits." diff --git a/native/i18n/locales/es/pulls.json b/native/i18n/locales/es/pulls.json index 8dc863c7b..3d90b7cc2 100644 --- a/native/i18n/locales/es/pulls.json +++ b/native/i18n/locales/es/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "Un conjunto de reglas gobierna {base}: cada commit {clauses}." }, "pullsChecks": { - "none": "No se informó ninguna comprobación." + "none": "No se informó ninguna comprobación.", + "required": "Obligatorio" }, "pullsCommits": { "none": "No hay commits." diff --git a/native/i18n/locales/ja/pulls.json b/native/i18n/locales/ja/pulls.json index 160997f0a..c283b3935 100644 --- a/native/i18n/locales/ja/pulls.json +++ b/native/i18n/locales/ja/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "{base} にはルールセットが適用されています: すべてのコミットについて、{clauses}。" }, "pullsChecks": { - "none": "報告されたチェックはありません。" + "none": "報告されたチェックはありません。", + "required": "必須" }, "pullsCommits": { "none": "コミットはありません。" diff --git a/native/i18n/locales/ko/pulls.json b/native/i18n/locales/ko/pulls.json index fc2b19cb6..7ab46bb4c 100644 --- a/native/i18n/locales/ko/pulls.json +++ b/native/i18n/locales/ko/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "{base}에 규칙 세트가 적용됩니다. 모든 커밋에서 {clauses}." }, "pullsChecks": { - "none": "보고된 체크가 없습니다." + "none": "보고된 체크가 없습니다.", + "required": "필수" }, "pullsCommits": { "none": "커밋이 없습니다." diff --git a/native/i18n/locales/pt-br/pulls.json b/native/i18n/locales/pt-br/pulls.json index e23cd815f..e7b446ea6 100644 --- a/native/i18n/locales/pt-br/pulls.json +++ b/native/i18n/locales/pt-br/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "Um ruleset governa {base}: em todo commit, {clauses}." }, "pullsChecks": { - "none": "Nenhuma verificação reportada." + "none": "Nenhuma verificação reportada.", + "required": "Obrigatório" }, "pullsCommits": { "none": "Nenhum commit." diff --git a/native/i18n/locales/ru/pulls.json b/native/i18n/locales/ru/pulls.json index 8ab9a9298..dccffe9e5 100644 --- a/native/i18n/locales/ru/pulls.json +++ b/native/i18n/locales/ru/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "Для {base} действует набор правил. Каждый коммит: {clauses}." }, "pullsChecks": { - "none": "Проверок нет." + "none": "Проверок нет.", + "required": "Обязательный" }, "pullsCommits": { "none": "Коммитов нет." diff --git a/native/i18n/locales/zh-cn/pulls.json b/native/i18n/locales/zh-cn/pulls.json index 354698e4e..32950f8fd 100644 --- a/native/i18n/locales/zh-cn/pulls.json +++ b/native/i18n/locales/zh-cn/pulls.json @@ -51,7 +51,8 @@ "ruleNote": "规则集约束着 {base}：对每个提交，{clauses}。" }, "pullsChecks": { - "none": "没有检查结果。" + "none": "没有检查结果。", + "required": "必需" }, "pullsCommits": { "none": "没有提交。" diff --git a/native/lichclient/pulls.go b/native/lichclient/pulls.go index 1bbfb5635..57e29e5f8 100644 --- a/native/lichclient/pulls.go +++ b/native/lichclient/pulls.go @@ -28,6 +28,7 @@ type CheckItem struct { URL string `json:"url"` StartedAt string `json:"startedAt"` CompletedAt string `json:"completedAt"` + Required bool `json:"required"` } // PRSummary mirrors project.PRSummary: one row of the list.