fix(sidebar): stop the CI spinner on a closed PR chip

[darko-mesaros]

Fixes #1020

Problem

A session whose transcript mentions a closed (not merged) pull request renders its sidebar chip as #993 Closed plus a "checks running" spinner that never stops. The user sees a session advertising live CI work on a pull request that can never merge and that nobody is waiting for.

The spinner cannot settle on its own. A closed PR's check rollup never advances: GitHub parks fork-PR checks in PENDING / ACTION_REQUIRED when the PR is closed before a maintainer approves the run, so _rollup_ci keeps correctly projecting ci: "running" for as long as the link stays in the transcript. The chip spins for the life of the session.

Why it matters

The chip is a status signal, and a permanently-spinning one is worse than no signal: it claims work is in flight on a dead pull request, and the sidebar's own "Closed" badge sits right next to it saying the opposite. The same chip also disagreed with the Changes panel, which already suppressed CI for closed pull requests — so the two surfaces rendered two different lifecycles for the same PR, which is exactly the class of divergence #443 set out to eliminate.

Fix (symptoms → root cause → change)

Symptom: a closed chip shows the red "Closed" label and an endless spinner.

Root cause: the chip gated all three CI glyphs on link.state !== 'merged' — a per-glyph inline literal that covered only one of the two terminal states:

{link.state === 'closed' && <span className="capitalize text-danger">{link.state}</span>}
{link.state !== 'merged' && link.ci === 'running' && <Loader2 … animate-spin />}
{link.state !== 'merged' && link.ci === 'passed'  && <Check … />}
{link.state !== 'merged' && link.ci === 'failed'  && <X … />}

closed is as terminal as merged: a closed pull request can never merge, so its CI rollup carries no actionable information. #301 ("hide CI status icons on merged PR chips") established precisely this reasoning — "once merged, CI state is moot — the merge icon is the terminal signal" — but implemented only merged. PullRequestPanel.tsx::SourceTabState already had the rule right for both states (lifecycle === 'merged' || lifecycle === 'closed' ? undefined : status?.ci), so the sidebar was the lone outlier.

Change: name the terminal vocabulary once and gate the three glyphs on it, so one rule covers both states instead of a literal repeated three times — the shape that let closed be handled by the badge but missed by the CI gate:

const TERMINAL_SOURCE_LINK_STATES: ReadonlySet<SourceLinkState> = new Set(['merged', 'closed'])

function showsChipCi(state: SourceLinkState | undefined): boolean {
  return state === undefined || !TERMINAL_SOURCE_LINK_STATES.has(state)
}

An absent state stays non-terminal — the provider status has not been read yet (or the payload predates the field), so such a chip keeps rendering CI exactly as before. A state outside the known set (GitLab locked, which the backend deliberately projects as no state) also still renders CI, identical to the old !== 'merged' behavior, so there is no regression for unexpected values.

SourceLinkState is derived from the wire type (NonNullable<NonNullable<Slot['source_links']>[number]['state']>) rather than re-typed, so the set is compiler-checked against the real vocabulary and cannot drift silently. Both call sites now cross-reference each other in comments, since the chip and the panel tab must not disagree about one pull request's lifecycle.

The backend is faithful and unchanged. _project_state correctly reports closed and _rollup_ci correctly reports the rollup it was handed; this was purely a render-condition bug.

Tests

website/src/test/ChatSidebar.sourceLinkChip.test.tsx — the harness is parameterized to take chip fixtures, and a new table locks in the whole rule (every fixture carries ci: 'running', so state is the only variable):

• hides the running-checks spinner on a closed chip — the reported bug.
• hides the running-checks spinner on a merged chip — fix(sidebar): hide CI status icons on merged PR chips #301's assertion lived in ChatSidebar.integration.test.tsx, which has since been split up, and did not survive; there was no test covering CI suppression in either terminal state, which is why the closed half went unnoticed. Restored here.
• still shows the spinner while the PR is live or its state is unknown — positive control over an open chip and a chip with no state at all, so the negative assertions cannot pass vacuously and the absent-state path is pinned as non-terminal.
• keeps the closed chip's own lifecycle label — the spinner goes away, the terminal signal must not.
• hides a passed / failed CI glyph on a closed chip too (it.each) — all three glyphs, not just the spinner.

Non-vacuity proven, not assumed: with 'closed' removed from the terminal set, exactly 3 of these tests fail (AssertionError: expected SVGSVGElement to be null) while the merged and live-PR controls keep passing. Restored, all 8 in the file pass.

docs/system-specs/modules/learn-cron-dashboard.md is updated in the same commit: the chip description now states the terminal-state suppression rule, why a closed rollup can stay pending forever, that both surfaces apply the same rule, and that an absent state is not terminal.

Manual verification

N/A — unit coverage is sufficient and stronger than a manual pass here. The change is a pure render-condition guard whose entire input space is the four-value state union plus undefined, and the test table enumerates every branch of it (closed, merged, open, absent) against all three CI glyphs. A manual repro would additionally require a real closed pull request whose check rollup is parked in PENDING, which is not reproducible on demand.

Gates run locally: pytest (22783 passed), isort / flake8 / mypy (clean, 567 files), npm run build (tsc -b + vite, clean), npm test (6547 passed / 541 files), jscpd (0 clones), eslint on the changed files (0 errors), i18n:check (at baseline). The three backend failures and one frontend failure seen locally were each confirmed to fail identically on pristine main with these changes absent — see the note below.

Pre-existing local failures (verified independent of this change)

Confirmed by re-running them on pristine origin/main with this branch's changes absent:

• test/test_app_backend.py::TestBootSpawnLatency::test_survival_check_exits_early_for_a_healthy_child — timing budget, load-sensitive.
• test/test_cli.py::TestInstallPidfdChildWatcher::test_real_subprocess_works_after_install_on_linux
• test/test_module_loader.py::TestModuleUnload::test_reload_after_unload_gets_fresh_code
• website/src/i18n/format.test.ts — asserts Intl.DurationFormat is absent; it exists on Node ≥ 23. Local Node is v24.14.0, CI pins Node 20, so this is local-only.

None touch the sidebar chip path.

Screenshots

Not included. The change removes an icon from an existing chip and adds no new UI surface, colour, layout, or component — the affected pixels are one <Loader2> glyph that should no longer be painted, which the tests assert on directly by aria-label. Reproducing the "before" state additionally needs a real closed pull request with a permanently-pending rollup. This follows the precedent of #301, the same change in the merged case, which shipped with no images for the same reason.

Notes for review (deliberately out of scope)

Three items were identified and consciously left alone rather than widening the diff:

1. The terminal vocabulary now appears in two files — ChatSidebar.tsx (over the sidebar's local Slot source-link type) and PullRequestPanel.tsx (over PullRequestStatus['state']). Unifying them would require a new shared module, since the two types are declared independently. Both sites now name the other in a comment so a future change to one is pointed at the other.
2. The closed label is not localized — <span className="capitalize text-danger">{link.state}</span> renders raw English while the adjacent merged marker uses i18nT. Pre-existing, on a line this PR does not change, and fix(i18n): localize agent-capabilities nav, composer placeholder, session status, and approval-mode picker #1016 is actively localizing this file.
3. npm run typecheck is a no-op — the root website/tsconfig.json is {"files": [], "references": [...]}, and tsc --noEmit does not follow project references, so it typechecks nothing (verified: a deliberate type error in ChatSidebar.tsx passes tsc --noEmit and is caught only by tsc -b). The real check runs via tsc -b inside npm run build, so coverage is not actually lost — but the typecheck and check scripts are misleading. Pre-existing and repo-wide; worth its own issue.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 46c9b9377abd843b67147ab8c5127e6fdec153b1 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Real terminal-state bug, fixed at the render-rule root cause with the vocabulary named once, aligned with the panel's existing rule, and non-vacuously tested.

[DESIGN-REVIEWED] 46c9b93

[github-actions]

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of 46c9b9377abd843b67147ab8c5127e6fdec153b1 — updated in place on each push; does not block merge.

UX-Verdict: PASS

Removes an infinite spinner from persistent sidebar chrome and aligns the chip with the panel's existing terminal-state rule — strictly less noise, no new surface.

[UX-REVIEWED] 46c9b93

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 46c9b9377abd843b67147ab8c5127e6fdec153b1 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 46c9b93

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 46c9b9377abd843b67147ab8c5127e6fdec153b1: <one-sentence reason>

[github-actions]

Opus 5 Review — ✅ no blocking findings

Reviewed 46c9b9377abd843b67147ab8c5127e6fdec153b1 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 46c9b93

Verdict parsed from the review's SHA-scoped output markers for commit 46c9b9377abd843b67147ab8c5127e6fdec153b1.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 46c9b9377abd843b67147ab8c5127e6fdec153b1: <one-sentence reason>

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 46c9b9377abd843b67147ab8c5127e6fdec153b1.

Second-order review for 46c9b9377abd843b67147ab8c5127e6fdec153b1; this comment is updated in place on each push.

Review details

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Both line-level reviewers (Opus 5 and GPT 5.6) reported no findings, and the design and UX reviewers each returned a clean PASS with no CONCERNS. There are therefore zero sub-threshold findings to evaluate for escalation, and nothing substantive was demoted to a follow-up. The diff itself is a narrow, well-tested frontend render-rule fix (suppressing the CI rollup on terminal PR chips) plus a spec update — no contract, schema, persisted-data, or wire-format decisions are locked in by merging it.

[ARBITER-REVIEWED] 46c9b93

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter 46c9b9377abd843b67147ab8c5127e6fdec153b1: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.