fix(dashboard): stop the PR panel re-reading unchanged pull requests

[bolichen97]

Problem / Motivation

Opening the Changes side panel (GitHub PR sidebar) re-ran the full provider fanout every time — the core gh pr view, the files, review-comment and check-rollup reads, and the merge-state re-reads (~5 gh subprocesses) — even when nothing about the pull request had changed. Merged and closed pull requests kept being re-read on the open-PR cadence for as long as their chip stayed in a sidebar.

Why it matters

Every panel open shows a reload and spends five subprocesses plus five GitHub API calls against the user's 5,000/h primary rate limit; with a dozen finished PRs across the session list the chip loop alone was one gh subprocess per finished PR per minute, forever. The same shape (a panel poll loop that never slows on finished items) exhausted a provider-side throttle for every user of our internal edition, so this closes the class on the public side too.

What changed (motivation → approach → change)

Two independent causes, one on each side of the wire, plus a conditional-request layer so an expired open PR is revalidated instead of re-read.

Client (root cause 1). useWebSocket.ts invalidated the WHOLE ['pull-request-source'] query family — no URL key — on every turn boundary of ANY session (active slot: refetch now; background: mark stale). While any chat was running, every session's detail payload was stale on open. Now the invalidation is scoped to the finished slot: the active slot refetches the MOUNTED detail query with refetchQueries({queryKey, type: 'active'}) (the PR on screen — the slots payload names only the first few chips, so the PR being viewed can lie outside that set; refetchQueries marks nothing else stale, whereas invalidateQueries with refetchType: 'active' still marks every cached PR stale) and marks its own source_links change URLs stale for their next mount; a background slot only marks its own URLs; the panel's mutation success handlers invalidate only the PR they changed. slotChangeUrls (utils/pullRequestLinks.ts) is the pure helper. The PR and issue detail queries also retain an unmounted payload for one hour (SOURCE_DETAIL_GC_MS) instead of React Query's five-minute default, and the PR detail query revalidates on mount whenever the retained data is older than the gateway's cache window (SOURCE_REMOUNT_REVALIDATE_MS, 30s), so a reopened panel renders the retained payload at once and refetches in the background — stale-while-revalidate — rather than presenting an hour-old discussion as current (events from this gateway cannot see a teammate's comments). Younger data is not refetched: the gateway would return the same bytes, and Code Review Sage mounts its pane and this panel on one key, which stays one provider read per open. Past the window the refetch is cheap because the gateway revalidates with conditional GETs. The Issues panel gets the same predicate (it has no turn-boundary or status-delta invalidation at all). A background revalidation that fails over a loaded payload renders a compact one-line notice — "Couldn't refresh — showing the last loaded version", with the login command and a retry — instead of stacking the full-height "could not load" card over content that is visibly on screen; the string is added to all twelve catalogs and the pseudolocale regenerated.

Gateway (root cause 2). source_providers.py aged every full payload by one 30s TTL and re-read every chip every 60s regardless of lifecycle. The chip cache — and the full payloads with no cheaper read (GitLab, registered plugins) — now age a merged entry by _TERMINAL_TTL_SECS (six hours) and a closed one by _CLOSED_TTL_SECS (one hour: it can be reopened and keeps accruing discussion). The payload TTL is decided from the payload itself through the same _project_state the chip projection uses (_full_payload_ttl), so the two caches cannot disagree about whether a URL is finished. The explicit refresh button and mutation invalidation still bypass it, and the turn-boundary force (request_check_refresh_now) still re-reads a closed chip (an agent can reopen one) but never a merged one (_chip_refresh_due).

Conditional revalidation. An expired github.com payload — whatever its lifecycle — is first revalidated with small conditional REST GETs (gh api -i -H If-None-Match) before the fanout:

• issues/{n} — its ETag follows the pull request's updated_at (title/body/labels/lifecycle including a reopen, a push, reviews, comments), so post-merge comments and a reopen reach the panel within one TTL for one rate-limit-free request. pulls/{n} is deliberately NOT the probe: it embeds the base/head repository objects whose live counters (open issues, stars, pushed_at) change its ETag on a busy repo without the PR changing.
• For an OPEN pull request also commits/{head_sha}/check-runs and commits/{head_sha}/status — CI hangs off the commit and never moves updated_at; check runs and legacy commit statuses are separate resources and the rollup renders both. Skipped for merged/closed, whose CI the panel and chip no longer track.

All-304 re-stamps the cached entry; anything else runs the fanout. Strictly 304-only: the first probe of a URL has no validator, answers 200 and only LEARNS the ETags (_REVALIDATORS, bounded, the two commit-level validators scoped to the head sha so a push cannot reuse the old commit's); a failed probe is "unknown"; bodies are never compared. Validators from an all-304 are committed at once; validators from a 200 are committed only after the full read that follows has succeeded, so a fanout that fails (rate limit, 503) cannot pair the old payload with new ETags and have every later probe re-stamp it as current. _ConditionalRead carries status and ETag only — bodies are never decoded, because nothing is ever judged by comparing them. Re-stamping is capped: a validator set remembers when its payload was last read in full, and past 6 h (_REVALIDATED_MAX_AGE_SECS) one full read runs without probing and the set is dropped, so an ETag-coverage gap on GitHub's side degrades to bounded staleness rather than unbounded. GitHub's GraphQL API — what gh pr view speaks — has no conditional requests, an authenticated 304 costs nothing on the primary rate limit, and gh exits 1 on a 304 (gh: HTTP 304), so _parse_conditional_get reads the status line rather than the exit code. The merge pair moves neither validator and stays with the existing chip↔full protocol. Refresh, mutation invalidation, GitLab and registered plugins never probe. _run_json is now a thin wrapper over _run_provider(parse=...) so the conditional reader shares the same isolation, bounds and SEL audit (every probe is an audited gh invocation).

Design references (how mature tools do it): GitLens and GitHub Desktop serve the cache on panel open and revalidate in the background with 30-min list TTLs and a 60s checks floor; the VS Code GitHub PR extension backs off 5→30 min on an unchanged open PR; GitHub Desktop and the JetBrains GitLab plugin send real If-None-Match; GitHub's REST guidance: "a 304 does not count against your primary rate limit".

Tests

Backend (test/test_source_providers.py, +34):

• _full_payload_ttl by projected lifecycle (open/draft/opened → short; merged → six hours; closed incl. closed-while-draft → one hour; locked/unknown → short) and the ordering property the retention tests rest on.
• fetch_pull_request (GitLab, no conditional read): a merged payload past the open TTL is a hit with no provider read and refresh=True still bypasses; closed ages on the one-hour clock; a terminal payload past its own TTL re-reads; the on-write sweep ages each entry by its own TTL. GitHub: an expired merged payload is revalidated with the issue probe alone (304 → served and re-stamped; 200 → full read).
• _chip_refresh_due parametrized over open/draft/closed/merged/unknown × age (open / closed / merged clocks) × force (merged not force-read; closed force-read); schedule_check_refresh + request_check_refresh_now skip finished PRs.
• _parse_conditional_get: 200 with CRLF headers, 304 arriving as exit 1, LF-only headers, missing ETag, rejection of 404/401/no status line/bad JSON, login hint on auth failure.
• _gh_conditional_get argv (header only when a validator is known); _github_payload_unchanged: first probe learns and is unknown, all-304 required (issue + check-runs + commit status for open; issue only for merged/closed), CI-moved, status-moved and review-landed cases, commit-level validators not reused across a push, probe failure/missing head are unknown; _REVALIDATORS bounded; _revalidation_applies (github.com only).
• End to end: an expired open payload is served and re-stamped when both probes answer 304 (fanout not awaited; the next read is a plain hit with no probe), falls through on a change, refresh and cold reads never probe, and the generation guard never re-stamps an entry a mutation dropped.

Frontend:

• PullRequestPanel.test.tsx: a retained payload older than the window paints immediately and triggers one background refetch; one inside the window paints without any fetch; a failing background revalidation shows the compact status notice and never the alert card.
• useWebSocketSourceStatus.test.ts: active-slot turn calls refetchQueries(type: 'active') and marks its own URL stale, never invalidateQueries on the bare family; a background slot's turn marks ONLY its own PR (one pull-request-source call, no refetch); a slot with no PR links touches no detail query.
• pullRequestLinks.test.ts: slotChangeUrls skips issue links and duplicates, yields nothing for an unknown slot or missing links.

Local gates: flake8, isort, mypy --platform linux, pytest on the touched suites (885 passed; the two test_provider_executable_* failures reproduce on untouched origin/main — host uid ownership), tsc -b, eslint, vitest (163 passed), docs-lint.sh, black/brand/harness/subprocess-encoding/sync-io gates.

Manual verification

Parser checked against real gh (2.x) output on repos/kirodotdev/KiroCrew/issues/8208: -i prints the status line with a bare \n and headers with \r\n; 200 → W/"…" ETag + JSON body parsed; replaying with If-None-Match → exit 1, gh: HTTP 304 on stderr, HTTP/2.0 304 Not Modified on stdout, strong-form ETag echoed, parsed as _ConditionalRead(304, '"…"', None).

Screenshots / video

Why no screenshot: the frontend change alters only WHEN the panel refetches (scoped query invalidation and a longer cache retention); no component renders differently, and the spinner it removes was the default loading state, not a new surface.

Related Issues

N/A — reported internally (same shape as the CRUX panel throttle incident on the internal edition).

Pattern harvest

Rule candidate: review-prompt
Pattern: an event handler that invalidates a whole query-key family (invalidateQueries({queryKey: [family]})) instead of the entity the event names, and a cache whose TTL ignores that the entity has reached a terminal state.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 7a3deae065a01571ca29c7ebfbc03d9f983245a3 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Reading complete — I've reviewed the full patch (backend TTL/revalidation layer, frontend scoped invalidation and stale-while-revalidate, tests, docs) against the base module.

Design-Verdict: PASS

Root-cause fix on both sides of the wire, with fail-safe degradation (a failed probe just runs the old fanout) and every freshness assumption bounded and tested.

Suggestions

• source_providers.py now carries three coherence mechanisms (chip↔full projection + flap damp, per-URL generations, ETag revalidators) in one module; a follow-up extracting the cache/revalidation protocol into its own unit would keep the next change reviewable.
• The GitLab/plugin merged path serves up-to-6h-old discussion as current with no "as of" hint — the exact presentation the GitHub path was redesigned to avoid; a cheap follow-up is a staleness hint (or shorter merged TTL) for providers without conditional reads.

[DESIGN-REVIEWED] 7a3deae

[github-actions]

UX Review (Fable 5, fork) — ✅ PASS

UX-level review of 7a3deae065a01571ca29c7ebfbc03d9f983245a3 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

A failed refresh now degrades to a compact, actionable notice over retained content instead of replacing a visible PR with an error card — the copy asserts, explains, and offers Retry, in all 12 locales.

Suggestions

• The compact notice's Retry (could_not_refresh_showing_cached bar in PullRequestPanel.tsx/IssuePanel.tsx) shows nothing while its refetch runs — a quickly-failing retry re-renders the identical bar, so the click looks dead; reuse the header pattern (animate-spin on RefreshCw + disabled during query.isFetching).
• "Couldn't refresh — showing the last loaded version" hides how old that version is; retention runs to an hour (SOURCE_DETAIL_GC_MS), so append the age from query.state.dataUpdatedAt via the existing timeAgo (e.g. "…version from 40 minutes ago").

[UX-REVIEWED] 7a3deae

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 7a3deae065a01571ca29c7ebfbc03d9f983245a3 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've now read the full patch, the intent file, and the relevant base files. My verification: the base has exactly 2 unscoped ['pull-request-source'] invalidations (useWebSocket.ts:1772, PullRequestPanel.tsx:594) and the patch fixes both — no unfixed siblings of root cause 1. The one real finding is that Code Review Sage's usePrSource (PrSourcePanel.tsx:24-48) already encodes the same "gateway cache window is 30s" policy on the same query key as a plain staleTime: 30_000 (SOURCE_STALE_MS), while this PR adds a second spelling of it (SOURCE_REMOUNT_REVALIDATE_MS + a refetchOnMount predicate over staleTime: Infinity) in two panels. Everything else — lifecycle TTLs, the conditional-GET revalidation layer, the compact refresh-failure notice, the catalog strings, the same-commit spec update — is declared, derived from the measured subprocess/rate-limit cost or a documented invariant, and sits at cause level.

First-Principles-Verdict: CONCERNS

Every item is declared and cause-level; the one soft spot is a second spelling of the gateway's 30s window that Sage already encodes on the same query key.

What this change ships

Intent: stop the PR panel and chip loop spending provider subprocesses and rate limit on pull requests that have not changed — a FIX.

1. A turn finishing in one chat no longer marks every other session's PR stale — justified (root cause 1; both unscoped sites in base fixed, 0 siblings left)
2. Panel mutation buttons refresh only the PR they changed — justified
3. A reopened PR/issue panel paints the retained payload for up to an hour instead of a spinner — justified (changed default, declared)
4. A remount past 30s revalidates in the background; younger ones don't — duplicate of PrSourcePanel.tsx:48's spelling
5. Failed background refresh shows a one-line notice over content, not the full error card (12 locales) — justified
6. Merged PRs re-read every 6h, closed every 1h, instead of the open cadence — justified (root cause 2)
7. Turn boundary never force-reads a merged chip, still re-reads a closed one — justified
8. Expired github.com payloads answer 1–3 rate-limit-free conditional GETs before any fanout — justified (cause-level, bounded, fail-open to the full read)
9. Re-stamping capped at 6h without a full read — justified (bounded staleness)
10. _run_json reshaped over shared _run_provider(parse=) — rides along, but required by item 8 (probes need the same isolation/SEL path)

Watch

The "gateway cache window ≈ 30s" fact now has two client spellings on one query key (counted: SOURCE_STALE_MS at PrSourcePanel.tsx:24, SOURCE_REMOUNT_REVALIDATE_MS in PullRequestPanel.tsx; grep 30_000 + pull-request-source). They will drift independently of _CACHE_TTL_SECS and of each other. Also note the predicate keys on dataUpdatedAt, not staleness, so a background-slot invalidation younger than 30s at next mount is silently skipped — a finite staleTime would honor it.

Subtractions

• Drop SOURCE_REMOUNT_REVALIDATE_MS and the refetchOnMount predicate in PullRequestPanel.tsx and IssuePanel.tsx; set staleTime: 30_000 (the existing SOURCE_STALE_MS policy, hoisted to one shared constant). With focus/reconnect refetch already off, mount-time behavior is identical and invalidation-deferred refetches get honored natively.

[FIRST-PRINCIPLES-REVIEWED] 7a3deae

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 7a3deae065a01571ca29c7ebfbc03d9f983245a3 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- website/src/components/PullRequestPanel.tsx:888 -- refetchOnMount returns false for invalidated data younger than 30 seconds, so background-turn updates can remain stale while mounted -> Fix: return 'always' when query.state.isInvalidated.

FINDING -- src/kiro_crew/dashboard/handlers/source_providers.py:1795 -- _lifecycle_ttl(state) makes a rate-floored turn-boundary refresh leave a reopened PR’s chip closed for up to an hour -> Fix: retain _CHECK_TTL_SECS for closed chips in this non-forced path.

FINDING -- src/kiro_crew/dashboard/handlers/source_providers.py:4688 -- _full_payload_ttl(payload) evicts open GitHub payloads after 30 seconds when any other PR is written, bypassing the new conditional-revalidation path -> Fix: exempt revalidatable GitHub entries from the TTL sweep and leave eviction to the existing count/byte caps. (origin: validation)

[GPT-REVIEWED] 7a3deae

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 7a3deae065a01571ca29c7ebfbc03d9f983245a3 via the fork AI-review pipeline; updated in place on each push.

Review details

Based on my analysis, I need to verify the single candidate against the actual _probe_github_payload code.

The candidate claims asyncio.gather(*probes) (line 408, no return_exceptions=True) leaves sibling probe tasks orphaned on first exception, producing "Task exception was never retrieved" warnings and wasted subprocesses.

Falsifying:

• (a)/(b) hold: an OPEN-PR revalidation where one of the three probes raises SourceProviderError while a sibling is still in flight is reachable via fetch_pull_request → _revalidate_pull_request → _probe_github_payload.
• (c) fails. The candidate's central observable outcome — asyncio logging "Task exception was never retrieved" — is false. CPython's gather installs a _done_callback on every child that, once the outer future is already resolved by the first exception, calls fut.exception() on each later-failing sibling specifically to mark it retrieved. No warning is logged. What remains is a single in-flight gh subprocess running to completion unconsumed — bounded, no incorrect data (the code correctly returns _PROBE_UNKNOWN and falls through to a full read). That is trivial resource waste, not a crash, data loss, corruption, security hole, or removed guard.

The one grounded effect is a robustness nicety whose stated failure mode does not occur, and the candidate self-rated it low. It does not clear 80+, and it is not a BLOCKING class. Dropped.

Step 2: while tracing the revalidation path (_commit_revalidator, the 200-vs-304 validator-commit ordering, the read_at/_REVALIDATED_MAX_AGE_SECS ceiling, the generation/is cached re-stamp guard), I found the logic internally consistent — validators from a 200 are committed only after the successful full read, the unchanged branch re-stamps only under generation+identity match, and the ceiling drops validators to force a fresh learn. Nothing groundable at 80+ to add.

No findings.

[OPUS-REVIEWED] 7a3deae

[bolichen97]

Addressed in b459d4c (rebased on main, one commit):

• Design / validators committed before the fanout succeeds — real. Validators from a 200 are now returned to the caller and committed only after _fetch_pull_request_uncached lands; an all-304 still commits at once. Test: test_validators_from_a_200_are_committed_only_after_the_fanout_succeeds.
• First Principles / _ConditionalRead.body — dropped; the parser returns status and ETag only and no longer decodes a 200 body.
• UX / login command clipped in the compact notice — the <code> now sits outside the truncating span with a title.
• Previous round: the cron "Script-mode MCP identity" paragraph was a stale-base artefact and is restored byte-identical; the Issues panel has the same remount predicate and the same compact notice.

Kept as is, deliberately:

• SOURCE_REMOUNT_REVALIDATE_MS mirrors the gateway window. The detail endpoint returns the normalized payload, not a cache envelope, so there is no ttlSecs to pace off without changing the response shape. Drift is bounded to when a background refetch fires (never whether data is correct), and the spec names the pairing next to the constant.
• Finished GitLab MRs age on the lifecycle clock (1h closed / 6h merged). GitLab has no conditional read here; this PR's whole point is not re-reading finished changes on every open. The header Refresh bypasses the TTL, and the panel's own state chip already says the MR is merged/closed.
• Review-thread resolution vs issues/{n} updated_at — noted; the thread list is rendered from the same REST/GraphQL fanout, so if GitHub does not bump the issue for a resolve the exception is the chip protocol's TTL, same as before this PR for a closed thread that reopens.

[bolichen97]

7a3deae: took the Design suggestion — re-stamping now has an absolute ceiling. Each validator set carries read_at (last full read; an all-304 carries it forward, a full read resets it), and past _REVALIDATED_MAX_AGE_SECS (6 h, the merged TTL) _revalidate_pull_request reads in full without probing and drops the set so the next cycle learns afresh. Tests: test_revalidation_forces_a_full_read_past_the_max_age, test_all_304_carries_read_at_forward_and_a_full_read_resets_it. Probe-parse drift already fails safe to the fanout with an info log; left as is.

[chenmingwei23]

Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.