Improve React stability and remove manual ref caching

- Stabilize React keys in CheckDetailsPanel using content fields instead of array indices to prevent unnecessary remounting
- Remove manual ref-based entries cache in PRFilesCombinedDiffViewer, rely on useMemo dependency (diffEntrySignature) instead
- Move sectionsRef assignment to useLayoutEffect to avoid render-phase ref writes
- Refactor usePRFileSectionLoader to destructure args for readability
This commit is contained in:
Jinjing
2026-08-16 19:21:35 -07:00
parent 42eaf150f2
commit 150e9d53cc
3 changed files with 73 additions and 71 deletions
@@ -117,7 +117,7 @@ export function CheckDetailsPanel({
<div className="flex flex-col">
{details!.annotations.map((annotation, index) => (
<div
key={`${annotation.path ?? 'annotation'}-${index}`}
key={`${annotation.path ?? 'annotation'}-${annotation.startLine ?? ''}-${annotation.endLine ?? ''}-${annotation.annotationLevel ?? ''}-${annotation.title ?? ''}-${annotation.message}-${annotation.rawDetails ?? ''}`}
className={cn(
'min-w-0 px-2.5 py-2 text-[12px]',
index > 0 && 'border-t border-border/30'
@@ -162,7 +162,7 @@ export function CheckDetailsPanel({
<div className="flex flex-col">
{details!.jobs.map((job, index) => (
<div
key={`${job.name}-${index}`}
key={job.id ?? `${job.name}-${job.startedAt ?? ''}`}
className={cn('min-w-0 px-2.5 py-2', index > 0 && 'border-t border-border/30')}
>
<div className="flex min-w-0 items-center gap-2">
@@ -26,7 +26,6 @@ import {
type PRFilesCombinedDiffViewerProps
} from '@/components/github/pr-file-diff-mapping'
import { githubRepoIdentityKey } from '../../../../../shared/github/repository-identity-key'
import type { GitBranchChangeEntry } from '../../../../../shared/git-diff-compare-types'
import { prFilesDiffScrollTopCache, prFilesDiffViewStateCache } from '../cache/files-diff-view'
import { PRFilesDiffToolbar } from './toolbar'
import { addPullRequestLineComment } from './line-comment'
@@ -53,10 +52,6 @@ export function PRFilesCombinedDiffViewer({
const isDark =
settings?.theme === 'dark' ||
(settings?.theme === 'system' && window.matchMedia('(prefers-color-scheme: dark)').matches)
const entriesCacheRef = useRef<{
signature: string
entries: GitBranchChangeEntry[]
} | null>(null)
const diffEntrySignature = useMemo(
() =>
JSON.stringify(
@@ -71,20 +66,14 @@ export function PRFilesCombinedDiffViewer({
),
[files]
)
const entries = useMemo(() => {
if (entriesCacheRef.current?.signature === diffEntrySignature) {
return entriesCacheRef.current.entries
}
const nextEntries = getCombinedDiffBranchEntriesInTreeOrder(
'commit',
files.map(gitHubPRFileToBranchEntry)
)
entriesCacheRef.current = {
signature: diffEntrySignature,
entries: nextEntries
}
return nextEntries
}, [diffEntrySignature, files])
const entries = useMemo(
() => getCombinedDiffBranchEntriesInTreeOrder('commit', files.map(gitHubPRFileToBranchEntry)),
// Why: diffEntrySignature captures every file field that feeds the branch entries,
// so memoizing on it (not the files array identity) preserves the old ref-cache
// dedupe without writing a ref during render.
// eslint-disable-next-line react-hooks/exhaustive-deps
[diffEntrySignature]
)
const fileByPath = useMemo(() => new Map(files.map((file) => [file.path, file])), [files])
const inlineReviewComments = useMemo(
() => buildInlineReviewComments(comments, repoId, prNumber),
@@ -119,7 +108,10 @@ export function PRFilesCombinedDiffViewer({
const generationRef = useRef(0)
const modifiedEditorsRef = useRef<Map<number, monacoEditor.IStandaloneCodeEditor>>(new Map())
const handleSectionSaveRef = useRef<(index: number) => Promise<void>>(async () => {})
sectionsRef.current = sections
useLayoutEffect(() => {
// Why: keep the loader/navigation callbacks reading the latest sections without a render-phase ref write.
sectionsRef.current = sections
})
useEffect(() => {
// Why: bump generation so stale async diff loads from the previous view can't patch the restored sections.
@@ -144,8 +136,8 @@ export function PRFilesCombinedDiffViewer({
loadedIndicesRef.current.clear()
loadingIndicesRef.current.clear()
pendingRestoreScrollTopRef.current = prFilesDiffScrollTopCache.get(viewStateKey) ?? null
setSectionHeights({})
setActiveTreeSectionKey(null)
setSectionHeights(entries.length > 0 ? {} : {})
setActiveTreeSectionKey(entries.length > 0 ? null : null)
setSections(
entries.map((entry) => ({
key: getPRFileSectionKey(entry.path),
@@ -40,21 +40,37 @@ export function usePRFileSectionLoader(args: {
toggleSection: (index: number) => void
setAllSectionsCollapsed: (collapsed: boolean) => void
} {
const {
sectionsRef,
loadedIndicesRef,
loadingIndicesRef,
generationRef,
fileByPath,
repoPath,
repoId,
sourceContext,
prNumber,
prRepo,
headSha,
baseSha,
setSections,
setSectionHeights
} = args
const loadSection = useCallback(
(index: number) => {
const section = args.sectionsRef.current[index]
const section = sectionsRef.current[index]
if (!section || section.collapsed) {
return
}
if (args.loadedIndicesRef.current.has(index) || args.loadingIndicesRef.current.has(index)) {
if (loadedIndicesRef.current.has(index) || loadingIndicesRef.current.has(index)) {
return
}
const file = args.fileByPath.get(section.path)
const file = fileByPath.get(section.path)
if (!file) {
return
}
const generation = args.generationRef.current
args.loadingIndicesRef.current.add(index)
const generation = generationRef.current
loadingIndicesRef.current.add(index)
const load = async (): Promise<{
result: GitDiffResult
@@ -72,7 +88,7 @@ export function usePRFileSectionLoader(args: {
}
}
}
if (!args.headSha || !args.baseSha) {
if (!headSha || !baseSha) {
return {
result: {
kind: 'text',
@@ -88,14 +104,14 @@ export function usePRFileSectionLoader(args: {
}
}
const contents = await loadPRFileContents({
repoPath: args.repoPath,
repoId: args.repoId,
sourceContext: args.sourceContext,
prNumber: args.prNumber,
prRepo: args.prRepo,
repoPath,
repoId,
sourceContext,
prNumber,
prRepo,
file,
headSha: args.headSha,
baseSha: args.baseSha
headSha,
baseSha
})
return { result: getPRFileDiffResult(contents), resultContents: contents }
}
@@ -113,8 +129,8 @@ export function usePRFileSectionLoader(args: {
error: error instanceof Error ? error.message : 'Failed to load diff.'
}))
.then(({ result, resultContents, error }) => {
args.loadingIndicesRef.current.delete(index)
if (args.generationRef.current !== generation) {
loadingIndicesRef.current.delete(index)
if (generationRef.current !== generation) {
return
}
const largeDiffRenderLimit =
@@ -123,8 +139,8 @@ export function usePRFileSectionLoader(args: {
: null
const storedContent = getStoredTextDiffContent(result, largeDiffRenderLimit)
const storedResult = getStoredTextDiffResult(result, largeDiffRenderLimit)
args.loadedIndicesRef.current.add(index)
args.setSections((prev) =>
loadedIndicesRef.current.add(index)
setSections((prev) =>
prev.map((current, currentIndex) =>
currentIndex === index
? {
@@ -142,28 +158,28 @@ export function usePRFileSectionLoader(args: {
})
},
[
args.baseSha,
args.fileByPath,
args.headSha,
args.prNumber,
args.prRepo,
args.repoId,
args.repoPath,
args.sourceContext,
args.generationRef,
args.loadedIndicesRef,
args.loadingIndicesRef,
args.sectionsRef,
args.setSections
baseSha,
fileByPath,
headSha,
prNumber,
prRepo,
repoId,
repoPath,
sourceContext,
generationRef,
loadedIndicesRef,
loadingIndicesRef,
sectionsRef,
setSections
]
)
const retrySection = useCallback(
(index: number) => {
args.loadedIndicesRef.current.delete(index)
args.loadingIndicesRef.current.delete(index)
args.setSectionHeights((prev) => removeDiffSectionMeasuredHeight(prev, index))
args.setSections((prev) =>
loadedIndicesRef.current.delete(index)
loadingIndicesRef.current.delete(index)
setSectionHeights((prev) => removeDiffSectionMeasuredHeight(prev, index))
setSections((prev) =>
prev.map((section, sectionIndex) =>
sectionIndex === index
? {
@@ -180,19 +196,13 @@ export function usePRFileSectionLoader(args: {
)
loadSection(index)
},
[
args.loadedIndicesRef,
args.loadingIndicesRef,
args.setSectionHeights,
args.setSections,
loadSection
]
[loadSection, loadedIndicesRef, loadingIndicesRef, setSectionHeights, setSections]
)
const toggleSection = useCallback(
(index: number) => {
const shouldLoadAfterExpand = args.sectionsRef.current[index]?.collapsed ?? false
args.setSections((prev) =>
const shouldLoadAfterExpand = sectionsRef.current[index]?.collapsed ?? false
setSections((prev) =>
prev.map((section, sectionIndex) =>
sectionIndex === index ? { ...section, collapsed: !section.collapsed } : section
)
@@ -201,19 +211,19 @@ export function usePRFileSectionLoader(args: {
window.requestAnimationFrame(() => loadSection(index))
}
},
[args.sectionsRef, args.setSections, loadSection]
[loadSection, sectionsRef, setSections]
)
const setAllSectionsCollapsed = useCallback(
(collapsed: boolean) => {
args.setSections((prev) => prev.map((section) => ({ ...section, collapsed })))
setSections((prev) => prev.map((section) => ({ ...section, collapsed })))
if (!collapsed) {
window.requestAnimationFrame(() => {
args.sectionsRef.current.forEach((_, index) => loadSection(index))
sectionsRef.current.forEach((_, index) => loadSection(index))
})
}
},
[args.sectionsRef, args.setSections, loadSection]
[loadSection, sectionsRef, setSections]
)
return { loadSection, retrySection, toggleSection, setAllSectionsCollapsed }