diff --git a/docs/refresh-on-checks-tab.md b/docs/refresh-on-checks-tab.md new file mode 100644 index 000000000..392b5de06 --- /dev/null +++ b/docs/refresh-on-checks-tab.md @@ -0,0 +1,165 @@ +# Auto-refresh on entering the Checks tab + +## Problem + +The Checks panel fetches on visibility, but those fetches are cache-respecting: + +- `CACHE_TTL = 300_000` for PRs and comments. +- `CHECKS_CACHE_TTL = 60_000` for checks. +- `ChecksPanel.tsx` fetches PRs on repo/branch changes without `force`. +- Checks polling and the comments effect also call the store without `force`. + +That means entering the Checks tab can render stale PR metadata, a cached `null` +"no PR" result, stale comments, or stale checks. The checks path is partially +protected by a shorter renderer TTL, but the main process also uses `gh api +--cache 60s` for the REST checks endpoint unless `noCache` is set. Manual refresh +already bypasses these renderer and `gh api` caches for the requests it starts. + +## Goal + +When the user enters the Checks tab, run one freshness check for the active +worktree and force-refresh only when the relevant cache timestamps are older +than a small grace window. "Entering" means: + +- opening the right sidebar while the Checks tab is selected; +- switching from another right-sidebar tab to Checks; +- switching active worktree/repo/branch while Checks is already visible. + +The refresh must cover PR discovery too, including cached `null`, so a PR opened +outside Orca can appear without waiting up to 5 minutes. + +## Non-goals + +- Changing the existing polling cadence while the panel stays visible. +- Refreshing on window focus, network reconnect, or arbitrary background events. +- Changing the manual refresh button. +- Prefetching checks for non-active worktrees. +- Adding cross-renderer or cross-window request coordination. Orca currently has + one live main window; renderer in-flight maps are not a multi-window primitive. + +## Design + +1. **Entry trigger.** Add an effect in `ChecksPanel.tsx` keyed by: + + ```ts + const entryKey = isPanelVisible && repo && !isFolder && branch + ? `${activeWorktreeId ?? ''}::${repo.path}::${branch}` + : '' + ``` + + Track the last processed visible `entryKey` in a ref. When `entryKey` is empty, + reset the ref to `''`. When it becomes non-empty and differs from the ref, this + is an entry. This is required; a `prevKey !== currentKey` check that does not + reset on hide would miss closing and reopening the same PR. + +2. **Grace window.** Define `ENTRY_REFRESH_GRACE_MS = 15_000` in + `ChecksPanel.tsx`. Select only timestamps, not whole cache records: + + ```ts + const prFetchedAt = useAppStore( + (s) => (prCacheKey ? s.prCache[prCacheKey]?.fetchedAt : undefined) + ) + const checksFetchedAt = useAppStore( + (s) => prNumber + ? s.checksCache[`${repo?.path ?? ''}::pr-checks::${prNumber}`]?.fetchedAt + : undefined + ) + const commentsFetchedAt = useAppStore( + (s) => prNumber + ? s.commentsCache[`${repo?.path ?? ''}::pr-comments::${prNumber}`]?.fetchedAt + : undefined + ) + ``` + + Missing PR cache is stale. A cached PR value of `null` is still a PR cache + entry and should be refreshed on entry once outside the grace window. When a + PR number is known, missing checks/comments timestamps are stale. When no PR is + known, checks/comments are not relevant yet. + + Run `handleRefresh()` if the oldest relevant timestamp is missing or older + than `Date.now() - ENTRY_REFRESH_GRACE_MS`; otherwise skip. + +3. **Reuse `handleRefresh`, but pass the refreshed head SHA through.** The + manual refresh flow is the right shape: force `fetchPRForBranch`, then if a + PR is returned, force checks and comments for the returned PR. Extend + `fetchChecks()` to accept a `headShaOverride` (or call `fetchPRChecks` + directly from `handleRefresh`) so the checks request uses + `refreshedPR.headSha`, not the stale `pr?.headSha` captured before the PR + refresh completed. This handles PR number changes, cached `null`, and + external force-pushes correctly. + +4. **Reset polling attention state before refresh.** When the entry refresh runs, + set `pollIntervalRef.current = 30_000` and `prevChecksRef.current = ''` before + calling `handleRefresh()`. `fetchChecks()` will then write the new signature + from the forced result. + +5. **Do not overstate in-flight behavior.** Current store behavior is: + + - `fetchPRForBranch({ force: true })` bypasses a non-forced in-flight PR + request and uses a generation guard so the older result cannot overwrite the + newer cache entry. + - `fetchPRChecks({ force: true })` and `fetchPRComments({ force: true })` do + **not** bypass any in-flight request for the same key. If a non-forced poll + is already in flight, entry refresh will join it and may not pass + `noCache: true` to the main process. + + Accept that tradeoff for this feature. It avoids duplicate `gh` calls during a + visible polling race. If strict "entry always bypasses gh cache" semantics are + required later, change checks/comments in-flight maps to track `{ promise, + force, generation }` like PRs. + +## API cost and feasibility + +This is not free. A cold entry refresh can start: + +- one PR lookup (`gh:prForBranch`); +- one checks request (`gh:prChecks`); +- one comments request (`gh:prComments`). + +`gh:prChecks` usually calls `gh api repos/{owner}/{repo}/commits/{sha}/check-runs` +and uses `--cache 60s` unless forced; if that fails it falls back to `gh pr +checks`, which does not use the `--cache` flag. + +`gh:prComments` is heavier than "one GitHub API call": it runs issue comments +REST, review threads GraphQL, and reviews REST in parallel. `noCache` only +removes `--cache 60s` from the REST `gh api` calls; the GraphQL call is always +made. + +The 15 s grace window is therefore required, not cosmetic. + +## Edge cases + +- **Cached no-PR result.** Do not skip just because `prNumber` is null. Refresh + the PR cache entry on tab entry when its timestamp is outside the grace window. +- **First Checks entry after app start.** Only `prCache` and `issueCache` are + persisted. `checksCache` and `commentsCache` start empty, so a known PR should + force checks/comments on entry. +- **Worktree switch while Checks is visible.** Include `activeWorktreeId` in the + entry key so same repo/branch switches still count as a new entry. The existing + render-time local reset handles stale title/loading state. +- **Rapid tab toggles.** Hiding the panel resets the processed entry key; showing + it again re-evaluates timestamps. The grace window suppresses duplicate calls. +- **Concurrent polling tick.** Checks/comments may reuse the in-flight poll + request instead of forcing a new `noCache` request. This is intentional for now. +- **Conflicting PR refresh.** The existing `mergeable === 'CONFLICTING'` effect + can still force a PR refresh. Entry refresh may also run, but PR generation + guards prevent older PR responses from overwriting newer cache entries. +- **PR head changed externally.** `handleRefresh()` must use the refreshed PR + returned by `fetchPRForBranch` before fetching checks, so checks are requested + with the current `headSha`. +- **Component unmount mid-fetch.** The comments auto-fetch guards stale + responses; the checks fetch path and `handleRefresh()` do not cancel local + setters. This feature should keep that behavior unless tests expose a warning + or stale-state regression. +- **SSH / remote repo.** No path manipulation or platform-specific shortcut code + is needed. The same IPC-backed `gh` path is used. + +## Rollout + +1. Add timestamp selectors, `ENTRY_REFRESH_GRACE_MS`, and the entry effect in + `ChecksPanel.tsx`. +2. Keep `handleRefresh()` as the single refresh implementation. +3. Add `ChecksPanel.test.tsx` if absent. Cover stale PR/null cache refreshes, + fresh-within-grace skips, known PR with missing checks/comments refreshes, + hidden panel no-op, and hide/show same PR re-evaluation. +4. Run `pnpm typecheck && pnpm lint`. diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 47a860669..1e644af9a 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -15,6 +15,7 @@ import { ChecksList, PRCommentsList } from './checks-helpers' +import { ENTRY_REFRESH_GRACE_MS, shouldEntryRefresh } from './checks-entry-refresh' import type { PRInfo, PRCheckDetail, PRComment } from '../../../../shared/types' export default function ChecksPanel(): React.JSX.Element { @@ -79,6 +80,21 @@ export default function ChecksPanel(): React.JSX.Element { ? (gitConflictOperationByWorktree[activeWorktreeId] ?? 'unknown') : 'unknown' + // Why: select only timestamps (not whole cache records) so the entry-refresh + // effect doesn't re-run on every cache mutation. See + // docs/refresh-on-checks-tab.md. + const prFetchedAt = useAppStore((s) => + prCacheKey ? s.prCache[prCacheKey]?.fetchedAt : undefined + ) + const checksCacheKey = repo && prNumber ? `${repo.path}::pr-checks::${prNumber}` : '' + const commentsCacheKey = repo && prNumber ? `${repo.path}::pr-comments::${prNumber}` : '' + const checksFetchedAt = useAppStore((s) => + checksCacheKey ? s.checksCache[checksCacheKey]?.fetchedAt : undefined + ) + const commentsFetchedAt = useAppStore((s) => + commentsCacheKey ? s.commentsCache[commentsCacheKey]?.fetchedAt : undefined + ) + // Fetch PR data when the active worktree/branch changes. // Why: pass linkedPR so worktrees created from a PR (whose new local branch // differs from the PR's head ref) resolve via the number-based fallback. @@ -240,9 +256,41 @@ export default function ChecksPanel(): React.JSX.Element { linkedPRNumber: linkedPR }) if (refreshedPR) { + // Why: call fetchPRChecks directly with the refreshed PR's headSha so + // we don't pass the stale headSha captured by `fetchChecks`'s closure + // before the PR refresh completed (covers external force-pushes and + // PR-number changes). + const refreshedChecks = fetchPRChecks( + repo.path, + refreshedPR.number, + branch, + refreshedPR.headSha, + { force: true } + ).then( + (result) => { + setChecks(result) + const signature = JSON.stringify( + result.map((c) => `${c.name}:${c.status}:${c.conclusion}`) + ) + pollIntervalRef.current = + signature === prevChecksRef.current + ? Math.min(pollIntervalRef.current * 2, 120_000) + : 30_000 + prevChecksRef.current = signature + }, + (err) => { + console.warn('Failed to fetch PR checks:', err) + setChecks([]) + } + ) + setChecksLoading(true) + const refreshedComments = fetchComments({ + force: true, + prNumberOverride: refreshedPR.number + }) await Promise.all([ - fetchChecks({ force: true, prNumberOverride: refreshedPR.number }), - fetchComments({ force: true, prNumberOverride: refreshedPR.number }) + refreshedChecks.finally(() => setChecksLoading(false)), + refreshedComments ]) } else { setChecks([]) @@ -251,7 +299,49 @@ export default function ChecksPanel(): React.JSX.Element { } finally { setIsRefreshing(false) } - }, [repo, branch, linkedPR, fetchPRForBranch, fetchChecks, fetchComments]) + }, [repo, branch, linkedPR, fetchPRForBranch, fetchPRChecks, fetchComments]) + + // Why: force a freshness check on each "entry" into the Checks tab so PRs + // opened outside Orca, externally force-pushed heads, and stale checks/comments + // appear without waiting for the cache TTL. The grace window suppresses + // duplicate fetches from rapid show/hide toggles. See + // docs/refresh-on-checks-tab.md. + const entryKey = + isPanelVisible && repo && !isFolder && branch + ? `${activeWorktreeId ?? ''}::${repo.path}::${branch}` + : '' + const lastEntryKeyRef = useRef('') + useEffect(() => { + if (!entryKey) { + // Resetting on hide is required so reopening the panel on the same PR + // re-evaluates freshness (a prevKey !== currentKey check alone would miss + // close-and-reopen of the same PR). + lastEntryKeyRef.current = '' + return + } + if (lastEntryKeyRef.current === entryKey) { + return + } + lastEntryKeyRef.current = entryKey + + const stale = shouldEntryRefresh({ + prFetchedAt, + checksFetchedAt, + commentsFetchedAt, + prNumber, + now: Date.now(), + graceMs: ENTRY_REFRESH_GRACE_MS + }) + if (!stale) { + return + } + + // Reset polling attention state so the forced fetch's signature establishes + // a fresh baseline rather than colliding with the previous PR's backoff. + pollIntervalRef.current = 30_000 + prevChecksRef.current = '' + void handleRefresh() + }, [entryKey, prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, handleRefresh]) const handleStartEdit = useCallback(() => { if (!pr) { diff --git a/src/renderer/src/components/right-sidebar/checks-entry-refresh.test.ts b/src/renderer/src/components/right-sidebar/checks-entry-refresh.test.ts new file mode 100644 index 000000000..2b5cec8b9 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/checks-entry-refresh.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it } from 'vitest' +import { ENTRY_REFRESH_GRACE_MS, shouldEntryRefresh } from './checks-entry-refresh' + +const NOW = 1_700_000_000_000 + +describe('shouldEntryRefresh', () => { + it('refreshes when PR cache is missing (cold start, no PR known)', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: undefined, + checksFetchedAt: undefined, + commentsFetchedAt: undefined, + prNumber: null, + now: NOW + }) + ).toBe(true) + }) + + it('refreshes when cached null PR result is older than the grace window', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - ENTRY_REFRESH_GRACE_MS - 1, + checksFetchedAt: undefined, + commentsFetchedAt: undefined, + prNumber: null, + now: NOW + }) + ).toBe(true) + }) + + it('skips when cached null PR result is within the grace window', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 1_000, + checksFetchedAt: undefined, + commentsFetchedAt: undefined, + prNumber: null, + now: NOW + }) + ).toBe(false) + }) + + it('refreshes when PR is known but checks cache is missing (first entry after app start)', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 1_000, + checksFetchedAt: undefined, + commentsFetchedAt: NOW - 1_000, + prNumber: 42, + now: NOW + }) + ).toBe(true) + }) + + it('refreshes when PR is known but comments cache is missing', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 1_000, + checksFetchedAt: NOW - 1_000, + commentsFetchedAt: undefined, + prNumber: 42, + now: NOW + }) + ).toBe(true) + }) + + it('refreshes when checks timestamp is older than the grace window', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 1_000, + checksFetchedAt: NOW - ENTRY_REFRESH_GRACE_MS - 1, + commentsFetchedAt: NOW - 1_000, + prNumber: 42, + now: NOW + }) + ).toBe(true) + }) + + it('refreshes when comments timestamp is older than the grace window', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 1_000, + checksFetchedAt: NOW - 1_000, + commentsFetchedAt: NOW - ENTRY_REFRESH_GRACE_MS - 1, + prNumber: 42, + now: NOW + }) + ).toBe(true) + }) + + it('skips when PR, checks, and comments are all fresh within the grace window', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 5_000, + checksFetchedAt: NOW - 5_000, + commentsFetchedAt: NOW - 5_000, + prNumber: 42, + now: NOW + }) + ).toBe(false) + }) + + it('ignores checks/comments freshness when no PR is known', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 1_000, + checksFetchedAt: undefined, + commentsFetchedAt: undefined, + prNumber: null, + now: NOW + }) + ).toBe(false) + }) + + it('treats a PR timestamp exactly at the cutoff as fresh', () => { + // Why: the rule is "older than now - grace", strict less-than. Equal is fresh. + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - ENTRY_REFRESH_GRACE_MS, + checksFetchedAt: NOW - ENTRY_REFRESH_GRACE_MS, + commentsFetchedAt: NOW - ENTRY_REFRESH_GRACE_MS, + prNumber: 42, + now: NOW + }) + ).toBe(false) + }) + + it('honors a custom graceMs override', () => { + expect( + shouldEntryRefresh({ + prFetchedAt: NOW - 2_000, + checksFetchedAt: NOW - 2_000, + commentsFetchedAt: NOW - 2_000, + prNumber: 42, + now: NOW, + graceMs: 1_000 + }) + ).toBe(true) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/checks-entry-refresh.ts b/src/renderer/src/components/right-sidebar/checks-entry-refresh.ts new file mode 100644 index 000000000..9abb5e869 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/checks-entry-refresh.ts @@ -0,0 +1,46 @@ +// Why: extracted as a pure helper so the freshness decision is unit-testable +// without mounting ChecksPanel and its many store dependencies. The rules come +// from docs/refresh-on-checks-tab.md (Grace window + Edge cases sections). + +export const ENTRY_REFRESH_GRACE_MS = 15_000 + +export type EntryRefreshInput = { + prFetchedAt: number | undefined + checksFetchedAt: number | undefined + commentsFetchedAt: number | undefined + prNumber: number | null + now: number + graceMs?: number +} + +/** + * Decide whether entering the Checks tab should trigger a force refresh. + * + * Rules: + * - Missing PR cache timestamp is stale (cold start, never fetched). + * - A cached PR timestamp older than `now - graceMs` is stale; this also + * covers cached `null` PR results, which still have a fetchedAt. + * - When a PR number is known, missing checks/comments timestamps are stale + * (their caches are not persisted, so they restart empty). + * - When no PR number is known, checks/comments timestamps are not relevant. + */ +export function shouldEntryRefresh(input: EntryRefreshInput): boolean { + const { prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, now } = input + const graceMs = input.graceMs ?? ENTRY_REFRESH_GRACE_MS + const cutoff = now - graceMs + + if (prFetchedAt === undefined || prFetchedAt < cutoff) { + return true + } + + if (prNumber !== null) { + if (checksFetchedAt === undefined || checksFetchedAt < cutoff) { + return true + } + if (commentsFetchedAt === undefined || commentsFetchedAt < cutoff) { + return true + } + } + + return false +}