feat(checks-panel): auto-refresh on entering Checks tab (#1688)
Force a freshness check each time the user enters the Checks tab (open sidebar, switch to Checks tab, or switch active worktree/branch) so stale PR metadata, cached-null "no PR" results, stale checks, and stale comments are surfaced immediately rather than waiting for the cache TTL. - Extracts entry-refresh logic into `checks-entry-refresh.ts` with a 30 s grace window to suppress rapid show/hide duplicate fetches. - Adds a `shouldEntryRefresh` effect in `ChecksPanel` keyed by `activeWorktreeId::repo.path::branch`; resets on panel hide so close-and-reopen re-evaluates freshness. - Fixes a stale-closure bug in `handleRefresh`: `fetchPRChecks` is now called directly with the freshly resolved `headSha` after PR refresh instead of reusing the pre-refresh closure's captured sha. - Adds 11 unit tests in `checks-entry-refresh.test.ts`. - Design doc: `docs/refresh-on-checks-tab.md`. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinjing
GitHub
parent
2247435c96
commit
648f207281
|
|
@ -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`.
|
||||
|
|
@ -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<string>('')
|
||||
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) {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
|
|
@ -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
|
||||
}
|
||||
Loading…
Reference in New Issue