From 33a378a8dc6caeb36e352fbb60a69774ce437c0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Thu, 21 May 2026 14:01:31 +0100 Subject: [PATCH] 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 * 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 * 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 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- .../web/components/task/file-browser-hooks.ts | 42 +++++- .../task/file-browser-load-children.test.ts | 108 +++++++++++++++ .../task/file-browser-toggle-expand.test.ts | 129 ++++++++++++++++++ apps/web/components/task/file-browser.tsx | 15 +- 4 files changed, 279 insertions(+), 15 deletions(-) create mode 100644 apps/web/components/task/file-browser-load-children.test.ts create mode 100644 apps/web/components/task/file-browser-toggle-expand.test.ts diff --git a/apps/web/components/task/file-browser-hooks.ts b/apps/web/components/task/file-browser-hooks.ts index 736e2b2e1f..8fd57f494e 100644 --- a/apps/web/components/task/file-browser-hooks.ts +++ b/apps/web/components/task/file-browser-hooks.ts @@ -186,9 +186,11 @@ function applyFileChanges(ctx: { function useLoadingTimers() { const loadingTimersRef = useRef>(new Map()); + const activeLoadsRef = useRef>(new Set()); const [visibleLoadingPaths, setVisibleLoadingPaths] = useState>(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, ) { 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; + 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 { + 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, diff --git a/apps/web/components/task/file-browser-load-children.test.ts b/apps/web/components/task/file-browser-load-children.test.ts new file mode 100644 index 0000000000..71cc6fabb3 --- /dev/null +++ b/apps/web/components/task/file-browser-load-children.test.ts @@ -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; + +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 } = {}): { + state: TreeState; + loadingPaths: Set; +} { + const loadingPaths = opts.loadingPaths ?? new Set(); + const state = { + tree: opts.tree ?? { name: "", path: "", is_dir: true, size: 0, children: [] }, + setTree: vi.fn(), + expandedPaths: new Set(), + setExpandedPaths: vi.fn(), + visibleRows: [], + visibleLoadingPaths: new Set(), + 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(); + }); +}); diff --git a/apps/web/components/task/file-browser-toggle-expand.test.ts b/apps/web/components/task/file-browser-toggle-expand.test.ts new file mode 100644 index 0000000000..312d5a0f2b --- /dev/null +++ b/apps/web/components/task/file-browser-toggle-expand.test.ts @@ -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; + +function makeTreeState(initialExpanded: Set = new Set()): { + state: TreeState; + expanded: { current: Set }; +} { + const expanded = { current: initialExpanded }; + const setExpandedPaths = vi.fn((updater: unknown) => { + if (typeof updater === "function") { + expanded.current = (updater as (p: Set) => Set)(expanded.current); + } else { + expanded.current = updater as Set; + } + }); + const state = { + tree: null, + setTree: vi.fn(), + get expandedPaths() { + return expanded.current as ReadonlySet; + }, + setExpandedPaths, + visibleRows: [], + visibleLoadingPaths: new Set(), + 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((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); + }); +}); diff --git a/apps/web/components/task/file-browser.tsx b/apps/web/components/task/file-browser.tsx index 01b40a8de7..35d33cd065 100644 --- a/apps/web/components/task/file-browser.tsx +++ b/apps/web/components/task/file-browser.tsx @@ -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], );