mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: prevent git diff panel scroll jumps on chat updates (#23243)
Three changes that eliminate unnecessary re-renders cascading into the FileDiff Shadow DOM components during chat/git-watcher updates: useGitWatcher: compare repo fields before updating state, return prev Map when nothing changed instead of always allocating a new one. RemoteDiffPanel: remove dataUpdatedAt from parsedFiles memo deps, replace it with a content-derived version counter. The memo now only recomputes when the actual diff string changes. DiffViewer: pre-compute per-file options and line annotations into memoized Maps, wrap LazyFileDiff in React.memo so it skips renders when props are reference-equal.
This commit is contained in:
@@ -13,6 +13,7 @@ import { ChevronRightIcon } from "lucide-react";
|
||||
import {
|
||||
type ComponentProps,
|
||||
type FC,
|
||||
memo,
|
||||
type ReactNode,
|
||||
useCallback,
|
||||
useEffect,
|
||||
@@ -340,66 +341,68 @@ const FileTreeNodeView: FC<{
|
||||
* FileDiff that the user has already scrolled past, which avoids
|
||||
* layout shifts and repeated highlighting work.
|
||||
*/
|
||||
const LazyFileDiff: FC<{
|
||||
const LazyFileDiff = memo<{
|
||||
fileDiff: FileDiffMetadata;
|
||||
options: ComponentProps<typeof FileDiff>["options"];
|
||||
lineAnnotations?: DiffLineAnnotation<string>[];
|
||||
renderAnnotation?: (annotation: DiffLineAnnotation<string>) => ReactNode;
|
||||
}> = ({
|
||||
fileDiff,
|
||||
options,
|
||||
lineAnnotations,
|
||||
renderAnnotation: renderAnnotationProp,
|
||||
}) => {
|
||||
const placeholderRef = useRef<HTMLDivElement>(null);
|
||||
const [visible, setVisible] = useState(false);
|
||||
}>(
|
||||
({
|
||||
fileDiff,
|
||||
options,
|
||||
lineAnnotations,
|
||||
renderAnnotation: renderAnnotationProp,
|
||||
}) => {
|
||||
const placeholderRef = useRef<HTMLDivElement>(null);
|
||||
const [visible, setVisible] = useState(false);
|
||||
|
||||
useEffect(() => {
|
||||
const el = placeholderRef.current;
|
||||
if (!el || visible) {
|
||||
return;
|
||||
useEffect(() => {
|
||||
const el = placeholderRef.current;
|
||||
if (!el || visible) {
|
||||
return;
|
||||
}
|
||||
const observer = new IntersectionObserver(
|
||||
([entry]) => {
|
||||
if (entry.isIntersecting) {
|
||||
setVisible(true);
|
||||
observer.disconnect();
|
||||
}
|
||||
},
|
||||
// Pre-load files that are within one viewport-height of
|
||||
// the visible area so they are ready before the user
|
||||
// scrolls to them.
|
||||
{ rootMargin: "100% 0px" },
|
||||
);
|
||||
observer.observe(el);
|
||||
return () => observer.disconnect();
|
||||
}, [visible]);
|
||||
|
||||
if (!visible) {
|
||||
return (
|
||||
<div
|
||||
ref={placeholderRef}
|
||||
style={{ height: estimateDiffHeight(fileDiff) }}
|
||||
className="p-4 space-y-2"
|
||||
>
|
||||
<Skeleton className="h-4 w-48" />
|
||||
<Skeleton className="h-3 w-full" />
|
||||
<Skeleton className="h-3 w-full" />
|
||||
<Skeleton className="h-3 w-3/4" />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
const observer = new IntersectionObserver(
|
||||
([entry]) => {
|
||||
if (entry.isIntersecting) {
|
||||
setVisible(true);
|
||||
observer.disconnect();
|
||||
}
|
||||
},
|
||||
// Pre-load files that are within one viewport-height of
|
||||
// the visible area so they are ready before the user
|
||||
// scrolls to them.
|
||||
{ rootMargin: "100% 0px" },
|
||||
);
|
||||
observer.observe(el);
|
||||
return () => observer.disconnect();
|
||||
}, [visible]);
|
||||
|
||||
if (!visible) {
|
||||
return (
|
||||
<div
|
||||
ref={placeholderRef}
|
||||
style={{ height: estimateDiffHeight(fileDiff) }}
|
||||
className="p-4 space-y-2"
|
||||
>
|
||||
<Skeleton className="h-4 w-48" />
|
||||
<Skeleton className="h-3 w-full" />
|
||||
<Skeleton className="h-3 w-full" />
|
||||
<Skeleton className="h-3 w-3/4" />
|
||||
</div>
|
||||
<FileDiff
|
||||
fileDiff={fileDiff}
|
||||
options={options}
|
||||
style={DIFFS_FONT_STYLE}
|
||||
lineAnnotations={lineAnnotations}
|
||||
renderAnnotation={renderAnnotationProp}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<FileDiff
|
||||
fileDiff={fileDiff}
|
||||
options={options}
|
||||
style={DIFFS_FONT_STYLE}
|
||||
lineAnnotations={lineAnnotations}
|
||||
renderAnnotation={renderAnnotationProp}
|
||||
/>
|
||||
);
|
||||
};
|
||||
},
|
||||
);
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
// Main component
|
||||
@@ -505,6 +508,30 @@ export const DiffViewer: FC<DiffViewerProps> = ({
|
||||
);
|
||||
}, [fileTree, parsedFiles]);
|
||||
|
||||
// Pre-compute per-file options so each LazyFileDiff receives a
|
||||
// stable reference and avoids re-highlighting on parent re-render.
|
||||
const perFileOptions = useMemo(() => {
|
||||
if (!hasPerFileCallbacks) return null;
|
||||
const map = new Map<string, ComponentProps<typeof FileDiff>["options"]>();
|
||||
for (const file of sortedFiles) {
|
||||
map.set(file.name, getOptionsForFile(file.name));
|
||||
}
|
||||
return map;
|
||||
}, [hasPerFileCallbacks, sortedFiles, getOptionsForFile]);
|
||||
|
||||
// Pre-compute per-file line annotations for the same reason.
|
||||
const perFileAnnotations = useMemo(() => {
|
||||
if (!getLineAnnotations) return null;
|
||||
const map = new Map<string, DiffLineAnnotation<string>[]>();
|
||||
for (const file of sortedFiles) {
|
||||
const annotations = getLineAnnotations(file.name);
|
||||
if (annotations.length > 0) {
|
||||
map.set(file.name, annotations);
|
||||
}
|
||||
}
|
||||
return map;
|
||||
}, [sortedFiles, getLineAnnotations]);
|
||||
|
||||
// ---------------------------------------------------------------
|
||||
// Container width measurement via ResizeObserver so we can decide
|
||||
// whether to show the file tree sidebar without a prop from the
|
||||
@@ -730,12 +757,8 @@ export const DiffViewer: FC<DiffViewerProps> = ({
|
||||
>
|
||||
<LazyFileDiff
|
||||
fileDiff={fileDiff}
|
||||
options={
|
||||
hasPerFileCallbacks
|
||||
? getOptionsForFile(fileDiff.name)
|
||||
: fileOptions
|
||||
}
|
||||
lineAnnotations={getLineAnnotations?.(fileDiff.name)}
|
||||
options={perFileOptions?.get(fileDiff.name) ?? fileOptions}
|
||||
lineAnnotations={perFileAnnotations?.get(fileDiff.name)}
|
||||
renderAnnotation={renderAnnotation}
|
||||
/>
|
||||
</div>
|
||||
|
||||
@@ -30,6 +30,18 @@ import type { DiffStyle } from "./DiffViewer";
|
||||
import { DiffViewer } from "./DiffViewer";
|
||||
import { parsePullRequestUrl } from "./pullRequest";
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
// Module-level counter for cache key uniqueness
|
||||
// -------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Monotonic counter shared across all RemoteDiffPanel instances.
|
||||
* Ensures parsePatchFiles cache keys never collide across mounts,
|
||||
* since component-local refs reset to 0 on remount while the
|
||||
* worker pool's LRU cache persists.
|
||||
*/
|
||||
let remoteDiffVersion = 0;
|
||||
|
||||
// -------------------------------------------------------------------
|
||||
// Diff content extraction
|
||||
// -------------------------------------------------------------------
|
||||
@@ -256,28 +268,38 @@ export const RemoteDiffPanel: FC<RemoteDiffPanelProps> = ({
|
||||
enabled: Boolean(diffStatus?.url),
|
||||
});
|
||||
|
||||
const diffContent = diffContentsQuery.data?.diff;
|
||||
const diffVersionRef = useRef(0);
|
||||
const prevDiffRef = useRef<string | undefined>(undefined);
|
||||
if (diffContent !== prevDiffRef.current) {
|
||||
prevDiffRef.current = diffContent;
|
||||
diffVersionRef.current = ++remoteDiffVersion;
|
||||
}
|
||||
|
||||
const parsedFiles = useMemo(() => {
|
||||
const diff = diffContentsQuery.data?.diff;
|
||||
if (!diff) {
|
||||
if (!diffContent) {
|
||||
return [];
|
||||
}
|
||||
try {
|
||||
// The cacheKeyPrefix enables the worker pool's LRU cache
|
||||
// so highlighted ASTs are reused across re-renders instead
|
||||
// of being re-computed on every render cycle. We include
|
||||
// dataUpdatedAt so that when the diff content changes
|
||||
// (e.g. new commits pushed) the old cached highlight AST
|
||||
// is not reused with mismatched line indices, which would
|
||||
// cause DiffHunksRenderer.processDiffResult to throw.
|
||||
// of being re-computed on every render cycle. We include a
|
||||
// version counter derived from the diff content so that when
|
||||
// the diff changes (e.g. new commits pushed) the old cached
|
||||
// highlight AST is not reused with mismatched line indices,
|
||||
// which would cause DiffHunksRenderer.processDiffResult to
|
||||
// throw. Unlike dataUpdatedAt, this counter only increments
|
||||
// when the actual diff string changes, avoiding unnecessary
|
||||
// recomputation on refetches with identical content.
|
||||
const patches = parsePatchFiles(
|
||||
diff,
|
||||
`chat-${chatId}-${diffContentsQuery.dataUpdatedAt}`,
|
||||
diffContent,
|
||||
`chat-${chatId}-v${diffVersionRef.current}`,
|
||||
);
|
||||
return patches.flatMap((p) => p.files);
|
||||
} catch {
|
||||
return [];
|
||||
}
|
||||
}, [diffContentsQuery.data?.diff, diffContentsQuery.dataUpdatedAt, chatId]);
|
||||
}, [diffContent, chatId]);
|
||||
|
||||
// ---------------------------------------------------------------
|
||||
// Line interaction callbacks
|
||||
|
||||
@@ -77,15 +77,28 @@ export function useGitWatcher({
|
||||
|
||||
if (data.type === "changes" && data.repositories) {
|
||||
setRepositories((prev) => {
|
||||
let changed = false;
|
||||
const next = new Map(prev);
|
||||
for (const repo of data.repositories!) {
|
||||
if (repo.removed) {
|
||||
next.delete(repo.repo_root);
|
||||
if (next.has(repo.repo_root)) {
|
||||
next.delete(repo.repo_root);
|
||||
changed = true;
|
||||
}
|
||||
} else {
|
||||
next.set(repo.repo_root, repo);
|
||||
const existing = next.get(repo.repo_root);
|
||||
if (
|
||||
!existing ||
|
||||
existing.branch !== repo.branch ||
|
||||
existing.remote_origin !== repo.remote_origin ||
|
||||
existing.unified_diff !== repo.unified_diff
|
||||
) {
|
||||
next.set(repo.repo_root, repo);
|
||||
changed = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
return next;
|
||||
return changed ? next : prev;
|
||||
});
|
||||
} else if (data.type === "error") {
|
||||
console.warn("[useGitWatcher] server error:", data.message);
|
||||
|
||||
Reference in New Issue
Block a user