From c2f6d360748933d9dbd02dbf11019e95281ea761 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:01:23 -0700 Subject: [PATCH] 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. --- .../active-checks-status.test.ts | 31 ++++++++- .../right-sidebar/active-checks-status.ts | 67 +++++++++++-------- 2 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/renderer/src/components/right-sidebar/active-checks-status.test.ts b/src/renderer/src/components/right-sidebar/active-checks-status.test.ts index 02553b09ab2..5bf09276c72 100644 --- a/src/renderer/src/components/right-sidebar/active-checks-status.test.ts +++ b/src/renderer/src/components/right-sidebar/active-checks-status.test.ts @@ -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() + const keyed = new Set(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) + }) }) diff --git a/src/renderer/src/components/right-sidebar/active-checks-status.ts b/src/renderer/src/components/right-sidebar/active-checks-status.ts index 0ba8aa681ca..9f1e08a03ba 100644 --- a/src/renderer/src/components/right-sidebar/active-checks-status.ts +++ b/src/renderer/src/components/right-sidebar/active-checks-status.ts @@ -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> +// 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 & + Partial> +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 }