crueber/walhub

Watch

Fork 0

PR with zero reported check contexts shows indefinite 'pending' instead of a 'No checks configured' empty state with reporting guidance #518

New issue

Closed

opened 2026-09-14 15:45:27 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 15:45:27 +00:00

Owner

What's requested

A PR whose head SHA has zero reported check contexts currently renders the checks card with a combined state of pending — indistinguishable from a CI run genuinely in flight — and offers no guidance that nothing has reported at all. It should show an explicit "No checks configured / no checks reported yet" state, with guidance on how checks get reported (a CI bot POSTs statuses with a wct_ token), and the merge-block reason should be surfaced in the same breath when required checks are configured.

Current behavior (static analysis; no local repro)

internal/checks/model.go:189-202 — combinedState deliberately maps zero contexts ⇒ pending ("a caller cannot distinguish 'not started' from 'in flight'"). That's the right wire semantics for the gate, but the UI inherits it verbatim:
web/src/pages/Checks.jsx:28 — stateLabel(state) returns state ?? "pending", and CheckPill renders the amber pending dot + label for the empty case.
web/src/pages/Pull.jsx:762-782 — the PR checks card renders CheckPill + ContextRows. ContextRows shows "no contexts reported" in its empty fallback (good), but the card header still shows the amber "pending" pill, so the user reads the PR as waiting on CI when no CI is configured or nothing has reported.
internal/checks/service.go:329 — the combined view maps "no index entry" to pending as well, so the server never distinguishes the two either.

Merge-block reasons

Blockers ARE surfaced today (Pull.jsx:774-778, MergeBox.jsx:63-70,189-192: "blocking merge: (missing)/; changes requested; conflicts: …"), but only for required checks (require_checks in policy rules matching the base branch, resolved client-side at Pull.jsx:619-638). When zero contexts exist and no required checks are configured, nothing tells the user why the checks pill says pending or that merge will proceed without any check signal. The "no checks" empty state is the place to say it.

Architecture notes

Keep the wire contract as-is: combinedState's zero-contexts⇒pending rule is load-bearing for the merge gate (internal/checks/gate_test.go:48-52, model_test.go:67-76). This is a client-only display fix: the UI can distinguish "no contexts" from "pending" using data it already fetches — checks.statuses(sha) returns an empty statuses array (no 404), and the combined view carries total_counts (see internal/checks/http_test.go:192).
ContextRows already has the empty fallback string; the fix is promoting that to a proper empty-state block on the PR checks card (and the CheckDetail page) and rendering a neutral "no checks" pill instead of the amber pending one when statuses is empty.
Reporting guidance copy should point at the API: POST …/api/checks/statuses/{sha} with a wct_ CI token (internal/checks/auth.go, internal/checks/http.go); link the checks page / API docs page rather than inventing new copy surface.

Acceptance criteria

PR checks card with zero reported contexts shows "No checks reported" (or "No checks configured" when no require_checks rules match the base branch) instead of an amber "pending" pill
The empty state includes brief reporting guidance (CI reports via POST …/api/checks/statuses/{sha} using a wct_ token; link to API docs / checks page)
A PR with ≥1 context (any state, including pending) is NOT affected — it still renders the real combined state
Merge-block reasons remain surfaced and cover the zero-contexts case: when required checks exist but none have reported, the blocker line reads e.g. <context> (missing) and the merge button tooltip states it
No server API change; combinedState wire behavior and its pinned tests stay untouched
stateLabel/stateDot handle the "no checks" presentation without regressing the commit-page pill usages

## What's requested A PR whose head SHA has **zero reported check contexts** currently renders the checks card with a combined state of `pending` — indistinguishable from a CI run genuinely in flight — and offers no guidance that nothing has reported at all. It should show an explicit **"No checks configured / no checks reported yet"** state, with guidance on how checks get reported (a CI bot POSTs statuses with a `wct_` token), and the merge-block reason should be surfaced in the same breath when required checks are configured. ### Current behavior (static analysis; no local repro) - `internal/checks/model.go:189-202` — `combinedState` deliberately maps **zero contexts ⇒ `pending`** ("a caller cannot distinguish 'not started' from 'in flight'"). That's the right wire semantics for the *gate*, but the UI inherits it verbatim: - `web/src/pages/Checks.jsx:28` — `stateLabel(state)` returns `state ?? "pending"`, and `CheckPill` renders the amber pending dot + label for the empty case. - `web/src/pages/Pull.jsx:762-782` — the PR checks card renders `CheckPill` + `ContextRows`. `ContextRows` shows "no contexts reported" in its empty fallback (good), but the card header still shows the amber "pending" pill, so the user reads the PR as *waiting on CI* when no CI is configured or nothing has reported. - `internal/checks/service.go:329` — the combined view maps "no index entry" to pending as well, so the server never distinguishes the two either. ### Merge-block reasons Blockers ARE surfaced today (`Pull.jsx:774-778`, `MergeBox.jsx:63-70,189-192`: "blocking merge: <ctx> (missing)/<state>; changes requested; conflicts: …"), but only for **required** checks (`require_checks` in policy rules matching the base branch, resolved client-side at `Pull.jsx:619-638`). When zero contexts exist and no required checks are configured, nothing tells the user *why* the checks pill says pending or that merge will proceed without any check signal. The "no checks" empty state is the place to say it. ### Architecture notes - Keep the wire contract as-is: `combinedState`'s zero-contexts⇒pending rule is load-bearing for the merge gate (`internal/checks/gate_test.go:48-52`, `model_test.go:67-76`). This is a **client-only** display fix: the UI can distinguish "no contexts" from "pending" using data it already fetches — `checks.statuses(sha)` returns an empty `statuses` array (no 404), and the combined view carries `total_counts` (see `internal/checks/http_test.go:192`). - `ContextRows` already has the empty fallback string; the fix is promoting that to a proper empty-state block on the PR checks card (and the CheckDetail page) and rendering a neutral "no checks" pill instead of the amber pending one when `statuses` is empty. - Reporting guidance copy should point at the API: `POST …/api/checks/statuses/{sha}` with a `wct_` CI token (`internal/checks/auth.go`, `internal/checks/http.go`); link the checks page / API docs page rather than inventing new copy surface. ## Acceptance criteria - [ ] PR checks card with zero reported contexts shows "No checks reported" (or "No checks configured" when no `require_checks` rules match the base branch) instead of an amber "pending" pill - [ ] The empty state includes brief reporting guidance (CI reports via `POST …/api/checks/statuses/{sha}` using a `wct_` token; link to API docs / checks page) - [ ] A PR with ≥1 context (any state, including pending) is NOT affected — it still renders the real combined state - [ ] Merge-block reasons remain surfaced and cover the zero-contexts case: when required checks exist but none have reported, the blocker line reads e.g. `<context> (missing)` and the merge button tooltip states it - [ ] No server API change; `combinedState` wire behavior and its pinned tests stay untouched - [ ] `stateLabel`/`stateDot` handle the "no checks" presentation without regressing the commit-page pill usages

crueber added the

pr/merge protection rules

pull requests

labels

2026-09-14 15:45:42 +00:00

crueber added this to the v1 milestone

2026-09-14 15:45:42 +00:00

crueber referenced this issue from a commit

2026-09-14 17:02:11 +00:00

Fix #518: zero-checks empty state (client display only)

crueber referenced this issue from a pull request that will close it,

2026-09-14 17:02:15 +00:00

Fix #518: zero-checks empty state #524

crueber commented

2026-09-14 17:02:19 +00:00

Author

Owner

Fixed by #524 (#524): client display only, wire contract untouched. Zero-contexts shas now render a neutral No checks configured/reported block with reporting guidance (wct_ + /api#checks-ci) on the PR card and CheckDetail page; ≥1 context keeps the real pill; blockers/tooltip unchanged.

Fixed by #524 (https://git.packden.us/crueber/walhub/pulls/524): client display only, wire contract untouched. Zero-contexts shas now render a neutral No checks configured/reported block with reporting guidance (wct_ + /api#checks-ci) on the PR card and CheckDetail page; ≥1 context keeps the real pill; blockers/tooltip unchanged.

crueber commented

2026-09-14 17:08:28 +00:00

Author

Owner

Review of PR #524 (fix/issue-518, commit 8a1354a) — independent verification in a scratch worktree (created /tmp/pr524, removed afterward; main worktree untouched, tracked-clean before/after).

ACCEPTANCE vs issue #518 — all six criteria hold:
(1) Empty-vs-pending: web/src/lib/checks-empty.js:16-21 isZeroChecks is true ONLY for a zero-length statuses array; null/undefined/missing-statuses (loading/unknown) return false, and a single pending context returns false (still in-flight). PR card (Pull.jsx:637) and CheckPill (Checks.jsx:42) key off the statuses array, never the wire state string.
(2) Titles: checks-empty.js:28-31 zeroChecksTitle returns 'No checks configured' iff required is a known-empty array, else 'No checks reported yet' (unknown-required on CheckDetail correctly reads 'reported'). Matches the issue's configured-vs-reported wording.
(3) Blockers: Pull.jsx:632 delegates to requiredCheckBlockers with identical semantics to the removed inline code (same Map/filter/map, incl. ' (missing)'); blocking-merge lines intact (Pull.jsx:849, MergeBox.jsx:191) and the required: line (Pull.jsx:844) still renders alongside the new block.
(4) Wire untouched: zero diff under internal/; combinedState zero=>pending and its pinned gate/model tests untouched; go test ./internal/checks/... green (2.4s, includes gate tests).
(5) Guidance: existing surfaces only — POST …/checks/statuses/{sha} matches the server route (internal/checks/http.go:32), wct_ matches internal/checks/auth.go:12, /api#checks-ci anchor exists (Apidocs.jsx:282), checks-page link uses the real /:owner/:name/checks route.
(6) CheckDetail: CheckDetail.jsx:26-28 reuses the exact CheckPill data key (checks:full:sha), so no extra fetch; zero-case swaps ContextRows for ZeroChecksBlock, non-zero renders exactly as before.
(7) Merge-button tooltip: MergeBox.jsx untouched; tooltip derives from the same blocker strings, so the zero+required case still reads 'blocked: (missing); …'.
(8) No backend change, no new deps (no package.json diff; checks-empty.js is import-free), Law-12 decision appended to docs/features/05_checks_statuses.md:327-338.

TESTS (scratch worktree, node_modules symlinked from main per instructions):

New checks-empty-518.test.js (8 tests) + sdk-checks + checks-tab-505/513 + pull-state-517/ pull-state: 34/34 pass.
Full unit suite minus smoke: 1167/1167 pass.
web vite build: clean (2.2s).
go test ./internal/checks/...: ok.
smoke.test.js fails (403 vs live 127.0.0.1:8080) — confirmed environmental and pre-existing: the file skips only when the server is unreachable, but a live instance IS up and answers 403 before any JS runs; this client-only PR cannot affect it (no server code touched). Out of scope, but the skip-guard fragility (reachable-but-gated) may deserve its own issue.
No browser drive (per task note): node tests + reasoning only; mobile-viewport check still owed at merge time as the PR body notes.

Non-blocking note: the checks INDEX page rows (Checks.jsx ShaRow, stateDot/stateLabel direct) still show the amber pending dot for zero-context shas — the 'no contexts' text is already there, so it is far less misleading than the PR card was, but a follow-up could extend the neutral treatment there. Out of the #518 scope; not a merge blocker.

No fixes needed — nothing pushed.

MERGE RECOMMENDATION: ready to merge (modulo the already-noted mobile-viewport check at merge time).

Review of PR #524 (fix/issue-518, commit 8a1354a) — independent verification in a scratch worktree (created /tmp/pr524, removed afterward; main worktree untouched, tracked-clean before/after). ACCEPTANCE vs issue #518 — all six criteria hold: (1) Empty-vs-pending: web/src/lib/checks-empty.js:16-21 isZeroChecks is true ONLY for a zero-length statuses array; null/undefined/missing-statuses (loading/unknown) return false, and a single pending context returns false (still in-flight). PR card (Pull.jsx:637) and CheckPill (Checks.jsx:42) key off the statuses array, never the wire state string. (2) Titles: checks-empty.js:28-31 zeroChecksTitle returns 'No checks configured' iff required is a known-empty array, else 'No checks reported yet' (unknown-required on CheckDetail correctly reads 'reported'). Matches the issue's configured-vs-reported wording. (3) Blockers: Pull.jsx:632 delegates to requiredCheckBlockers with identical semantics to the removed inline code (same Map/filter/map, incl. '<ctx> (missing)'); blocking-merge lines intact (Pull.jsx:849, MergeBox.jsx:191) and the required: line (Pull.jsx:844) still renders alongside the new block. (4) Wire untouched: zero diff under internal/; combinedState zero=>pending and its pinned gate/model tests untouched; go test ./internal/checks/... green (2.4s, includes gate tests). (5) Guidance: existing surfaces only — POST …/checks/statuses/{sha} matches the server route (internal/checks/http.go:32), wct_ matches internal/checks/auth.go:12, /api#checks-ci anchor exists (Apidocs.jsx:282), checks-page link uses the real /:owner/:name/checks route. (6) CheckDetail: CheckDetail.jsx:26-28 reuses the exact CheckPill data key (checks:full:sha), so no extra fetch; zero-case swaps ContextRows for ZeroChecksBlock, non-zero renders exactly as before. (7) Merge-button tooltip: MergeBox.jsx untouched; tooltip derives from the same blocker strings, so the zero+required case still reads 'blocked: <ctx> (missing); …'. (8) No backend change, no new deps (no package.json diff; checks-empty.js is import-free), Law-12 decision appended to docs/features/05_checks_statuses.md:327-338. TESTS (scratch worktree, node_modules symlinked from main per instructions): - New checks-empty-518.test.js (8 tests) + sdk-checks + checks-tab-505/513 + pull-state-517/ pull-state: 34/34 pass. - Full unit suite minus smoke: 1167/1167 pass. - web vite build: clean (2.2s). - go test ./internal/checks/...: ok. - smoke.test.js fails (403 vs live 127.0.0.1:8080) — confirmed environmental and pre-existing: the file skips only when the server is unreachable, but a live instance IS up and answers 403 before any JS runs; this client-only PR cannot affect it (no server code touched). Out of scope, but the skip-guard fragility (reachable-but-gated) may deserve its own issue. - No browser drive (per task note): node tests + reasoning only; mobile-viewport check still owed at merge time as the PR body notes. Non-blocking note: the checks INDEX page rows (Checks.jsx ShaRow, stateDot/stateLabel direct) still show the amber pending dot for zero-context shas — the 'no contexts' text is already there, so it is far less misleading than the PR card was, but a follow-up could extend the neutral treatment there. Out of the #518 scope; not a merge blocker. No fixes needed — nothing pushed. MERGE RECOMMENDATION: ready to merge (modulo the already-noted mobile-viewport check at merge time).

crueber closed this issue

2026-09-14 17:10:36 +00:00

crueber referenced this issue from a commit

2026-09-14 17:10:36 +00:00

Merge pull request 'Fix #518: zero-checks empty state' (#524) from fix/issue-518 into main

crueber commented

2026-09-14 17:10:36 +00:00

Author

Owner

Fixed by PR #524 (review clean — all 8 checks pass, wire untouched, blockers verbatim), merged. Closing.