diff --git a/apps/web/hooks/domains/session/use-session-commits.test.ts b/apps/web/hooks/domains/session/use-session-commits.test.ts index af1e16e251..cb0094def1 100644 --- a/apps/web/hooks/domains/session/use-session-commits.test.ts +++ b/apps/web/hooks/domains/session/use-session-commits.test.ts @@ -154,16 +154,105 @@ describe("useSessionCommits", () => { }); }); -// Helper: seed store with one existing commit and a deferred request so we -// can observe the store mid-refetch (stale-while-revalidate test). +// Lives in its own describe so the outer block stays under the 100-line +// max-lines-per-function limit. +describe("useSessionCommits — authoritative snapshot on mount", () => { + beforeEach(() => { + vi.clearAllMocks(); + setStore(); + }); + + afterEach(() => { + cleanup(); + }); + + it("fetches a snapshot on mount even when commits were prefilled by live events", async () => { + // Regression: a `commit_created` WS event can populate the commits list + // before the hook mounts (e.g. session already running when the panel + // opens). If the live event carried stale/zero stats — possible during + // stream reconnect / replay — the panel displays them forever unless + // the authoritative `git log --shortstat` snapshot also runs. The + // pre-existing `commits === undefined` gate skipped that fetch. + storeState.sessionCommits = { + byEnvironmentId: { + "sess-1": [ + { + commit_sha: "older", + commit_message: "feat: x", + parent_sha: "base", + files_changed: 0, + insertions: 0, + deletions: 0, + }, + ], + }, + loading: {}, + refetchTrigger: {}, + }; + mockRequest.mockResolvedValueOnce({ + commits: [ + { + commit_sha: "older", + commit_message: "feat: x", + parent_sha: "base", + files_changed: 60, + insertions: 3443, + deletions: 227, + }, + ], + ready: true, + }); + + renderHook(() => useSessionCommits("sess-1")); + + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); + await waitFor(() => { + expect(mockSetSessionCommits).toHaveBeenCalledWith( + "sess-1", + [ + expect.objectContaining({ + commit_sha: "older", + files_changed: 60, + insertions: 3443, + deletions: 227, + }), + ], + undefined, + ); + }); + }); + + it("re-fetches after sessionId cycles null → same id (post-teardown reselect)", async () => { + // Greptile P2: if the hook stays mounted while sessionId goes null and the + // store's commits are cleared via clearSessionCommits, then when the same + // sessionId returns the ref still equals it — so no fetch fires and the + // panel stays blank. Clearing the ref in the early-return path fixes it. + mockRequest.mockResolvedValue({ commits: [{ commit_sha: "a" }], ready: true }); + + const { rerender } = renderHook(({ id }) => useSessionCommits(id), { + initialProps: { id: "sess-1" as string | null }, + }); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); + + rerender({ id: null }); + rerender({ id: "sess-1" }); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(2)); + }); +}); + +// Helper: seed store with one existing commit, resolve the mount-time +// snapshot fetch with that same data, and return a deferred resolver for +// the trigger-bump refetch so the test can observe the store mid-refetch. async function seedAndDeferRefetch(sessionId: string) { + const seeded = [{ commit_sha: "old", insertions: 1, deletions: 0 }]; storeState.sessionCommits = { - byEnvironmentId: { - [sessionId]: [{ commit_sha: "old", insertions: 1, deletions: 0 }], - }, + byEnvironmentId: { [sessionId]: seeded }, loading: {}, refetchTrigger: { [sessionId]: 0 }, }; + // The mount-time snapshot fetch resolves immediately with the seeded + // data (no-op write), so we can isolate the trigger-bump refetch below. + mockRequest.mockResolvedValueOnce({ commits: seeded, ready: true }); let resolveRequest!: (value: unknown) => void; mockRequest.mockReturnValueOnce( new Promise((resolve) => { @@ -194,12 +283,13 @@ describe("useSessionCommits — stale-while-revalidate on trigger bump", () => { it("refetches when refetchTrigger bumps without nulling the visible list", async () => { const resolveRequest = await seedAndDeferRefetch("sess-1"); const { rerender } = renderHook(() => useSessionCommits("sess-1")); - expect(mockRequest).not.toHaveBeenCalled(); + // The mount-time snapshot fetch fires once with the seeded data. + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); bumpTrigger("sess-1", 1); rerender(); - await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(2)); // During mid-refetch the store must still hold the OLD commits. const midRefetch = (storeState.sessionCommits as { byEnvironmentId: Record }) .byEnvironmentId; @@ -217,23 +307,26 @@ describe("useSessionCommits — stale-while-revalidate on trigger bump", () => { // After a `git reset`, the refetch legitimately returns []. Without // `allowEmpty: true`, the default guard in `setSessionCommits` would // silently drop that response and the panel would keep showing stale data. + const seeded = [ + { commit_sha: "a", insertions: 0, deletions: 0 }, + { commit_sha: "b", insertions: 0, deletions: 0 }, + ]; storeState.sessionCommits = { - byEnvironmentId: { - "sess-1": [ - { commit_sha: "a", insertions: 0, deletions: 0 }, - { commit_sha: "b", insertions: 0, deletions: 0 }, - ], - }, + byEnvironmentId: { "sess-1": seeded }, loading: {}, refetchTrigger: { "sess-1": 0 }, }; + // Mount-time snapshot fetch returns the seeded data (no-op write). + mockRequest.mockResolvedValueOnce({ commits: seeded, ready: true }); + // Trigger-bump fetch returns the authoritative empty list. mockRequest.mockResolvedValueOnce({ commits: [], ready: true }); const { rerender } = renderHook(() => useSessionCommits("sess-1")); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); bumpTrigger("sess-1", 1); rerender(); - await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(2)); await waitFor(() => { expect(mockSetSessionCommits).toHaveBeenCalledWith("sess-1", [], { allowEmpty: true }); }); @@ -245,11 +338,15 @@ describe("useSessionCommits — stale-while-revalidate on trigger bump", () => { }); it("drops a stale response when a newer fetch already started", async () => { + const seeded = [{ commit_sha: "initial" }]; storeState.sessionCommits = { - byEnvironmentId: { "sess-1": [{ commit_sha: "initial" }] }, + byEnvironmentId: { "sess-1": seeded }, loading: {}, refetchTrigger: { "sess-1": 0 }, }; + // Mount-time snapshot fetch — resolves immediately with the seeded data + // so the stale-vs-fresh race below only involves the trigger-bump fetches. + mockRequest.mockResolvedValueOnce({ commits: seeded, ready: true }); let resolveFirst!: (value: unknown) => void; let resolveSecond!: (value: unknown) => void; mockRequest @@ -265,14 +362,15 @@ describe("useSessionCommits — stale-while-revalidate on trigger bump", () => { ); const { rerender } = renderHook(() => useSessionCommits("sess-1")); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); bumpTrigger("sess-1", 1); rerender(); - await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(2)); bumpTrigger("sess-1", 2); rerender(); - await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(2)); + await waitFor(() => expect(mockRequest).toHaveBeenCalledTimes(3)); resolveFirst({ commits: [{ commit_sha: "stale" }], ready: true }); resolveSecond({ commits: [{ commit_sha: "fresh" }], ready: true }); @@ -283,9 +381,16 @@ describe("useSessionCommits — stale-while-revalidate on trigger bump", () => { expect(after["sess-1"]).toEqual([{ commit_sha: "fresh" }]); }); - const winningCalls = mockSetSessionCommits.mock.calls.filter( - ([, commits]) => Array.isArray(commits) && commits.length > 0, - ); - expect(winningCalls).toHaveLength(1); + // The mount-fetch write (seeded data) and the fresh write should land; + // the stale write must be dropped by the request-version guard. + const writtenSHAs = mockSetSessionCommits.mock.calls + .map(([, commits]) => + Array.isArray(commits) && commits.length > 0 + ? (commits[0] as { commit_sha: string }).commit_sha + : null, + ) + .filter((sha): sha is string => sha !== null); + expect(writtenSHAs).not.toContain("stale"); + expect(writtenSHAs).toContain("fresh"); }); }); diff --git a/apps/web/hooks/domains/session/use-session-commits.ts b/apps/web/hooks/domains/session/use-session-commits.ts index ab30eac263..bda148f9a3 100644 --- a/apps/web/hooks/domains/session/use-session-commits.ts +++ b/apps/web/hooks/domains/session/use-session-commits.ts @@ -46,6 +46,15 @@ export function useSessionCommits(sessionId: string | null) { // still drives an initial refetch — otherwise prevRef would equal the live // value and `triggerBumped` would silently be false on first render. const prevRefetchTriggerRef = useRef(REFETCH_TRIGGER_INIT); + // Tracks which sessionId we've already run an authoritative snapshot fetch + // for. Without this, the initial-fetch gate fires only when `commits` is + // undefined — but commits can be populated by a live `commit_created` + // event that arrived before mount, and a replayed/raced event can carry + // zero stats. The bad stats then stick because the snapshot (which would + // overwrite with correct stats from `git log --shortstat`) never runs. + // Anchoring to sessionId guarantees a snapshot per session regardless of + // how `commits` got populated. + const fetchedSessionRef = useRef(null); // Retry timer for the not-ready case — agentctl recovers asynchronously // after a backend restart, so the first fetch may land before the workspace // execution has been ensured. Without a retry the store would be stuck on @@ -125,33 +134,43 @@ export function useSessionCommits(sessionId: string | null) { ); // Fetch commits when: - // 1. commits is undefined (initial load — clearSessionCommits is still - // called by callers that genuinely want to drop the list, e.g. session - // teardown). - // 2. the refetch trigger was bumped (commits_reset / branch_switched). + // 1. The hook mounts (or sessionId changes) — runs an authoritative + // snapshot once per session so live `commit_created` events that + // populated the store with stale/zero stats get overwritten by the + // real `git log --shortstat` data. + // 2. The refetch trigger was bumped (commits_reset / branch_switched). // - // The trigger path replaces the old "clear then refetch from undefined" - // pattern: it keeps the previous commits in the store so the Changes panel - // doesn't flicker through its empty state while the refetch is in flight - // (stale-while-revalidate, matching how useCumulativeDiff works). + // The trigger path keeps the previous commits in the store while the + // refetch is in flight (stale-while-revalidate, matching how + // useCumulativeDiff works). useEffect(() => { + // !sessionId clears the ref BEFORE the connection check so a teardown + // during a disconnect still resets the gate — otherwise the ref would + // retain the old id, and a same-id reselect after reconnect would skip + // the snapshot fetch ("session teardown during disconnect can still + // block the next snapshot fetch for the same session"). + if (!sessionId) { + fetchedSessionRef.current = null; + return; + } if (connectionStatus !== "connected") return; - if (!sessionId) return; - const needsInitialFetch = commits === undefined; const triggerBumped = refetchTrigger !== prevRefetchTriggerRef.current; + const sessionChanged = fetchedSessionRef.current !== sessionId; if (triggerBumped) { // Trigger-bump path: the backend already mutated state (reset / branch // switch). An empty response is authoritative — bypass the default // anti-race guard so the panel reflects the actual post-reset state. fetchCommits({ allowEmpty: true }); - } else if (needsInitialFetch) { + fetchedSessionRef.current = sessionId; + } else if (sessionChanged) { fetchCommits(); + fetchedSessionRef.current = sessionId; } prevRefetchTriggerRef.current = refetchTrigger; - }, [sessionId, commits, refetchTrigger, fetchCommits, connectionStatus]); + }, [sessionId, refetchTrigger, fetchCommits, connectionStatus]); // Cancel any in-flight retry on unmount, when the session changes, or when // the WS disconnects — a retry firing against a disconnected client would