refactor(right-sidebar): derive the active checks cache key and parameter type from one key list

The cache was keyed on a hand-written field list that had to be kept in sync
with every store field computeActiveChecksStatus reads; missing one would leave
the sidebar checks badge silently stale.

Derive both the parameter type and the cache key from ACTIVE_CHECKS_STATUS_INPUT_KEYS
so TypeScript rejects reading a field the cache is not keyed on, and add a Proxy
test that records every property actually read and fails on any unkeyed one.
This commit is contained in:
Neil
2026-09-04 14:01:23 -07:00
parent a928bd61ce
commit c2f6d36074
2 changed files with 68 additions and 30 deletions
@@ -1,5 +1,9 @@
import { beforeEach, describe, expect, it } from 'vitest'
import { clearActiveChecksStatusCacheForTests, getActiveChecksStatus } from './active-checks-status'
import {
ACTIVE_CHECKS_STATUS_INPUT_KEYS,
clearActiveChecksStatusCacheForTests,
getActiveChecksStatus
} from './active-checks-status'
import type { AppState } from '../../store/types'
import type { PRInfo } from '../../../../shared/github/pull-request-types'
@@ -167,4 +171,29 @@ describe('getActiveChecksStatus caching', () => {
)
).toBe('failure')
})
it('reads no store field the cache is not keyed on', () => {
// Guards against a cast or widened type bypassing the derived key list: every property the
// computation touches on the state object must invalidate the cache.
const reads = new Set<PropertyKey>()
const keyed = new Set<PropertyKey>(ACTIVE_CHECKS_STATUS_INPUT_KEYS)
const state = new Proxy(
makeState({ 'repo-1::feature/test': { data: makePR('success'), fetchedAt: 2 } }),
{
get(target, prop, receiver) {
reads.add(prop)
return Reflect.get(target, prop, receiver)
},
has(target, prop) {
reads.add(prop)
return Reflect.has(target, prop)
}
}
)
expect(getActiveChecksStatus(state)).toBe('success')
expect([...reads].filter((prop) => !keyed.has(prop))).toEqual([])
// The read set must also be non-trivial, or the guard proves nothing.
expect(reads.has('prCache')).toBe(true)
})
})
@@ -5,11 +5,29 @@ import { getGitHubPRCacheKey } from '../../store/slices/github-cache-key'
import { getHostedReviewCacheKey } from '../../store/slices/hosted-review-cache-identity'
import { isGitHubPRSuppressed } from '../../../../shared/worktree/github-pr-suppression'
type ActiveChecksStatusState = Pick<
AppState,
'activeWorktreeId' | 'worktreesByRepo' | 'repos' | 'prCache'
> &
Partial<Pick<AppState, 'settings' | 'hostedReviewCache'>>
// Why one list: the parameter type and the cache key both derive from it, so a new store field
// cannot be read here (TS rejects it) without also invalidating the cache on it.
const REQUIRED_INPUT_KEYS = [
'activeWorktreeId',
'worktreesByRepo',
'repos',
'prCache'
] as const satisfies readonly (keyof AppState)[]
const OPTIONAL_INPUT_KEYS = [
'settings',
'hostedReviewCache'
] as const satisfies readonly (keyof AppState)[]
/** @internal Every store field getActiveChecksStatus may read; the cache invalidates on any of them. */
export const ACTIVE_CHECKS_STATUS_INPUT_KEYS = [
...REQUIRED_INPUT_KEYS,
...OPTIONAL_INPUT_KEYS
] as const
type ActiveChecksStatusState = Pick<AppState, (typeof REQUIRED_INPUT_KEYS)[number]> &
Partial<Pick<AppState, (typeof OPTIONAL_INPUT_KEYS)[number]>>
type ActiveChecksStatusInputs = {
[K in (typeof ACTIVE_CHECKS_STATUS_INPUT_KEYS)[number]]: ActiveChecksStatusState[K]
}
function branchDisplayName(branch: string): string {
return branch.replace(/^refs\/heads\//, '')
@@ -18,12 +36,7 @@ function branchDisplayName(branch: string): string {
// Why cached: the right sidebar is always mounted, so this ran on every store write and rebuilt two
// cache-key strings each time. Same single-entry, reference-keyed shape as selectFloatingVisibleTabCount.
let activeChecksStatusCache: {
activeWorktreeId: ActiveChecksStatusState['activeWorktreeId']
worktreesByRepo: ActiveChecksStatusState['worktreesByRepo']
repos: ActiveChecksStatusState['repos']
prCache: ActiveChecksStatusState['prCache']
settings: ActiveChecksStatusState['settings']
hostedReviewCache: ActiveChecksStatusState['hostedReviewCache']
inputs: ActiveChecksStatusInputs
status: CheckStatus | null
} | null = null
@@ -32,29 +45,25 @@ export function clearActiveChecksStatusCacheForTests(): void {
activeChecksStatusCache = null
}
function hasSameInputs(inputs: ActiveChecksStatusInputs, state: ActiveChecksStatusState): boolean {
for (const key of ACTIVE_CHECKS_STATUS_INPUT_KEYS) {
if (inputs[key] !== state[key]) {
return false
}
}
return true
}
export function getActiveChecksStatus(state: ActiveChecksStatusState): CheckStatus | null {
const cached = activeChecksStatusCache
if (
cached &&
cached.activeWorktreeId === state.activeWorktreeId &&
cached.worktreesByRepo === state.worktreesByRepo &&
cached.repos === state.repos &&
cached.prCache === state.prCache &&
cached.settings === state.settings &&
cached.hostedReviewCache === state.hostedReviewCache
) {
if (cached && hasSameInputs(cached.inputs, state)) {
return cached.status
}
const status = computeActiveChecksStatus(state)
activeChecksStatusCache = {
activeWorktreeId: state.activeWorktreeId,
worktreesByRepo: state.worktreesByRepo,
repos: state.repos,
prCache: state.prCache,
settings: state.settings,
hostedReviewCache: state.hostedReviewCache,
status
}
const inputs = Object.fromEntries(
ACTIVE_CHECKS_STATUS_INPUT_KEYS.map((key) => [key, state[key]])
) as ActiveChecksStatusInputs
activeChecksStatusCache = { inputs, status }
return status
}