fix(workspace-cleanup): own facet caches with useMemo instead of render-time ref writes

React Doctor (CI changed-lines gate) correctly flagged the three cache refs
written during render. Each per-candidate cache now lives in one memo with
the derived context it is keyed on, so the memo deps are the invalidation
and interior fills stay content-addressed; the matched-set identity
stabilization is dropped since its only consumer reads through a
useEffectEvent and never keys on identity.
This commit is contained in:
Brennan Benson
2026-08-13 17:55:52 -07:00
parent d0fe1c73a0
commit b25831cb22
2 changed files with 90 additions and 104 deletions
@@ -1,4 +1,4 @@
import { useMemo, useRef } from 'react'
import { useMemo } from 'react'
import { useShallow } from 'zustand/react/shallow'
import { useAppStore } from '@/store'
import { getLiveAgentStatusByWorktreeId } from '@/lib/worktree-activity-state'
@@ -33,8 +33,8 @@ import {
import {
computeWorkspaceCleanupFacetList,
computeWorkspaceCleanupReviewInfoIndex,
type WorkspaceCleanupFacetListCache,
type WorkspaceCleanupReviewInfoCache
createWorkspaceCleanupFacetListCache,
createWorkspaceCleanupReviewInfoCache
} from './workspace-cleanup-facet-row-caches'
export type WorkspaceCleanupFacetRows = {
@@ -117,13 +117,25 @@ export function useWorkspaceCleanupFacetRows({
() => countWorkspaceCleanupCandidateIds(candidates),
[candidates]
)
const reviewLookup = useMemo(
() => buildWorkspaceCleanupReviewLookup({ repos, worktreesByRepo }),
[repos, worktreesByRepo]
// Why: each per-candidate cache lives in one memo together with the derived
// context it is keyed on — the memo deps ARE the cache invalidation, no refs
// (ref writes during render are unsafe under concurrent rendering), and
// interior fills are content-addressed and idempotent.
const reviewContext = useMemo(
() => ({
sources: { hostedReviewCache, repos, settings, worktreesByRepo },
lookup: buildWorkspaceCleanupReviewLookup({ repos, worktreesByRepo }),
cache: createWorkspaceCleanupReviewInfoCache()
}),
[hostedReviewCache, repos, settings, worktreesByRepo]
)
const worktreeById = useMemo(
() => buildWorkspaceCleanupWorktreeIndex(worktreesByRepo, repos),
[repos, worktreesByRepo]
const facetContext = useMemo(
() => ({
worktreeById: buildWorkspaceCleanupWorktreeIndex(worktreesByRepo, repos),
workspaceStatuses: sources.workspaceStatuses,
cache: createWorkspaceCleanupFacetListCache()
}),
[repos, sources.workspaceStatuses, worktreesByRepo]
)
const liveAgentStatusByWorktreeId = useMemo(
() => getLiveAgentStatusByWorktreeId(sources.agentStatusByPaneKey, sources.tabsByWorktree, now),
@@ -134,26 +146,17 @@ export function useWorkspaceCleanupFacetRows({
[sources.dismissals]
)
const reviewCacheRef = useRef<WorkspaceCleanupReviewInfoCache | null>(null)
const reviewInfoByWorktreeId = useMemo(() => {
const { cache, infos } = computeWorkspaceCleanupReviewInfoIndex({
candidates,
candidateIdCounts,
reviewSources: { hostedReviewCache, repos, settings, worktreesByRepo },
reviewLookup,
cache: reviewCacheRef.current
})
reviewCacheRef.current = cache
return infos
}, [
candidateIdCounts,
candidates,
hostedReviewCache,
repos,
reviewLookup,
settings,
worktreesByRepo
])
const reviewInfoByWorktreeId = useMemo(
() =>
computeWorkspaceCleanupReviewInfoIndex({
candidates,
candidateIdCounts,
reviewSources: reviewContext.sources,
reviewLookup: reviewContext.lookup,
cache: reviewContext.cache
}),
[candidateIdCounts, candidates, reviewContext]
)
const completedSizeByWorktreeId = useMemo(
() => buildWorkspaceCleanupSizeIndex(sources.spaceWorktrees, candidates),
@@ -169,33 +172,31 @@ export function useWorkspaceCleanupFacetRows({
])
}, [candidates, completedSizeByWorktreeId, sources.spaceMeasurements])
const facetCacheRef = useRef<WorkspaceCleanupFacetListCache | null>(null)
const facets = useMemo(() => {
const { cache, list } = computeWorkspaceCleanupFacetList({
const facets = useMemo(
() =>
computeWorkspaceCleanupFacetList({
candidates,
sources: {
worktreeById: facetContext.worktreeById,
workspaceStatuses: facetContext.workspaceStatuses,
sizeBytesByWorktreeId: sizeByWorktreeId,
lastVisitedAtByWorktreeId: sources.lastVisitedAtByWorktreeId,
liveAgentStatusByWorktreeId,
reviewInfoByWorktreeId,
dismissedWorktreeIds
},
cache: facetContext.cache
}),
[
candidates,
sources: {
worktreeById,
workspaceStatuses: sources.workspaceStatuses,
sizeBytesByWorktreeId: sizeByWorktreeId,
lastVisitedAtByWorktreeId: sources.lastVisitedAtByWorktreeId,
liveAgentStatusByWorktreeId,
reviewInfoByWorktreeId,
dismissedWorktreeIds
},
cache: facetCacheRef.current
})
facetCacheRef.current = cache
return list
}, [
candidates,
dismissedWorktreeIds,
liveAgentStatusByWorktreeId,
reviewInfoByWorktreeId,
sizeByWorktreeId,
sources.lastVisitedAtByWorktreeId,
sources.workspaceStatuses,
worktreeById
])
dismissedWorktreeIds,
facetContext,
liveAgentStatusByWorktreeId,
reviewInfoByWorktreeId,
sizeByWorktreeId,
sources.lastVisitedAtByWorktreeId
]
)
const result = useMemo(
() => runWorkspaceCleanupQuery(facets, { filters, sort }, now),
@@ -235,20 +236,13 @@ export function useWorkspaceCleanupFacetRows({
: EMPTY_FACET_COUNTS,
[facetFilters, facetPanelOpen, facets, now]
)
const matchedIdsRef = useRef<ReadonlySet<string>>(new Set())
const facetMatchedWorktreeIds = useMemo(() => {
const next = new Set(
filterWorkspaceCleanupFacets(facets, facetFilters, now).map((row) => row.worktreeId)
)
const previous = matchedIdsRef.current
// Why: destructive selection pruning keys off this set; identity must only
// change when membership does.
if (previous.size === next.size && [...next].every((id) => previous.has(id))) {
return previous
}
matchedIdsRef.current = next
return next
}, [facetFilters, facets, now])
// Identity churn here is harmless: the only consumer reads the latest set
// inside a useEffectEvent body and never keys an effect on it.
const facetMatchedWorktreeIds = useMemo<ReadonlySet<string>>(
() =>
new Set(filterWorkspaceCleanupFacets(facets, facetFilters, now).map((row) => row.worktreeId)),
[facetFilters, facets, now]
)
const measuredSizeCount = useMemo(() => countWorkspaceCleanupMeasuredRows(facets), [facets])
const unmeasuredSizeCount = facets.length - measuredSizeCount
@@ -23,16 +23,20 @@ import {
* rows reuse their previous facet/review objects and the row array itself
* keeps identity when nothing changed, which is what lets React.memo and
* downstream memos hold.
*
* The caches are owned by useMemo keyed on their invalidation inputs (never
* refs — ref writes during render are unsafe under concurrent rendering).
* Filling them during render is safe because they are content-addressed and
* idempotent: a replayed or discarded render stores the same value for the
* same key.
*/
type SourceToken = readonly unknown[]
export type WorkspaceCleanupReviewInfoCache = WeakMap<
WorkspaceCleanupCandidate,
WorkspaceCleanupReviewInfo
>
function tokensEqual(left: SourceToken, right: SourceToken): boolean {
return left.length === right.length && left.every((value, index) => value === right[index])
}
export type WorkspaceCleanupReviewInfoCache = {
token: SourceToken
byCandidate: WeakMap<WorkspaceCleanupCandidate, WorkspaceCleanupReviewInfo>
export function createWorkspaceCleanupReviewInfoCache(): WorkspaceCleanupReviewInfoCache {
return new WeakMap()
}
export function computeWorkspaceCleanupReviewInfoIndex(args: {
@@ -40,25 +44,15 @@ export function computeWorkspaceCleanupReviewInfoIndex(args: {
candidateIdCounts: ReadonlyMap<string, number>
reviewSources: WorkspaceCleanupRendererStateInputs
reviewLookup: WorkspaceCleanupReviewLookup
cache: WorkspaceCleanupReviewInfoCache | null
}): { cache: WorkspaceCleanupReviewInfoCache; infos: Map<string, WorkspaceCleanupReviewInfo> } {
const { candidates, candidateIdCounts, reviewSources, reviewLookup } = args
const token: SourceToken = [
reviewSources.hostedReviewCache,
reviewSources.repos,
reviewSources.settings,
reviewSources.worktreesByRepo
]
let cache = args.cache
if (!cache || !tokensEqual(cache.token, token)) {
cache = { token, byCandidate: new WeakMap() }
}
cache: WorkspaceCleanupReviewInfoCache
}): Map<string, WorkspaceCleanupReviewInfo> {
const { candidates, candidateIdCounts, reviewSources, reviewLookup, cache } = args
const infos = new Map<string, WorkspaceCleanupReviewInfo>()
for (const candidate of candidates) {
let info = cache.byCandidate.get(candidate)
let info = cache.get(candidate)
if (info === undefined) {
info = getWorkspaceCleanupReviewInfo(candidate, reviewSources, reviewLookup)
cache.byCandidate.set(candidate, info)
cache.set(candidate, info)
}
infos.set(
getWorkspaceCleanupHostIdentity(
@@ -71,7 +65,7 @@ export function computeWorkspaceCleanupReviewInfoIndex(args: {
infos.set(candidate.worktreeId, info)
}
}
return { cache, infos }
return infos
}
type FacetCacheEntry = {
@@ -84,24 +78,22 @@ type FacetCacheEntry = {
}
export type WorkspaceCleanupFacetListCache = {
token: SourceToken
byCandidate: WeakMap<WorkspaceCleanupCandidate, FacetCacheEntry>
lastList: WorkspaceCleanupFacets[]
}
export function createWorkspaceCleanupFacetListCache(): WorkspaceCleanupFacetListCache {
return { byCandidate: new WeakMap(), lastList: [] }
}
export function computeWorkspaceCleanupFacetList(args: {
candidates: readonly WorkspaceCleanupCandidate[]
sources: Required<Omit<WorkspaceCleanupFacetSources, 'workspaceStatuses'>> & {
workspaceStatuses: readonly WorkspaceStatusDefinition[]
}
cache: WorkspaceCleanupFacetListCache | null
}): { cache: WorkspaceCleanupFacetListCache; list: WorkspaceCleanupFacets[] } {
const { candidates, sources } = args
const token: SourceToken = [sources.worktreeById, sources.workspaceStatuses]
let cache = args.cache
if (!cache || !tokensEqual(cache.token, token)) {
cache = { token, byCandidate: new WeakMap(), lastList: [] }
}
cache: WorkspaceCleanupFacetListCache
}): WorkspaceCleanupFacets[] {
const { candidates, sources, cache } = args
const list = candidates.map((candidate) => {
const hostIdentity = getWorkspaceCleanupHostIdentity(
getWorkspaceCleanupCandidateHostId(candidate),
@@ -144,8 +136,8 @@ export function computeWorkspaceCleanupFacetList(args: {
// downstream memo skip its O(N) pass on no-op streaming ticks.
const previous = cache.lastList
if (previous.length === list.length && list.every((facet, index) => facet === previous[index])) {
return { cache, list: previous }
return previous
}
cache.lastList = list
return { cache, list }
return list
}