fix: diff panel follow-ups from #23243 (#23247)

LazyFileDiff memo comparator: ignore renderAnnotation reference
changes when the file has no lineAnnotations, preventing comment-box
interactions from re-rendering every file diff.

useGitWatcher stale socket guard: check socketRef.current against
the local socket variable in all event handlers. Stale close events
from superseded connections no longer clobber the active socket or
schedule spurious reconnects.

useGitWatcher field comparison guard: add a compile-time Record
type that errors when WorkspaceAgentRepoChanges gains a field not
covered by the bailout comparison.

Tests: stale close race, reference stability on duplicate messages,
per-field change detection (branch, remote_origin, unified_diff),
and no-op removal of unknown repos.

Follow-up to #23243
This commit is contained in:
Mathias Fredriksson
2026-03-19 14:41:55 +00:00
committed by GitHub
parent 635ce1f064
commit c424c31ab8
3 changed files with 233 additions and 0 deletions
+18
View File
@@ -402,6 +402,24 @@ const LazyFileDiff = memo<{
/>
);
},
(prev, next) => {
if (
prev.fileDiff !== next.fileDiff ||
prev.options !== next.options ||
prev.lineAnnotations !== next.lineAnnotations
) {
return false;
}
// When neither the previous nor next props include line
// annotations, a new `renderAnnotation` reference is
// irrelevant because there is nothing to render. Skip the
// re-render so that unrelated state changes (e.g.
// activeCommentBox) don't bust memo for every file.
if (prev.renderAnnotation !== next.renderAnnotation) {
return !prev.lineAnnotations && !next.lineAnnotations;
}
return true;
},
);
// -------------------------------------------------------------------
@@ -425,4 +425,194 @@ describe("useGitWatcher", () => {
});
expect(result.current.repositories.has("/home/user/project-x")).toBe(true);
});
it("stale close event after re-mount does not create duplicate connections", () => {
vi.useFakeTimers();
try {
const socket1 = createMockSocket();
const { result, rerender } = renderHook(
({ chatId }: { chatId: string | undefined }) =>
useGitWatcher({ chatId, agentStatus: "connected" }),
{ initialProps: { chatId: "chat-aaa" as string | undefined } },
);
act(() => socket1.simulateOpen());
expect(mockWatchChatGit).toHaveBeenCalledTimes(1);
// Prepare socket2 for the re-mount triggered by chatId change.
const socket2 = createMockSocket();
rerender({ chatId: "chat-bbb" });
expect(socket1.close).toHaveBeenCalled();
expect(mockWatchChatGit).toHaveBeenCalledTimes(2);
// Simulate socket1's close event arriving late (stale).
// This must NOT clobber socketRef or schedule a reconnect.
act(() => socket1.simulateClose());
expect(mockWatchChatGit).toHaveBeenCalledTimes(2);
// Advance timers to prove no reconnect was scheduled.
act(() => vi.advanceTimersByTime(60_000));
expect(mockWatchChatGit).toHaveBeenCalledTimes(2);
// socket2 should still work: open sets isConnected,
// messages update repositories.
act(() => socket2.simulateOpen());
expect(result.current.isConnected).toBe(true);
act(() => {
socket2.simulateMessage({
type: "changes",
repositories: [{ repo_root: "/repo", branch: "main" }],
});
});
expect(result.current.repositories.size).toBe(1);
} finally {
vi.useRealTimers();
}
});
it("preserves reference on duplicate messages", () => {
const socket = createMockSocket();
const { result } = renderHook(() =>
useGitWatcher({ chatId: "chat-123", agentStatus: "connected" }),
);
act(() => socket.simulateOpen());
const message = {
type: "changes" as const,
repositories: [
{
repo_root: "/repo",
branch: "main",
unified_diff: "diff1",
},
],
};
act(() => socket.simulateMessage(message));
const ref1 = result.current.repositories;
expect(ref1.size).toBe(1);
// Sending the exact same data should not produce a new reference.
act(() => socket.simulateMessage(message));
expect(result.current.repositories).toBe(ref1);
});
it("single field change triggers update", () => {
const socket = createMockSocket();
const { result } = renderHook(() =>
useGitWatcher({ chatId: "chat-123", agentStatus: "connected" }),
);
act(() => socket.simulateOpen());
const base = {
repo_root: "/repo",
branch: "main",
remote_origin: "git@github.com:org/repo.git",
unified_diff: "diff1",
};
act(() => {
socket.simulateMessage({
type: "changes",
repositories: [base],
});
});
let ref = result.current.repositories;
expect(ref.get("/repo")?.branch).toBe("main");
// Changing only branch.
act(() => {
socket.simulateMessage({
type: "changes",
repositories: [{ ...base, branch: "feature" }],
});
});
expect(result.current.repositories).not.toBe(ref);
expect(result.current.repositories.get("/repo")?.branch).toBe("feature");
ref = result.current.repositories;
// Changing only remote_origin.
act(() => {
socket.simulateMessage({
type: "changes",
repositories: [
{
...base,
branch: "feature",
remote_origin: "https://github.com/org/repo",
},
],
});
});
expect(result.current.repositories).not.toBe(ref);
ref = result.current.repositories;
// Changing only unified_diff.
act(() => {
socket.simulateMessage({
type: "changes",
repositories: [
{
...base,
branch: "feature",
remote_origin: "https://github.com/org/repo",
unified_diff: "diff2",
},
],
});
});
expect(result.current.repositories).not.toBe(ref);
expect(result.current.repositories.get("/repo")?.unified_diff).toBe(
"diff2",
);
});
it("removing unknown repo preserves reference", () => {
const socket = createMockSocket();
const { result } = renderHook(() =>
useGitWatcher({ chatId: "chat-123", agentStatus: "connected" }),
);
act(() => socket.simulateOpen());
act(() => {
socket.simulateMessage({
type: "changes",
repositories: [
{
repo_root: "/repo",
branch: "main",
unified_diff: "diff1",
},
],
});
});
const ref1 = result.current.repositories;
expect(ref1.size).toBe(1);
// Removing a repo_root that was never added should be a no-op.
act(() => {
socket.simulateMessage({
type: "changes",
repositories: [
{
repo_root: "/unknown",
branch: "",
removed: true,
},
],
});
});
expect(result.current.repositories).toBe(ref1);
});
});
@@ -7,6 +7,19 @@ import type {
} from "api/typesGenerated";
import { useCallback, useEffect, useRef, useState } from "react";
// Compile-time guard: ensures the bailout comparison in setRepositories
// covers every data field. If WorkspaceAgentRepoChanges gains a new
// field, this will error until the comparison is updated.
type _ComparedRepoFields = Omit<
WorkspaceAgentRepoChanges,
"repo_root" | "removed"
>;
const _repoFieldGuard: Record<keyof _ComparedRepoFields, true> = {
branch: true,
remote_origin: true,
unified_diff: true,
};
interface UseGitWatcherOptions {
chatId: string | undefined;
agentStatus: WorkspaceAgentStatus | undefined;
@@ -65,11 +78,19 @@ export function useGitWatcher({
socketRef.current = socket;
socket.addEventListener("open", () => {
// Ignore open events from superseded connections.
if (socketRef.current !== socket) {
return;
}
setIsConnected(true);
reconnectAttemptRef.current = 0;
});
socket.addEventListener("message", (event) => {
// Ignore messages from superseded connections.
if (socketRef.current !== socket) {
return;
}
try {
const data = JSON.parse(
String(event.data),
@@ -111,6 +132,10 @@ export function useGitWatcher({
// Note: WebSocket "error" events are always followed by a "close"
// event, so reconnection is handled here.
socket.addEventListener("close", () => {
// Ignore close events from superseded connections.
if (socketRef.current !== socket) {
return;
}
setIsConnected(false);
socketRef.current = null;