fix(web): expand folder optimistically on first click in Files panel (#984)

* fix(web): expand folder optimistically on first click in Files panel The first click on a folder in the Files panel previously deferred the expanded-state update until after loadNodeChildren resolved, so the chevron and children only appeared once the WebSocket round-trip finished. Users perceived the click as a no-op and had to click again. Flip the expanded set synchronously, then load children in the background. Extracts the toggle logic into a testable helper and locks in the regression with unit tests. Closes #983 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(web): collapse toggleFolderExpand docstring to a single line Per the CLAUDE.md "no multi-line comment blocks" rule flagged by review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web): dedupe in-flight loadNodeChildren and collapse comment to one line - Add `activeLoadsRef` + `isLoading(path)` to `useLoadingTimers`, and guard `loadNodeChildren` against duplicate fetches for the same path. Rapid double-clicks on a freshly collapsed folder no longer issue two identical WebSocket round-trips (Greptile P2). - Collapse the `toggleFolderExpand` comment to a single line per CLAUDE.md (Claude review). - Add `file-browser-load-children.test.ts` covering the dedupe contract, the post-finish re-fetch path, and the children-already-loaded short-circuit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-21 14:01:31 +01:00
parent d02441201f
commit 33a378a8dc
4 changed files with 279 additions and 15 deletions
@@ -186,9 +186,11 @@ function applyFileChanges(ctx: {

 function useLoadingTimers() {
  const loadingTimersRef = useRef<Map<string, NodeJS.Timeout>>(new Map());
+  const activeLoadsRef = useRef<Set<string>>(new Set());
  const [visibleLoadingPaths, setVisibleLoadingPaths] = useState<Set<string>>(new Set());

  const showLoading = useCallback((path: string) => {
+    activeLoadsRef.current.add(path);
    const timer = setTimeout(() => {
      setVisibleLoadingPaths((prev) => new Set(prev).add(path));
      loadingTimersRef.current.delete(path);
@@ -197,6 +199,7 @@ function useLoadingTimers() {
  }, []);

  const hideLoading = useCallback((path: string) => {
+    activeLoadsRef.current.delete(path);
    const timer = loadingTimersRef.current.get(path);
    if (timer) {
      clearTimeout(timer);
@@ -209,7 +212,9 @@ function useLoadingTimers() {
    });
  }, []);

-  return { visibleLoadingPaths, showLoading, hideLoading };
+  const isLoading = useCallback((path: string) => activeLoadsRef.current.has(path), []);
+
+  return { visibleLoadingPaths, showLoading, hideLoading, isLoading };
 }

 type TreeLoaderContext = {
@@ -358,7 +363,7 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) {
  const sessionIdRef = useRef(sessionId);
  sessionIdRef.current = sessionId;
  const agentctlStatus = useSessionAgentctl(sessionId);
-  const { visibleLoadingPaths, showLoading, hideLoading } = useLoadingTimers();
+  const { visibleLoadingPaths, showLoading, hideLoading, isLoading } = useLoadingTimers();

  const clearRetryTimer = useCallback(() => {
    if (retryTimerRef.current) {
@@ -448,6 +453,7 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) {
    loadTree,
    showLoading,
    hideLoading,
+    isLoading,
    collapseAll,
  };
 }
@@ -505,6 +511,8 @@ export async function loadNodeChildren(
  treeState: ReturnType<typeof useFileBrowserTree>,
 ) {
  if (node.children && node.children.length > 0) return;
+  // Dedupe in-flight fetches so rapid double-clicks don't issue two WS round-trips.
+  if (treeState.isLoading(node.path)) return;
  treeState.showLoading(node.path);
  try {
    const client = getWebSocketClient();
@@ -522,6 +530,36 @@ export async function loadNodeChildren(
  }
 }

+export type ToggleFolderExpandDeps = {
+  node: FileTreeNode;
+  sessionId: string;
+  treeState: ReturnType<typeof useFileBrowserTree>;
+  setActiveFolderPath: (path: string) => void;
+  loadChildren?: typeof loadNodeChildren;
+};
+
+// Flip expanded synchronously *then* fetch children so the chevron rotates on the first click.
+export async function toggleFolderExpand({
+  node,
+  sessionId,
+  treeState,
+  setActiveFolderPath,
+  loadChildren = loadNodeChildren,
+}: ToggleFolderExpandDeps): Promise<void> {
+  if (!node.is_dir) return;
+  setActiveFolderPath(node.path);
+  const wasExpanded = treeState.expandedPaths.has(node.path);
+  treeState.setExpandedPaths((prev) => {
+    const next = new Set(prev);
+    if (next.has(node.path)) next.delete(node.path);
+    else next.add(node.path);
+    return next;
+  });
+  if (!wasExpanded) {
+    await loadChildren(node, sessionId, treeState);
+  }
+}
+
 /** Fetch and open a file by path. */
 export async function fetchAndOpenFile(
  sessionId: string,
@@ -0,0 +1,108 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import type { FileTreeNode } from "@/lib/types/backend";
+import type {
+  useFileBrowserTree,
+  loadNodeChildren as LoadNodeChildren,
+} from "./file-browser-hooks";
+
+type TreeState = ReturnType<typeof useFileBrowserTree>;
+
+const requestFileTreeMock = vi.fn();
+const getWebSocketClientMock = vi.fn(() => ({}) as unknown);
+
+vi.mock("@/lib/ws/connection", () => ({
+  getWebSocketClient: () => getWebSocketClientMock(),
+}));
+vi.mock("@/lib/ws/workspace-files", () => ({
+  requestFileTree: (...args: unknown[]) => requestFileTreeMock(...args),
+  requestFileContent: vi.fn(),
+  searchWorkspaceFiles: vi.fn(),
+}));
+
+let loadNodeChildren: typeof LoadNodeChildren;
+beforeEach(async () => {
+  vi.resetModules();
+  requestFileTreeMock.mockReset();
+  getWebSocketClientMock.mockReset().mockReturnValue({});
+  ({ loadNodeChildren } = await import("./file-browser-hooks"));
+});
+
+function makeTreeState(opts: { tree?: FileTreeNode | null; loadingPaths?: Set<string> } = {}): {
+  state: TreeState;
+  loadingPaths: Set<string>;
+} {
+  const loadingPaths = opts.loadingPaths ?? new Set<string>();
+  const state = {
+    tree: opts.tree ?? { name: "", path: "", is_dir: true, size: 0, children: [] },
+    setTree: vi.fn(),
+    expandedPaths: new Set<string>(),
+    setExpandedPaths: vi.fn(),
+    visibleRows: [],
+    visibleLoadingPaths: new Set<string>(),
+    isLoadingTree: false,
+    loadState: "loaded",
+    loadError: null,
+    loadTree: vi.fn(),
+    showLoading: vi.fn((p: string) => loadingPaths.add(p)),
+    hideLoading: vi.fn((p: string) => loadingPaths.delete(p)),
+    isLoading: vi.fn((p: string) => loadingPaths.has(p)),
+    collapseAll: vi.fn(),
+  } as unknown as TreeState;
+  return { state, loadingPaths };
+}
+
+const FOLDER: FileTreeNode = { name: "src", path: "src", is_dir: true, size: 0, children: [] };
+
+describe("loadNodeChildren — in-flight dedupe", () => {
+  it("skips duplicate fetches while a load is in flight for the same path", async () => {
+    const { state } = makeTreeState();
+    let resolveFetch: (v: { root: FileTreeNode }) => void = () => {};
+    requestFileTreeMock.mockImplementation(
+      () =>
+        new Promise((resolve) => {
+          resolveFetch = resolve;
+        }),
+    );
+
+    const p1 = loadNodeChildren(FOLDER, "session-1", state);
+    const p2 = loadNodeChildren(FOLDER, "session-1", state);
+    const p3 = loadNodeChildren(FOLDER, "session-1", state);
+
+    // Only the first call should reach the WS request — the dedupe guard
+    // catches the rest before they fire.
+    expect(requestFileTreeMock).toHaveBeenCalledTimes(1);
+    expect(state.showLoading).toHaveBeenCalledTimes(1);
+
+    resolveFetch({ root: { name: "src", path: "src", is_dir: true, size: 0, children: [] } });
+    await Promise.all([p1, p2, p3]);
+
+    expect(state.hideLoading).toHaveBeenCalledTimes(1);
+  });
+
+  it("allows a fresh fetch once the previous one has finished", async () => {
+    const { state } = makeTreeState();
+    requestFileTreeMock.mockResolvedValue({
+      root: { name: "src", path: "src", is_dir: true, size: 0, children: [] },
+    });
+
+    await loadNodeChildren(FOLDER, "session-1", state);
+    await loadNodeChildren(FOLDER, "session-1", state);
+
+    // First call clears showLoading/hideLoading state, so the second one
+    // (still operating on a node with no children) issues its own fetch.
+    expect(requestFileTreeMock).toHaveBeenCalledTimes(2);
+  });
+
+  it("short-circuits when the node already has children", async () => {
+    const { state } = makeTreeState();
+    const loaded: FileTreeNode = {
+      ...FOLDER,
+      children: [{ name: "a.ts", path: "src/a.ts", is_dir: false, size: 0 }],
+    };
+
+    await loadNodeChildren(loaded, "session-1", state);
+
+    expect(requestFileTreeMock).not.toHaveBeenCalled();
+    expect(state.showLoading).not.toHaveBeenCalled();
+  });
+});
@@ -0,0 +1,129 @@
+import { describe, it, expect, vi } from "vitest";
+import { toggleFolderExpand } from "./file-browser-hooks";
+import type { FileTreeNode } from "@/lib/types/backend";
+import type { useFileBrowserTree } from "./file-browser-hooks";
+
+type TreeState = ReturnType<typeof useFileBrowserTree>;
+
+function makeTreeState(initialExpanded: Set<string> = new Set()): {
+  state: TreeState;
+  expanded: { current: Set<string> };
+} {
+  const expanded = { current: initialExpanded };
+  const setExpandedPaths = vi.fn((updater: unknown) => {
+    if (typeof updater === "function") {
+      expanded.current = (updater as (p: Set<string>) => Set<string>)(expanded.current);
+    } else {
+      expanded.current = updater as Set<string>;
+    }
+  });
+  const state = {
+    tree: null,
+    setTree: vi.fn(),
+    get expandedPaths() {
+      return expanded.current as ReadonlySet<string>;
+    },
+    setExpandedPaths,
+    visibleRows: [],
+    visibleLoadingPaths: new Set<string>(),
+    isLoadingTree: false,
+    loadState: "loaded",
+    loadError: null,
+    loadTree: vi.fn(),
+    showLoading: vi.fn(),
+    hideLoading: vi.fn(),
+    isLoading: vi.fn(() => false),
+    collapseAll: vi.fn(),
+  } as unknown as TreeState;
+  return { state, expanded };
+}
+
+const FOLDER: FileTreeNode = { name: "src", path: "src", is_dir: true, size: 0 };
+const FILE: FileTreeNode = { name: "a.ts", path: "src/a.ts", is_dir: false, size: 0 };
+
+describe("toggleFolderExpand", () => {
+  it("expands a collapsed folder synchronously, before the async load resolves", async () => {
+    const { state, expanded } = makeTreeState();
+    let resolveLoad: () => void = () => {};
+    const loadChildren = vi.fn(
+      () =>
+        new Promise<void>((resolve) => {
+          resolveLoad = resolve;
+        }),
+    );
+    const setActiveFolderPath = vi.fn();
+
+    const promise = toggleFolderExpand({
+      node: FOLDER,
+      sessionId: "session-1",
+      treeState: state,
+      setActiveFolderPath,
+      loadChildren,
+    });
+
+    // The expanded set must already include the folder even though loadChildren
+    // has not resolved yet — that is the regression we are guarding against.
+    expect(expanded.current.has("src")).toBe(true);
+    expect(setActiveFolderPath).toHaveBeenCalledWith("src");
+    expect(loadChildren).toHaveBeenCalledTimes(1);
+
+    resolveLoad();
+    await promise;
+    expect(expanded.current.has("src")).toBe(true);
+  });
+
+  it("collapses an already-expanded folder and skips the async load", async () => {
+    const { state, expanded } = makeTreeState(new Set(["src"]));
+    const loadChildren = vi.fn(() => Promise.resolve());
+
+    await toggleFolderExpand({
+      node: FOLDER,
+      sessionId: "session-1",
+      treeState: state,
+      setActiveFolderPath: vi.fn(),
+      loadChildren,
+    });
+
+    expect(expanded.current.has("src")).toBe(false);
+    expect(loadChildren).not.toHaveBeenCalled();
+  });
+
+  it("ignores file nodes", async () => {
+    const { state, expanded } = makeTreeState();
+    const loadChildren = vi.fn(() => Promise.resolve());
+    const setActiveFolderPath = vi.fn();
+
+    await toggleFolderExpand({
+      node: FILE,
+      sessionId: "session-1",
+      treeState: state,
+      setActiveFolderPath,
+      loadChildren,
+    });
+
+    expect(expanded.current.size).toBe(0);
+    expect(setActiveFolderPath).not.toHaveBeenCalled();
+    expect(loadChildren).not.toHaveBeenCalled();
+  });
+
+  it("uses functional setState so the latest expanded set is mutated", async () => {
+    const { state, expanded } = makeTreeState();
+    const loadChildren = vi.fn(() => Promise.resolve());
+
+    // Simulate another path being added while the toggle is in flight.
+    const promise = toggleFolderExpand({
+      node: FOLDER,
+      sessionId: "session-1",
+      treeState: state,
+      setActiveFolderPath: vi.fn(),
+      loadChildren,
+    });
+    // The toggle ran setExpandedPaths via a functional setter — adding another
+    // path concurrently must not be lost.
+    state.setExpandedPaths((prev) => new Set(prev).add("docs"));
+    await promise;
+
+    expect(expanded.current.has("src")).toBe(true);
+    expect(expanded.current.has("docs")).toBe(true);
+  });
+});
@@ -21,7 +21,7 @@ import {
  useFileBrowserSearch,
  useFileBrowserTree,
  useScrollPersistence,
-  loadNodeChildren,
+  toggleFolderExpand,
  fetchAndOpenFile,
 } from "./file-browser-hooks";
 import { getVisiblePaths, moveNodesInTree, computeMoveTargets } from "./file-tree-utils";
@@ -126,18 +126,7 @@ function useFileBrowserHandlers(
  );

  const toggleExpand = useCallback(
-    async (node: FileTreeNode) => {
-      if (!node.is_dir) return;
-      setActiveFolderPath(node.path);
-      const newExpanded = new Set(treeState.expandedPaths);
-      if (newExpanded.has(node.path)) {
-        newExpanded.delete(node.path);
-      } else {
-        await loadNodeChildren(node, sessionId, treeState);
-        newExpanded.add(node.path);
-      }
-      treeState.setExpandedPaths(newExpanded);
-    },
+    (node: FileTreeNode) => toggleFolderExpand({ node, sessionId, treeState, setActiveFolderPath }),
    [treeState, sessionId],
  );