From bab9e641faa3bb64302a721bb1df936c63369d4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 6 Sep 2026 13:30:24 +0100 Subject: [PATCH] perf: isolate task surface rendering (#3226) --- .../github/pr-task-icon.render.test.tsx | 7 +- apps/web/components/github/pr-task-icon.tsx | 7 +- .../web/components/task/file-browser-hooks.ts | 44 ++++- .../components/task/file-browser-parts.tsx | 174 ++++++++++++------ .../file-browser-render-identity.test.tsx | 89 +++++++++ apps/web/components/task/file-browser.tsx | 73 +++++--- .../web/components/task/file-context-menu.tsx | 118 ++++-------- .../components/task/file-delete-action.tsx | 60 ++++++ .../components/task/file-tree-row-props.ts | 84 +++++++++ .../task-session-sidebar-selection.test.ts | 40 +++- .../task/task-session-sidebar-selection.tsx | 8 +- ...task-switcher-row-render-identity.test.tsx | 42 +++++ .../task/task-top-bar-plugin-actions.test.tsx | 23 +++ .../task/task-top-bar-plugin-actions.tsx | 32 +++- apps/web/e2e/pages/file-tree-page.ts | 10 + apps/web/e2e/pages/session-page.ts | 10 + .../large-file-tree-virtualization-helpers.ts | 60 ++++++ .../large-file-tree-virtualization.spec.ts | 84 +++++++++ ...ile-large-file-tree-virtualization.spec.ts | 62 +++++++ .../use-task-pr-tooltip-hydration.test.tsx | 11 ++ .../github/use-task-pr-tooltip-hydration.ts | 23 ++- .../task-surface-render-isolation/plan.md | 174 ++++++++++++++++++ .../task-01-stabilize-file-tree-rows.md | 62 +++++++ .../task-02-virtualize-file-tree.md | 68 +++++++ .../task-03-stabilize-sidebar-rows.md | 63 +++++++ .../task-04-stabilize-contribution-props.md | 66 +++++++ docs/specs/ui/README.md | 1 + .../task-surface-render-isolation.md | 164 +++++++++++++++++ 28 files changed, 1475 insertions(+), 184 deletions(-) create mode 100644 apps/web/components/task/file-browser-render-identity.test.tsx create mode 100644 apps/web/components/task/file-delete-action.tsx create mode 100644 apps/web/components/task/file-tree-row-props.ts create mode 100644 apps/web/components/task/task-switcher-row-render-identity.test.tsx create mode 100644 apps/web/e2e/tests/task/large-file-tree-virtualization-helpers.ts create mode 100644 apps/web/e2e/tests/task/large-file-tree-virtualization.spec.ts create mode 100644 apps/web/e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts create mode 100644 docs/plans/task-surface-render-isolation/plan.md create mode 100644 docs/plans/task-surface-render-isolation/task-01-stabilize-file-tree-rows.md create mode 100644 docs/plans/task-surface-render-isolation/task-02-virtualize-file-tree.md create mode 100644 docs/plans/task-surface-render-isolation/task-03-stabilize-sidebar-rows.md create mode 100644 docs/plans/task-surface-render-isolation/task-04-stabilize-contribution-props.md create mode 100644 docs/specs/ui/system-design/task-surface-render-isolation.md diff --git a/apps/web/components/github/pr-task-icon.render.test.tsx b/apps/web/components/github/pr-task-icon.render.test.tsx index bb2e5b91e4..3f2eb87e13 100644 --- a/apps/web/components/github/pr-task-icon.render.test.tsx +++ b/apps/web/components/github/pr-task-icon.render.test.tsx @@ -4,7 +4,7 @@ import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-libra import { TooltipProvider } from "@kandev/ui/tooltip"; import { StateProvider } from "@/components/state-provider"; import { TaskContributionIcons } from "@/components/task/task-contribution-icons"; -import { PRTaskIcon } from "./pr-task-icon"; +import { normalizeTaskPRs, PRTaskIcon } from "./pr-task-icon"; import type { AppState } from "@/lib/state/store"; import type { TaskPR } from "@/lib/types/github"; import type { TaskCIAutomationOptions } from "@/lib/types/github"; @@ -100,6 +100,11 @@ beforeEach(() => { afterEach(() => cleanup()); describe("PRTaskIcon corrupted store entry", () => { + it("reuses one empty list for missing or malformed PR data", () => { + expect(normalizeTaskPRs(undefined)).toBe(normalizeTaskPRs(null)); + expect(normalizeTaskPRs({})).toBe(normalizeTaskPRs([])); + }); + // Regression: an upstream payload (partial hydration, WS reorder, etc.) once // landed in taskPRs.byTaskId[TASK_ID] as a non-array truthy value. The // length-based guards then fell through into MultiPRIcon, where for-of diff --git a/apps/web/components/github/pr-task-icon.tsx b/apps/web/components/github/pr-task-icon.tsx index 5e301548fa..8d62c4967f 100644 --- a/apps/web/components/github/pr-task-icon.tsx +++ b/apps/web/components/github/pr-task-icon.tsx @@ -39,6 +39,7 @@ const SKY_400 = CHANGE_REQUEST_STATUS_COLORS.review; const EMERALD_400 = CHANGE_REQUEST_STATUS_COLORS.ready; const QUEUED = CHANGE_REQUEST_STATUS_COLORS.queued; const GREEN_500 = CHANGE_REQUEST_STATUS_COLORS.passing; +const EMPTY_PRS: TaskPR[] = []; /** Maps the task-level PR projection to the same visual language as live PRs. */ export function getPRAggregateStatusColor(state: string | null | undefined): string { @@ -253,7 +254,7 @@ export function pickDefaultPR(prs: TaskPR[]): TaskPR | null { export function PRTaskIcon({ taskId, prInfo }: { taskId: string; prInfo?: TaskPRInfo }) { const prs = useAppStore((state) => getTaskPRsForCurrentWorkspace(state, taskId)); const hydration = useTaskPRTooltipHydration(taskId, { includeAutomation: true }); - const fullPRs = Array.isArray(prs) && prs.length > 0 ? prs : []; + const fullPRs = normalizeTaskPRs(prs); // Defensive: an upstream payload may briefly seed byTaskId[taskId] with a // non-array value (e.g. an empty object from a partial hydration). Bail @@ -271,6 +272,10 @@ export function PRTaskIcon({ taskId, prInfo }: { taskId: string; prInfo?: TaskPR ); } +export function normalizeTaskPRs(prs: unknown): TaskPR[] { + return Array.isArray(prs) && prs.length > 0 ? prs : EMPTY_PRS; +} + type TaskPRIconPresentation = { hasFullData: boolean; singlePR: TaskPR | null; diff --git a/apps/web/components/task/file-browser-hooks.ts b/apps/web/components/task/file-browser-hooks.ts index 30912b60bf..2d7dd9d005 100644 --- a/apps/web/components/task/file-browser-hooks.ts +++ b/apps/web/components/task/file-browser-hooks.ts @@ -1,6 +1,7 @@ "use client"; import { useEffect, useState, useCallback, useRef, useMemo } from "react"; +import type React from "react"; import { getWebSocketClient } from "@/lib/ws/connection"; import { requestFileTree, searchWorkspaceFiles } from "@/lib/ws/workspace-files"; import type { FileTreeNode } from "@/lib/types/backend"; @@ -25,6 +26,45 @@ export type FileBrowserRow = VisibleRow; export type LoadState = "loading" | "waiting" | "loaded" | "manual" | "error"; +type FileBrowserTreeResult = { + tree: FileTreeNode | null; + setTree: React.Dispatch>; + expandedPaths: ReadonlySet; + setExpandedPaths: React.Dispatch>>; + visibleRows: FileBrowserRow[]; + visibleLoadingPaths: Set; + isLoadingTree: boolean; + loadState: LoadState; + loadError: string | null; + loadTree: ReturnType; + showLoading: (path: string) => void; + hideLoading: (path: string) => void; + isLoading: (path: string) => boolean; + collapseAll: () => void; +}; + +function useMemoizedFileBrowserTreeResult(result: FileBrowserTreeResult) { + return useMemo( + () => result, + [ + result.tree, + result.setTree, + result.expandedPaths, + result.setExpandedPaths, + result.visibleRows, + result.visibleLoadingPaths, + result.isLoadingTree, + result.loadState, + result.loadError, + result.loadTree, + result.showLoading, + result.hideLoading, + result.isLoading, + result.collapseAll, + ], + ); +} + /** Hook encapsulating file search state and handlers. */ export function useFileBrowserSearch(sessionId: string) { const [isSearchActive, setIsSearchActive] = useState(false); @@ -450,7 +490,7 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) { setFilesPanelExpandedPaths(effectiveResetKey, Array.from(expandedPaths)); }, [expandedPaths, effectiveResetKey, isLoadingTree]); useFileChangeSubscription({ sessionIdRef, expandedPathsRef, setTree, setLoadState }); - return { + return useMemoizedFileBrowserTreeResult({ tree, setTree, expandedPaths, @@ -465,7 +505,7 @@ export function useFileBrowserTree(sessionId: string, resetKey?: string) { hideLoading, isLoading, collapseAll: treeApi.collapseAll, - }; + }); } export { diff --git a/apps/web/components/task/file-browser-parts.tsx b/apps/web/components/task/file-browser-parts.tsx index 433a0a8c70..d59b09e1c5 100644 --- a/apps/web/components/task/file-browser-parts.tsx +++ b/apps/web/components/task/file-browser-parts.tsx @@ -1,6 +1,7 @@ "use client"; import React from "react"; +import { useVirtualizer } from "@tanstack/react-virtual"; import { IconChevronRight, IconChevronDown, @@ -23,6 +24,7 @@ import { InlineConfirmActions } from "@/components/confirmation/inline-confirm-a import type { FileTreeNode } from "@/lib/types/backend"; import type { FileInfo } from "@/lib/state/store"; import type { FileBrowserRow } from "./file-browser-hooks"; +import { areTreeNodeRowPropsEqual, type TreeNodeRowProps } from "./file-tree-row-props"; import { InlineFileInput } from "./inline-file-input"; import { renderSessionOrLoadState } from "./file-browser-load-state"; import { @@ -47,35 +49,6 @@ export function shouldShowFileTreeTouchActions(isMobile: boolean, isFinePointer: return isMobile || !isFinePointer; } -type TreeNodeRowProps = { - row: FileBrowserRow; - activeFolderPath: string; - activeFilePath?: string | null; - visibleLoadingPaths: Set; - fileStatuses: Map; - tree: FileTreeNode | null; - onToggleExpand: (node: FileTreeNode) => void; - onOpenFile: (path: string) => void; - onDeleteFile?: (path: string) => Promise; - onRenameFile?: (oldPath: string, newPath: string) => Promise; - onDownloadFile?: (path: string) => Promise; - onUploadFilesHere?: (path: string) => void; - setTree: React.Dispatch>; - isSelectedFn?: (path: string) => boolean; - onSelect?: (path: string, e: React.MouseEvent) => boolean; - isDragging?: boolean; - dragOverPath?: string | null; - onDragStart?: (path: string, e: React.DragEvent) => void; - onDragEnd?: () => void; - onDragOver?: (path: string, e: React.DragEvent) => void; - onDragLeave?: (e: React.DragEvent) => void; - onDrop?: (targetPath: string, e: React.DragEvent) => void; - selectedCount?: number; - selectedPaths?: Set; - showTouchActions?: boolean; - onAddToChatContext?: (node: FileTreeNode) => void; -}; - function treeNodePaddingLeft(depth: number, isDir: boolean): string { return `${depth * 12 + 8 + (isDir ? 0 : 20)}px`; } @@ -231,7 +204,7 @@ export function FileTreeNodeTouchActions({ ); } -export function TreeNodeItem(props: TreeNodeRowProps) { +export const TreeNodeItem = React.memo(function TreeNodeItem(props: TreeNodeRowProps) { const { row, activeFolderPath, activeFilePath, visibleLoadingPaths } = props; const { fileStatuses, @@ -252,7 +225,7 @@ export function TreeNodeItem(props: TreeNodeRowProps) { const isActive = !node.is_dir && activeFilePath === node.path; const isActiveFolder = node.is_dir && activeFolderPath === node.path; const gitStatus = node.is_dir ? undefined : fileStatuses.get(node.path); - const rename = useFileRename(node, tree, setTree, onRenameFile); + const rename = useFileRename(node, tree, setTree, onRenameFile, props.treeRef); const isSelected = props.isSelectedFn?.(node.path) ?? false; const isDropTarget = node.is_dir && props.dragOverPath === node.path; const rowAnchorRef = React.useRef(null); @@ -316,6 +289,7 @@ export function TreeNodeItem(props: TreeNodeRowProps) { ); -} +}, areTreeNodeRowPropsEqual); type SearchResultsListProps = { searchResults: string[] | null; @@ -437,6 +411,8 @@ type FileBrowserContentAreaProps = { creatingInPath: string | null; fileStatuses: Map; visibleRows: FileBrowserRow[]; + scrollViewportRef?: React.RefObject; + treeRef?: React.RefObject; activeFolderPath: string; activeFilePath?: string | null; visibleLoadingPaths: Set; @@ -465,7 +441,11 @@ type FileBrowserContentAreaProps = { onAddToChatContext?: (node: FileTreeNode) => void; }; -function rowToItemProps(props: FileBrowserContentAreaProps, row: FileBrowserRow): TreeNodeRowProps { +function rowToItemProps( + props: FileBrowserContentAreaProps, + row: FileBrowserRow, + treeRef: React.RefObject | undefined = props.treeRef, +): TreeNodeRowProps { return { row, activeFolderPath: props.activeFolderPath, @@ -473,6 +453,7 @@ function rowToItemProps(props: FileBrowserContentAreaProps, row: FileBrowserRow) visibleLoadingPaths: props.visibleLoadingPaths, fileStatuses: props.fileStatuses, tree: props.tree, + treeRef, onToggleExpand: props.onToggleExpand, onOpenFile: props.onOpenFile, onDeleteFile: props.onDeleteFile, @@ -496,30 +477,115 @@ function rowToItemProps(props: FileBrowserContentAreaProps, row: FileBrowserRow) }; } +type FileTreeVirtualRow = + | { type: "node"; row: FileBrowserRow } + | { type: "create"; parentPath: string; depth: number }; + +function scheduleVirtualRowReveal(reveal: () => void): () => void { + let settleFrame: number | null = null; + const frame = requestAnimationFrame(() => { + reveal(); + settleFrame = requestAnimationFrame(reveal); + }); + return () => { + cancelAnimationFrame(frame); + if (settleFrame !== null) cancelAnimationFrame(settleFrame); + }; +} + function FileTreeView(props: FileBrowserContentAreaProps) { + if (!props.tree) return null; + return ; +} + +function VirtualizedFileTreeView(props: FileBrowserContentAreaProps) { const { tree, visibleRows, creatingInPath, onCreateFileSubmit, onCancelCreate } = props; - if (!tree) return null; + const internalScrollViewportRef = React.useRef(null); + const scrollViewportRef = props.scrollViewportRef ?? internalScrollViewportRef; + const treeRef = React.useRef(tree); + treeRef.current = tree; + const virtualRows = React.useMemo(() => { + const rows: FileTreeVirtualRow[] = []; + if (creatingInPath === "") rows.push({ type: "create", parentPath: "", depth: 0 }); + for (const row of visibleRows) { + rows.push({ type: "node", row }); + if (creatingInPath === row.path && row.isDir && row.isExpanded) { + rows.push({ type: "create", parentPath: row.path, depth: row.depth + 1 }); + } + } + return rows; + }, [creatingInPath, visibleRows]); + const virtualizer = useVirtualizer({ + count: virtualRows.length, + getScrollElement: () => scrollViewportRef.current, + estimateSize: (index) => { + const row = virtualRows[index]; + if (row?.type === "create") return 44; + return props.showTouchActions ? 48 : 28; + }, + getItemKey: (index) => { + const row = virtualRows[index]; + return row?.type === "create" ? `create:${row.parentPath}` : (row?.row.path ?? index); + }, + overscan: 5, + }); + + const revealedActiveFileRef = React.useRef(null); + React.useEffect(() => { + if (revealedActiveFileRef.current !== props.activeFilePath) { + revealedActiveFileRef.current = null; + } + if (!props.activeFilePath || revealedActiveFileRef.current === props.activeFilePath) return; + const index = virtualRows.findIndex( + (row) => row.type === "node" && row.row.path === props.activeFilePath, + ); + if (index < 0) return; + revealedActiveFileRef.current = props.activeFilePath; + return scheduleVirtualRowReveal(() => virtualizer.scrollToIndex(index, { align: "auto" })); + }, [props.activeFilePath, virtualRows, virtualizer]); + + const revealedCreatePathRef = React.useRef(null); + React.useEffect(() => { + if (revealedCreatePathRef.current !== creatingInPath) { + revealedCreatePathRef.current = null; + } + if (creatingInPath === null || revealedCreatePathRef.current === creatingInPath) return; + const index = virtualRows.findIndex( + (row) => row.type === "create" && row.parentPath === creatingInPath, + ); + if (index < 0) return; + revealedCreatePathRef.current = creatingInPath; + return scheduleVirtualRowReveal(() => virtualizer.scrollToIndex(index, { align: "auto" })); + }, [creatingInPath, virtualRows, virtualizer]); + return ( -
- {creatingInPath === "" && ( - onCreateFileSubmit("", name)} - onCancel={onCancelCreate} - /> - )} - {visibleRows.map((row) => ( - - - {creatingInPath === row.path && row.isDir && row.isExpanded && ( - onCreateFileSubmit(row.path, name)} - onCancel={onCancelCreate} - /> - )} - - ))} +
+ {virtualizer.getVirtualItems().map((virtualItem) => { + const row = virtualRows[virtualItem.index]; + if (!row) return null; + return ( +
+ {row.type === "node" ? ( + + ) : ( + onCreateFileSubmit(row.parentPath, name)} + onCancel={onCancelCreate} + /> + )} +
+ ); + })}
); } diff --git a/apps/web/components/task/file-browser-render-identity.test.tsx b/apps/web/components/task/file-browser-render-identity.test.tsx new file mode 100644 index 0000000000..7aa46f13f1 --- /dev/null +++ b/apps/web/components/task/file-browser-render-identity.test.tsx @@ -0,0 +1,89 @@ +import type { ReactNode } from "react"; +import { cleanup, render, renderHook } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { FileTreeNode } from "@/lib/types/backend"; +import type { FileBrowserRow } from "./file-browser-hooks"; + +const fileContextMenuRender = vi.hoisted(() => vi.fn()); + +vi.mock("./file-context-menu", () => ({ + FileContextMenu: ({ children }: { children: ReactNode }) => { + fileContextMenuRender(); + return <>{children}; + }, + useFileDeleteAction: () => null, + useFileRename: () => ({ + isRenaming: false, + renameValue: "", + setRenameValue: vi.fn(), + handleStartRename: vi.fn(), + handleConfirmRename: vi.fn(), + handleRenameKeyDown: vi.fn(), + }), + TreeNodeName: ({ node }: { node: FileTreeNode }) => {node.name}, + getGitStatusTextClass: () => "", +})); + +vi.mock("@/hooks/domains/session/use-session-agentctl", () => ({ + useSessionAgentctl: () => ({ isReady: false }), +})); + +vi.mock("@/lib/ws/connection", () => ({ + getWebSocketClient: () => null, +})); + +import { TreeNodeItem } from "./file-browser-parts"; +import { useFileBrowserTree } from "./file-browser-hooks"; + +const NODE: FileTreeNode = { name: "README.md", path: "README.md", is_dir: false, size: 0 }; +const TREE: FileTreeNode = { name: "root", path: "", is_dir: true, size: 0 }; +const ROW: FileBrowserRow = { + node: NODE, + chainRoot: NODE, + displayName: NODE.name, + path: NODE.path, + depth: 0, + isExpanded: false, + isDir: false, +}; + +function rowProps(row: FileBrowserRow = ROW): Parameters[0] { + return { + row, + activeFolderPath: "", + visibleLoadingPaths: new Set(), + fileStatuses: new Map(), + tree: null, + onToggleExpand: vi.fn(), + onOpenFile: vi.fn(), + setTree: vi.fn(), + showTouchActions: false, + }; +} + +afterEach(() => { + cleanup(); + fileContextMenuRender.mockReset(); +}); + +describe("TreeNodeItem render isolation", () => { + it("skips an unrelated owner render when the row inputs remain equal", () => { + const stableProps = { ...rowProps(), tree: TREE, treeRef: { current: TREE } }; + const { rerender } = render(); + + rerender(); + + expect(fileContextMenuRender).toHaveBeenCalledTimes(1); + }); +}); + +describe("useFileBrowserTree render identity", () => { + it("keeps its aggregate result stable during an unrelated rerender", () => { + const { result, rerender } = renderHook(() => useFileBrowserTree("session-1", "reset-1")); + const firstResult = result.current; + + rerender(); + + expect(result.current).toBe(firstResult); + }); +}); diff --git a/apps/web/components/task/file-browser.tsx b/apps/web/components/task/file-browser.tsx index 0c516d9861..818036c551 100644 --- a/apps/web/components/task/file-browser.tsx +++ b/apps/web/components/task/file-browser.tsx @@ -73,6 +73,10 @@ function useFileBrowserHandlers( const [creatingInPath, setCreatingInPath] = useState(null); const [activeFolderPath, setActiveFolderPath] = useState(""); const openFileAbortRef = useRef(null); + const treeStateRef = useRef(treeState); + treeStateRef.current = treeState; + + const { expandedPaths, setExpandedPaths, setTree } = treeState; useLayoutEffect( () => () => { @@ -83,32 +87,38 @@ function useFileBrowserHandlers( ); const handleStartCreate = useCallback(() => { - if (activeFolderPath && !treeState.expandedPaths.has(activeFolderPath)) { - treeState.setExpandedPaths((prev) => new Set(prev).add(activeFolderPath)); + if (activeFolderPath && !expandedPaths.has(activeFolderPath)) { + setExpandedPaths((prev) => new Set(prev).add(activeFolderPath)); } setCreatingInPath(activeFolderPath); - }, [activeFolderPath, treeState]); + }, [activeFolderPath, expandedPaths, setExpandedPaths]); const handleCreateFileSubmit = useCallback( (parentPath: string, name: string) => { setCreatingInPath(null); const newPath = parentPath ? `${parentPath}/${name}` : name; const newNode: FileTreeNode = { name, path: newPath, is_dir: false, size: 0 }; - treeState.setTree((prev) => (prev ? insertNodeInTree(prev, parentPath, newNode) : prev)); + setTree((prev) => (prev ? insertNodeInTree(prev, parentPath, newNode) : prev)); onCreateFile?.(newPath) .then((ok) => { - if (!ok) treeState.setTree((prev) => (prev ? removeNodeFromTree(prev, newPath) : prev)); + if (!ok) setTree((prev) => (prev ? removeNodeFromTree(prev, newPath) : prev)); }) .catch(() => { - treeState.setTree((prev) => (prev ? removeNodeFromTree(prev, newPath) : prev)); + setTree((prev) => (prev ? removeNodeFromTree(prev, newPath) : prev)); }); }, - [onCreateFile, treeState], + [onCreateFile, setTree], ); const toggleExpand = useCallback( - (node: FileTreeNode) => toggleFolderExpand({ node, sessionId, treeState, setActiveFolderPath }), - [treeState, sessionId], + (node: FileTreeNode) => + toggleFolderExpand({ + node, + sessionId, + treeState: treeStateRef.current, + setActiveFolderPath, + }), + [sessionId], ); const openFileByPath = useCallback( @@ -213,6 +223,8 @@ function useDragAndDrop( const [isDragging, setIsDragging] = useState(false); const [dragOverPath, setDragOverPath] = useState(null); const dragPathsRef = useRef([]); + const treeStateRef = useRef(treeState); + treeStateRef.current = treeState; const handleDragStart = useCallback( (path: string, e: React.DragEvent) => { @@ -273,12 +285,18 @@ function useDragAndDrop( const sources = dragPathsRef.current; if (sources.length === 0 || isDropInvalid(sources, targetPath)) return; executeMoveFiles( - { sources, targetPath, treeState, setSelectedPaths, onRenameFile }, + { + sources, + targetPath, + treeState: treeStateRef.current, + setSelectedPaths, + onRenameFile, + }, toast, t, ); }, - [onRenameFile, treeState, setSelectedPaths, t, toast], + [onRenameFile, setSelectedPaths, t, toast], ); return { @@ -312,14 +330,20 @@ function useSelectionInteractions( ); useKeyboardShortcuts(containerRef, multiSelect.clearSelection, multiSelect.selectAll); + const treeStateRef = useRef(treeState); + treeStateRef.current = treeState; + const loadChildren = useCallback( + (node: FileTreeNode, shouldApply?: () => boolean) => + loadNodeChildren(node, sessionId, treeStateRef.current, { force: true, shouldApply }), + [sessionId], + ); useFileTreeReveal({ activeFilePath, sessionId, tree: treeState.tree, setExpandedPaths: treeState.setExpandedPaths, isLoading: treeState.isLoading, - loadChildren: (node, shouldApply) => - loadNodeChildren(node, sessionId, treeState, { force: true, shouldApply }), + loadChildren, }); const handleClickOutside = useCallback( @@ -336,7 +360,7 @@ function useSelectionInteractions( return; multiSelect.clearSelection(); }, - [multiSelect], + [multiSelect.selectedPaths, multiSelect.clearSelection], ); return { multiSelect, dnd, handleClickOutside }; @@ -481,11 +505,14 @@ function FileBrowserTreeContent({ showTouchActions: boolean; }) { const { search, isSessionFailed, sessionError, treeState, fileStatuses } = data; + const scrollViewportRef = useRef(null); return ( div]:!block [&>div]:!min-w-0 [&>div]:!w-full", @@ -503,6 +530,7 @@ function FileBrowserTreeContent({ creatingInPath={handlers.creatingInPath} fileStatuses={fileStatuses} visibleRows={treeState.visibleRows} + scrollViewportRef={scrollViewportRef} activeFolderPath={handlers.activeFolderPath} activeFilePath={activeFilePath} visibleLoadingPaths={treeState.visibleLoadingPaths} @@ -562,15 +590,12 @@ export function FileBrowser({ containerRef, }); const { openFolder, copied, copyPath, search, treeState, fullPath, displayPath } = data; - const upload = useFileUploadEntryPoints(sessionId); + const { openPicker, uploads, elements } = useFileUploadEntryPoints(sessionId); const handleToolbarUpload = useCallback( - (mode: "files" | "folder") => upload.openPicker(mode, handlers.activeFolderPath ?? ""), - [upload, handlers.activeFolderPath], - ); - const handleUploadHere = useCallback( - (path: string) => upload.openPicker("files", path), - [upload], + (mode: "files" | "folder") => openPicker(mode, handlers.activeFolderPath ?? ""), + [openPicker, handlers.activeFolderPath], ); + const handleUploadHere = useCallback((path: string) => openPicker("files", path), [openPicker]); return (
- - {upload.elements} + + {elements}
); diff --git a/apps/web/components/task/file-context-menu.tsx b/apps/web/components/task/file-context-menu.tsx index 212b7fa775..3566adae20 100644 --- a/apps/web/components/task/file-context-menu.tsx +++ b/apps/web/components/task/file-context-menu.tsx @@ -19,38 +19,17 @@ import { useTranslation } from "react-i18next"; import type { FileTreeNode } from "@/lib/types/backend"; import { FileContextMenuItems } from "./file-context-menu-items"; import type { FileInfo } from "@/lib/state/store"; -import { - removeNodeFromTree, - renameNodeInTree, - treeContainsPath, - countFilesInTree, -} from "./file-tree-utils"; +import { removeNodeFromTree, renameNodeInTree, treeContainsPath } from "./file-tree-utils"; +import { createFileDeleteAction, type FileDeleteAction } from "./file-delete-action"; import { OpenInEditorMenuItems, canOpenNodeInEditor, useFileTreeEditorActions, } from "./file-tree-editor-menu"; -import { - DeleteConfirmDialog, - DeleteFileDescription, - DeleteFolderDescription, -} from "./file-delete-confirmation"; +import { DeleteConfirmDialog } from "./file-delete-confirmation"; type GitFileStatus = FileInfo["status"] | undefined; -export type FileDeleteAction = { - isBulk: boolean; - selectedCount: number; - confirming: boolean; - title: string; - label: string; - cancelLabel: string; - description: React.ReactNode; - onDelete: () => void; - onCancel: () => void; - onConfirm: () => void; -}; - const FileDeleteActionContext = React.createContext(null); export function useFileDeleteAction() { @@ -120,6 +99,7 @@ function attachAnchorRef( function useFileContextMenuDelete({ tree, + treeRef, setTree, node, isBulk, @@ -127,6 +107,7 @@ function useFileContextMenuDelete({ selectedPaths, }: { tree: FileTreeNode | null; + treeRef?: React.RefObject; setTree: React.Dispatch>; node: FileTreeNode; isBulk: boolean; @@ -134,12 +115,15 @@ function useFileContextMenuDelete({ selectedPaths?: Set; }) { const [deleteConfirmationOpen, setDeleteConfirmationOpen] = useState(false); + const fallbackTreeRef = useRef(tree); + fallbackTreeRef.current = tree; + const currentTreeRef = treeRef ?? fallbackTreeRef; const handleConfirmDelete = useCallback(() => { setDeleteConfirmationOpen(false); if (!onDeleteFile) return; if (isBulk && selectedPaths) { - const snapshot = tree; + const snapshot = currentTreeRef.current; const paths = [...selectedPaths]; setTree((prev) => { let nextTree = prev; @@ -153,8 +137,8 @@ function useFileContextMenuDelete({ }); return; } - deleteNodeOptimistically(tree, setTree, node.path, onDeleteFile); - }, [tree, setTree, node.path, onDeleteFile, isBulk, selectedPaths]); + deleteNodeOptimistically(currentTreeRef.current, setTree, node.path, onDeleteFile); + }, [setTree, node.path, onDeleteFile, isBulk, selectedPaths, currentTreeRef]); const handleDelete = useCallback(() => { if (!onDeleteFile) return; @@ -312,55 +296,11 @@ function FileContextMenuSurface({ } /** Context menu for file nodes with Download, Rename, and Delete options */ -/** - * Assemble the confirm-delete descriptor. Extracted so `FileContextMenu` stays - * within the component size and complexity limits. - */ -function useDeleteAction({ - node, - isBulk, - selectedCount, - enabled, - confirming, - handleDelete, - handleConfirmDelete, - setDeleteConfirmationOpen, -}: { - node: FileTreeNode; - isBulk: boolean; - selectedCount: number; - enabled: boolean; - confirming: boolean; - handleDelete: () => void; - handleConfirmDelete: () => void; - setDeleteConfirmationOpen: (open: boolean) => void; -}) { - const { t } = useTranslation(); - if (!enabled) return null; - return { - isBulk, - selectedCount, - confirming, - title: isBulk - ? t("task:deleteItemsTitle", { count: selectedCount }) - : t("task:delete2", { name: node.name }), - label: isBulk ? t("task:deleteItemsLabel", { count: selectedCount }) : t("task:delete"), - cancelLabel: t("common:cancel"), - description: node.is_dir ? ( - - ) : ( - - ), - onDelete: handleDelete, - onCancel: () => setDeleteConfirmationOpen(false), - onConfirm: handleConfirmDelete, - }; -} - export function FileContextMenu({ children, node, tree, + treeRef, setTree, onDeleteFile, onRenameFile, @@ -375,6 +315,7 @@ export function FileContextMenu({ children: React.ReactNode; node: FileTreeNode; tree: FileTreeNode | null; + treeRef?: React.RefObject; setTree: React.Dispatch>; onDeleteFile?: (path: string) => Promise; onRenameFile?: (oldPath: string, newPath: string) => Promise; @@ -388,6 +329,7 @@ export function FileContextMenu({ }) { const editorActions = useFileTreeEditorActions(); const { isFinePointer } = useResponsiveBreakpoint(); + const { t } = useTranslation(); const isBulk = selectedCount > 1; // A bulk selection would make a single-node "Open in " ambiguous. const showOpenInEditor = !isBulk && canOpenNodeInEditor(editorActions, node); @@ -400,6 +342,7 @@ export function FileContextMenu({ const { deleteConfirmationOpen, setDeleteConfirmationOpen, handleConfirmDelete, handleDelete } = useFileContextMenuDelete({ tree, + treeRef, setTree, node, isBulk, @@ -413,15 +356,16 @@ export function FileContextMenu({ contextMenuRef, }); - const deleteAction = useDeleteAction({ + const deleteAction = createFileDeleteAction({ node, - isBulk, + onDeleteFile, selectedCount, - enabled: !!onDeleteFile, - confirming: deleteConfirmationOpen, + isBulk, + deleteConfirmationOpen, + t, handleDelete, - handleConfirmDelete, setDeleteConfirmationOpen, + handleConfirmDelete, }); if (!hasFileActions && !showOpenInEditor && !showAddToChatContext) return <>{children}; @@ -460,11 +404,15 @@ export function useFileRename( tree: FileTreeNode | null, setTree: React.Dispatch>, onRenameFile?: (oldPath: string, newPath: string) => Promise, + treeRef?: React.RefObject, ) { const { t } = useTranslation(); const { toast } = useToast(); const [isRenaming, setIsRenaming] = useState(false); const [renameValue, setRenameValue] = useState(node.name); + const fallbackTreeRef = useRef(tree); + fallbackTreeRef.current = tree; + const currentTreeRef = treeRef ?? fallbackTreeRef; const handleStartRename = useCallback(() => { setRenameValue(node.name); @@ -495,9 +443,10 @@ export function useFileRename( ? node.path.substring(0, node.path.lastIndexOf("/")) : ""; const newPath = parentPath ? `${parentPath}/${newName}` : newName; - const snapshot = tree; + const currentTree = currentTreeRef.current; + const snapshot = currentTree; setIsRenaming(false); - if (tree && treeContainsPath(tree, newPath)) { + if (currentTree && treeContainsPath(currentTree, newPath)) { toast({ title: t("task:failedToRenameItem"), description: t("task:targetAlreadyExists", { newPath }), @@ -514,7 +463,16 @@ export function useFileRename( .catch(() => { setTree(snapshot); }); - }, [renameValue, node.name, node.path, onRenameFile, tree, setTree, handleCancelRename, toast]); + }, [ + renameValue, + node.name, + node.path, + onRenameFile, + setTree, + handleCancelRename, + toast, + currentTreeRef, + ]); const handleRenameKeyDown = useCallback( (e: React.KeyboardEvent) => { diff --git a/apps/web/components/task/file-delete-action.tsx b/apps/web/components/task/file-delete-action.tsx new file mode 100644 index 0000000000..19af9d58cb --- /dev/null +++ b/apps/web/components/task/file-delete-action.tsx @@ -0,0 +1,60 @@ +import type React from "react"; +import type { TFunction } from "i18next"; +import type { FileTreeNode } from "@/lib/types/backend"; +import { countFilesInTree } from "./file-tree-utils"; +import { DeleteFileDescription, DeleteFolderDescription } from "./file-delete-confirmation"; + +export type FileDeleteAction = { + isBulk: boolean; + selectedCount: number; + confirming: boolean; + title: string; + label: string; + cancelLabel: string; + description: React.ReactNode; + onDelete: () => void; + onCancel: () => void; + onConfirm: () => void; +}; + +export function createFileDeleteAction({ + node, + onDeleteFile, + selectedCount, + isBulk, + deleteConfirmationOpen, + t, + handleDelete, + setDeleteConfirmationOpen, + handleConfirmDelete, +}: { + node: FileTreeNode; + onDeleteFile?: (path: string) => Promise; + selectedCount: number; + isBulk: boolean; + deleteConfirmationOpen: boolean; + t: TFunction; + handleDelete: () => void; + setDeleteConfirmationOpen: (open: boolean) => void; + handleConfirmDelete: () => void; +}): FileDeleteAction | null { + if (!onDeleteFile) return null; + return { + isBulk, + selectedCount, + confirming: deleteConfirmationOpen, + title: isBulk + ? t("task:deleteItemsTitle", { count: selectedCount }) + : t("task:delete2", { name: node.name }), + label: isBulk ? t("task:deleteItemsLabel", { count: selectedCount }) : t("task:delete"), + cancelLabel: t("common:cancel"), + description: node.is_dir ? ( + + ) : ( + + ), + onDelete: handleDelete, + onCancel: () => setDeleteConfirmationOpen(false), + onConfirm: handleConfirmDelete, + }; +} diff --git a/apps/web/components/task/file-tree-row-props.ts b/apps/web/components/task/file-tree-row-props.ts new file mode 100644 index 0000000000..5454e9cbbf --- /dev/null +++ b/apps/web/components/task/file-tree-row-props.ts @@ -0,0 +1,84 @@ +import type React from "react"; +import type { FileTreeNode } from "@/lib/types/backend"; +import type { FileInfo } from "@/lib/state/store"; +import type { VisibleRow } from "@/hooks/use-tree"; + +type GitFileStatus = FileInfo["status"] | undefined; + +export type TreeNodeRowProps = { + row: VisibleRow; + activeFolderPath: string; + activeFilePath?: string | null; + visibleLoadingPaths: Set; + fileStatuses: Map; + tree: FileTreeNode | null; + treeRef?: React.RefObject; + onToggleExpand: (node: FileTreeNode) => void; + onOpenFile: (path: string) => void; + onDeleteFile?: (path: string) => Promise; + onRenameFile?: (oldPath: string, newPath: string) => Promise; + onDownloadFile?: (path: string) => Promise; + onUploadFilesHere?: (path: string) => void; + setTree: React.Dispatch>; + isSelectedFn?: (path: string) => boolean; + onSelect?: (path: string, e: React.MouseEvent) => boolean; + isDragging?: boolean; + dragOverPath?: string | null; + onDragStart?: (path: string, e: React.DragEvent) => void; + onDragEnd?: () => void; + onDragOver?: (path: string, e: React.DragEvent) => void; + onDragLeave?: (e: React.DragEvent) => void; + onDrop?: (targetPath: string, e: React.DragEvent) => void; + selectedCount?: number; + selectedPaths?: Set; + showTouchActions?: boolean; + onAddToChatContext?: (node: FileTreeNode) => void; +}; + +function rowIdentity(props: TreeNodeRowProps): unknown[] { + const path = props.row.path; + return [ + props.row.node, + props.row.path, + props.row.depth, + props.row.displayName, + props.row.isExpanded, + props.row.isDir, + props.activeFilePath === path, + props.activeFolderPath === path, + props.visibleLoadingPaths.has(path), + props.fileStatuses.get(path), + props.isSelectedFn?.(path), + Boolean(props.isDragging && props.isSelectedFn?.(path)), + props.dragOverPath === path, + props.treeRef, + props.treeRef ? undefined : props.tree, + props.onToggleExpand, + props.onOpenFile, + props.onDeleteFile, + props.onRenameFile, + props.onDownloadFile, + props.onUploadFilesHere, + props.setTree, + props.isSelectedFn, + props.onSelect, + props.onDragStart, + props.onDragEnd, + props.onDragOver, + props.onDragLeave, + props.onDrop, + props.selectedCount, + props.selectedPaths, + props.showTouchActions, + props.onAddToChatContext, + ]; +} + +export function areTreeNodeRowPropsEqual( + previous: TreeNodeRowProps, + next: TreeNodeRowProps, +): boolean { + const previousIdentity = rowIdentity(previous); + const nextIdentity = rowIdentity(next); + return previousIdentity.every((value, index) => Object.is(value, nextIdentity[index])); +} diff --git a/apps/web/components/task/task-session-sidebar-selection.test.ts b/apps/web/components/task/task-session-sidebar-selection.test.ts index edf9fabe22..87b9863418 100644 --- a/apps/web/components/task/task-session-sidebar-selection.test.ts +++ b/apps/web/components/task/task-session-sidebar-selection.test.ts @@ -1,6 +1,6 @@ import { act, renderHook } from "@testing-library/react"; import { describe, expect, it, vi } from "vitest"; -import { useBulkConfirmDialog } from "./task-session-sidebar-selection"; +import { useBulkConfirmDialog, useSelectionHandlers } from "./task-session-sidebar-selection"; const tasks = [ { id: "a", remoteExecutorType: "docker" }, @@ -54,3 +54,41 @@ describe("useBulkConfirmDialog", () => { } }); }); + +describe("useSelectionHandlers", () => { + it("keeps bulk move stable when the selection aggregate is recreated", () => { + const clearSelection = vi.fn(); + const selectRange = vi.fn(); + const pruneToVisible = vi.fn(); + const bulkMove = vi.fn(); + const multiSelect = { + isSelecting: false, + clearSelection, + selectRange, + pruneToVisible, + bulkMove, + } as unknown as Parameters[0]["multiSelect"]; + const stableArgs = { + pinTasks: vi.fn(), + unpinTasks: vi.fn(), + pinnedTaskIds: ["pinned"], + visibleTaskIds: ["a", "b"], + movableSelectedIds: new Set(["a", "b"]), + }; + + const { result, rerender } = renderHook( + ({ selection }: { selection: typeof multiSelect }) => + useSelectionHandlers({ ...stableArgs, multiSelect: selection }), + { initialProps: { selection: multiSelect } }, + ); + const firstBulkMove = result.current.onBulkMove; + + rerender({ + selection: { + ...multiSelect, + }, + }); + + expect(result.current.onBulkMove).toBe(firstBulkMove); + }); +}); diff --git a/apps/web/components/task/task-session-sidebar-selection.tsx b/apps/web/components/task/task-session-sidebar-selection.tsx index 450ef37b87..d74fb90685 100644 --- a/apps/web/components/task/task-session-sidebar-selection.tsx +++ b/apps/web/components/task/task-session-sidebar-selection.tsx @@ -57,7 +57,7 @@ type Multi = ReturnType; * The selection-lifecycle effects (Escape-to-clear, prune-hidden) and the * memoized range/move/pin callbacks threaded into `TaskSwitcher`. */ -function useSelectionHandlers(args: { +export function useSelectionHandlers(args: { multiSelect: Multi; pinTasks: (ids: string[]) => void; unpinTasks: (ids: string[]) => void; @@ -67,7 +67,7 @@ function useSelectionHandlers(args: { }) { const { multiSelect, pinTasks, unpinTasks, pinnedTaskIds, visibleTaskIds, movableSelectedIds } = args; - const { isSelecting, clearSelection, selectRange, pruneToVisible } = multiSelect; + const { isSelecting, clearSelection, selectRange, pruneToVisible, bulkMove } = multiSelect; // Escape clears an active selection. useEffect(() => { @@ -95,7 +95,7 @@ function useSelectionHandlers(args: { // the destination; drop workflow-less rows that can't be moved. const onBulkMove = useCallback( (ids: string[], targetWorkflowId: string, targetStepId: string) => - multiSelect.bulkMove( + bulkMove( sortIdsByVisibleOrder( ids.filter((id) => movableSelectedIds.has(id)), visibleTaskIds, @@ -103,7 +103,7 @@ function useSelectionHandlers(args: { targetWorkflowId, targetStepId, ), - [multiSelect, visibleTaskIds, movableSelectedIds], + [bulkMove, visibleTaskIds, movableSelectedIds], ); const onBulkPin = useCallback( diff --git a/apps/web/components/task/task-switcher-row-render-identity.test.tsx b/apps/web/components/task/task-switcher-row-render-identity.test.tsx new file mode 100644 index 0000000000..71773b7106 --- /dev/null +++ b/apps/web/components/task/task-switcher-row-render-identity.test.tsx @@ -0,0 +1,42 @@ +import type { ReactNode } from "react"; +import { cleanup, render } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { TaskSwitcherItem } from "./task-switcher-types"; + +const taskItemRender = vi.hoisted(() => vi.fn()); + +vi.mock("./task-item", () => ({ + TaskItem: ({ title }: { title: string }) => { + taskItemRender(); + return
{title}
; + }, +})); + +vi.mock("./task-switcher-context-menu", () => ({ + TaskItemWithContextMenu: ({ children }: { children: ReactNode }) => <>{children}, +})); + +import { TaskRow, type TaskRowProps } from "./task-switcher-row"; + +const TASK: TaskSwitcherItem = { id: "task-1", title: "Task 1", state: "IN_PROGRESS" }; + +afterEach(() => { + cleanup(); + taskItemRender.mockReset(); +}); + +describe("TaskRow render isolation", () => { + it("skips an unrelated owner render when the row props remain equal", () => { + const stableProps: TaskRowProps = { + task: TASK, + activeTaskId: null, + selectedTaskId: null, + onSelectTask: vi.fn(), + }; + const { rerender } = render(); + + rerender(); + + expect(taskItemRender).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/web/components/task/task-top-bar-plugin-actions.test.tsx b/apps/web/components/task/task-top-bar-plugin-actions.test.tsx index 0a21558c02..1acbf7a1dd 100644 --- a/apps/web/components/task/task-top-bar-plugin-actions.test.tsx +++ b/apps/web/components/task/task-top-bar-plugin-actions.test.tsx @@ -16,6 +16,7 @@ const mockState = { }, }, }; +const originalSessions = mockState.taskSessionsByTask.itemsByTaskId.t1; vi.mock("@/components/state-provider", () => ({ useOptionalAppStore: (selector: (s: typeof mockState) => unknown) => selector(mockState), @@ -25,6 +26,7 @@ describe("TaskTopBarPluginActions", () => { afterEach(() => { cleanup(); pluginRegistry.unregisterPlugin("plugin-a"); + mockState.taskSessionsByTask.itemsByTaskId.t1 = originalSessions; }); it("renders nothing when no plugin registered a chat-top-bar component", () => { @@ -82,4 +84,25 @@ describe("TaskTopBarPluginActions", () => { expect(screen.getByTestId("plugin-topbar").textContent).toBe("null|s9|s9"); }); + + it("does not rerender the contribution for equal session ids", () => { + const pluginRender = vi.fn(); + pluginRegistry.forPlugin("plugin-a").registerComponent(SLOT, () => { + pluginRender(); + return null; + }); + + const { rerender } = render( + , + ); + mockState.taskSessionsByTask.itemsByTaskId.t1 = originalSessions.map((session) => ({ + ...session, + state: "finished", + })); + rerender( + , + ); + + expect(pluginRender).toHaveBeenCalledTimes(1); + }); }); diff --git a/apps/web/components/task/task-top-bar-plugin-actions.tsx b/apps/web/components/task/task-top-bar-plugin-actions.tsx index a4abd98c7f..6ad428e2a1 100644 --- a/apps/web/components/task/task-top-bar-plugin-actions.tsx +++ b/apps/web/components/task/task-top-bar-plugin-actions.tsx @@ -1,6 +1,6 @@ "use client"; -import { useCallback, useMemo } from "react"; +import { memo, useCallback, useMemo, useRef } from "react"; import { useOptionalAppStore } from "@/components/state-provider"; import { PluginSlot } from "@/components/plugins/plugin-slot"; import type { AppState } from "@/lib/state/store"; @@ -34,6 +34,27 @@ export type ChatTopBarSlotProps = { }; const EMPTY_SESSIONS: TaskSession[] = []; +const MemoizedPluginSlot = memo(PluginSlot); + +function sameStringArray(left: string[], right: string[]): boolean { + return left.length === right.length && left.every((value, index) => value === right[index]); +} + +function useStableSessionIds( + taskSessions: TaskSession[], + activeSessionId: string | null, +): string[] { + const sessionIdsRef = useRef([]); + return useMemo(() => { + const nextSessionIds: string[] = taskSessions.map((session) => session.id); + if (activeSessionId && !nextSessionIds.includes(activeSessionId)) { + nextSessionIds.unshift(activeSessionId); + } + if (sameStringArray(sessionIdsRef.current, nextSessionIds)) return sessionIdsRef.current; + sessionIdsRef.current = nextSessionIds; + return nextSessionIds; + }, [taskSessions, activeSessionId]); +} /** * Plugin extension point in the session top bar, rendered alongside the @@ -59,14 +80,11 @@ export function TaskTopBarPluginActions(props: { [taskId], ); const taskSessions = useOptionalAppStore(selectSessions, EMPTY_SESSIONS); + const sessionIds = useStableSessionIds(taskSessions, sessionId); const slotProps = useMemo(() => { - const sessionIds: string[] = taskSessions.map((session) => session.id); - // The active session may not yet be in the store list (freshly prepared); - // make sure the plugin always receives it. - if (sessionId && !sessionIds.includes(sessionId)) sessionIds.unshift(sessionId); return { taskId, taskTitle, workspaceId, activeSessionId: sessionId, sessionIds }; - }, [taskSessions, sessionId, taskId, taskTitle, workspaceId]); + }, [sessionId, sessionIds, taskId, taskTitle, workspaceId]); - return ; + return ; } diff --git a/apps/web/e2e/pages/file-tree-page.ts b/apps/web/e2e/pages/file-tree-page.ts index d1d72c17a4..366c733ef0 100644 --- a/apps/web/e2e/pages/file-tree-page.ts +++ b/apps/web/e2e/pages/file-tree-page.ts @@ -15,6 +15,16 @@ export class FileTreePage { ); } + /** The existing Files viewport that owns tree scrolling. */ + fileTreeScrollViewport(): Locator { + return this.page.locator('[data-testid="file-tree-scroll"]:visible'); + } + + /** Visible tree rows, including only rows currently mounted by the tree. */ + visibleFileTreeNodes(): Locator { + return this.page.locator('[data-testid="file-tree-node"]:visible'); + } + /** Visible search button in the Files panel, including the mobile panel mount. */ fileSearchButton(): Locator { return this.page.locator('button[aria-label="Search files"]:visible'); diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index bd5214881d..2e70146e56 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -1713,6 +1713,16 @@ export class SessionPage { return this.fileTree.fileTreeNode(nodePath); } + /** The existing Files viewport that owns tree scrolling. */ + fileTreeScrollViewport(): Locator { + return this.fileTree.fileTreeScrollViewport(); + } + + /** Visible tree rows, including only rows currently mounted by the tree. */ + visibleFileTreeNodes(): Locator { + return this.fileTree.visibleFileTreeNodes(); + } + /** Visible search button in the Files panel. */ fileSearchButton(): Locator { return this.fileTree.fileSearchButton(); diff --git a/apps/web/e2e/tests/task/large-file-tree-virtualization-helpers.ts b/apps/web/e2e/tests/task/large-file-tree-virtualization-helpers.ts new file mode 100644 index 0000000000..8cc91679bc --- /dev/null +++ b/apps/web/e2e/tests/task/large-file-tree-virtualization-helpers.ts @@ -0,0 +1,60 @@ +import path from "node:path"; +import type { Page } from "@playwright/test"; +import type { SeedData } from "../../fixtures/test-base"; +import type { ApiClient } from "../../helpers/api-client"; +import type { BackendContext } from "../../fixtures/backend"; +import { GitHelper, makeGitEnv } from "../../helpers/git-helper"; +import { SessionPage } from "../../pages/session-page"; + +export const LARGE_FILE_TREE_FOLDER = "large-file-tree"; +export const LARGE_FILE_TREE_COUNT = 600; + +export function largeFileTreePath(index: number): string { + return `${LARGE_FILE_TREE_FOLDER}/entry-${index.toString().padStart(4, "0")}.txt`; +} + +export function seedLargeFileTree(backend: BackendContext): void { + const git = new GitHelper( + path.join(backend.tmpDir, "repos", "e2e-repo"), + makeGitEnv(backend.tmpDir), + ); + for (let index = 0; index < LARGE_FILE_TREE_COUNT; index += 1) { + git.createFile(largeFileTreePath(index), `large tree entry ${index}\n`); + } + git.stageAll(); + if (git.exec("git status --short").trim()) { + git.commit("seed large file tree"); + } +} + +export async function setupLargeFileTreeTask({ + testPage, + apiClient, + seedData, + backend, + title, +}: { + testPage: Page; + apiClient: ApiClient; + seedData: SeedData; + backend: BackendContext; + title: string; +}): Promise { + seedLargeFileTree(backend); + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + title, + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await session.waitForChatIdle({ timeout: 45_000 }); + return session; +} diff --git a/apps/web/e2e/tests/task/large-file-tree-virtualization.spec.ts b/apps/web/e2e/tests/task/large-file-tree-virtualization.spec.ts new file mode 100644 index 0000000000..6ad46ee385 --- /dev/null +++ b/apps/web/e2e/tests/task/large-file-tree-virtualization.spec.ts @@ -0,0 +1,84 @@ +import { test, expect } from "../../fixtures/test-base"; +import { + LARGE_FILE_TREE_FOLDER, + LARGE_FILE_TREE_COUNT, + largeFileTreePath, + setupLargeFileTreeTask, +} from "./large-file-tree-virtualization-helpers"; + +test.describe("Large file tree virtualization", () => { + test("mounts a bounded row window and reaches the last file", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(120_000); + const session = await setupLargeFileTreeTask({ + testPage, + apiClient, + seedData, + backend, + title: "Large file tree virtualization", + }); + + await session.clickTab("Files"); + const folder = session.fileTreeNode(LARGE_FILE_TREE_FOLDER); + await expect(folder).toBeVisible({ timeout: 15_000 }); + await expect(session.fileTreeNode(largeFileTreePath(0))).toHaveCount(0); + + await folder.click(); + await expect(session.fileTreeNode(largeFileTreePath(0))).toBeVisible({ timeout: 15_000 }); + await expect + .poll(() => session.visibleFileTreeNodes().count(), { timeout: 5_000 }) + .toBeLessThan(80); + + const viewport = session.fileTreeScrollViewport(); + await expect(viewport).toBeVisible(); + await viewport.evaluate((element) => { + element.scrollTop = element.scrollHeight; + element.dispatchEvent(new Event("scroll", { bubbles: true })); + }); + + const lastFile = largeFileTreePath(LARGE_FILE_TREE_COUNT - 1); + await expect(session.fileTreeNode(lastFile)).toBeVisible({ timeout: 15_000 }); + await session.fileTreeNode(lastFile).click(); + await expect(testPage.getByTestId("preview-tab-file-editor")).toBeVisible({ timeout: 15_000 }); + }); + + test("reveals the create input after scrolling away from its folder", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(120_000); + const session = await setupLargeFileTreeTask({ + testPage, + apiClient, + seedData, + backend, + title: "Large file tree create reveal", + }); + + await session.clickTab("Files"); + const folder = session.fileTreeNode(LARGE_FILE_TREE_FOLDER); + await expect(folder).toBeVisible({ timeout: 15_000 }); + await folder.click(); + await expect(session.fileTreeNode(largeFileTreePath(0))).toBeVisible({ timeout: 15_000 }); + + const viewport = session.fileTreeScrollViewport(); + await viewport.evaluate((element) => { + element.scrollTop = element.scrollHeight; + element.dispatchEvent(new Event("scroll", { bubbles: true })); + }); + + const createMenu = testPage.getByTestId("files-create-menu"); + await expect(createMenu).toBeVisible({ timeout: 15_000 }); + await createMenu.click(); + await testPage.getByRole("menuitem", { name: "New file" }).click(); + const input = testPage.getByPlaceholder("filename..."); + await expect(input).toBeVisible({ timeout: 15_000 }); + await expect(input).toBeFocused({ timeout: 5_000 }); + }); +}); diff --git a/apps/web/e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts b/apps/web/e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts new file mode 100644 index 0000000000..e5dea4c42a --- /dev/null +++ b/apps/web/e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts @@ -0,0 +1,62 @@ +import { test, expect } from "../../fixtures/test-base"; +import { + LARGE_FILE_TREE_COUNT, + LARGE_FILE_TREE_FOLDER, + largeFileTreePath, + setupLargeFileTreeTask, +} from "./large-file-tree-virtualization-helpers"; + +test.describe("Mobile large file tree virtualization", () => { + test("keeps the row window bounded and preserves touch actions", async ({ + testPage, + apiClient, + seedData, + backend, + }) => { + test.setTimeout(120_000); + const session = await setupLargeFileTreeTask({ + testPage, + apiClient, + seedData, + backend, + title: "Mobile large file tree virtualization", + }); + + await testPage.getByRole("button", { name: "Files" }).tap(); + const folder = session.fileTreeNode(LARGE_FILE_TREE_FOLDER); + await expect(folder).toBeVisible({ timeout: 15_000 }); + await folder.tap(); + await expect(session.fileTreeNode(largeFileTreePath(0))).toBeVisible({ timeout: 15_000 }); + await expect + .poll(() => session.visibleFileTreeNodes().count(), { timeout: 5_000 }) + .toBeLessThan(80); + + const firstActions = session.fileTreeNodeActions(largeFileTreePath(0)); + await expect(firstActions).toBeVisible(); + const firstActionsBox = await firstActions.boundingBox(); + expect(firstActionsBox).not.toBeNull(); + expect(firstActionsBox!.width).toBeGreaterThanOrEqual(44); + expect(firstActionsBox!.height).toBeGreaterThanOrEqual(44); + + const viewport = session.fileTreeScrollViewport(); + await expect(viewport).toBeVisible(); + await viewport.evaluate((element) => { + element.scrollTop = element.scrollHeight; + element.dispatchEvent(new Event("scroll", { bubbles: true })); + }); + + const lastFile = largeFileTreePath(LARGE_FILE_TREE_COUNT - 1); + await expect(session.fileTreeNode(lastFile)).toBeVisible({ timeout: 15_000 }); + const lastActions = session.fileTreeNodeActions(lastFile); + await expect(lastActions).toBeVisible(); + const lastActionsBox = await lastActions.boundingBox(); + expect(lastActionsBox).not.toBeNull(); + expect(lastActionsBox!.width).toBeGreaterThanOrEqual(44); + expect(lastActionsBox!.height).toBeGreaterThanOrEqual(44); + + await session.fileTreeNode(lastFile).tap(); + const viewer = testPage.getByTestId("mobile-file-viewer-panel"); + await expect(viewer).toBeVisible({ timeout: 15_000 }); + await expect(viewer).toContainText(lastFile); + }); +}); diff --git a/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx b/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx index 045e96f4f9..e643ce7493 100644 --- a/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx +++ b/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx @@ -112,6 +112,17 @@ afterEach(() => { }); describe("useTaskPRTooltipHydration request coordination", () => { + it("keeps its aggregate result stable when inputs are unchanged", () => { + const { result, rerender } = renderHook(() => useTaskPRTooltipHydration("task-1"), { + wrapper: createStateWrapper(), + }); + const firstResult = result.current; + + rerender(); + + expect(result.current).toBe(firstResult); + }); + it("deduplicates concurrent requests for the same task and store", async () => { const response = deferred<{ task_prs: Record }>(); listTaskPRsMock.mockReturnValue(response.promise); diff --git a/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.ts b/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.ts index 338840744f..fc9f32da7c 100644 --- a/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.ts +++ b/apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.ts @@ -1,6 +1,6 @@ "use client"; -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import type { StoreApi } from "zustand"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { getTaskCIAutomationOptions, listTaskPRs } from "@/lib/api/domains/github-api"; @@ -235,13 +235,16 @@ export function useTaskPRTooltipHydration( }); }, [includeAutomation, store, taskId, workspaceContextGeneration, workspaceId]); - return { - status, - hydrate, - // Prefer the store entry because WebSocket updates replace the fetched - // snapshot while the disclosure remains mounted. Fall back to the local - // snapshot for compatibility with callers/tests that provide no store - // entry (for example, a request that completed before state hydration). - automationOptions: storedAutomationOptions ?? hydratedAutomationOptions, - }; + return useMemo( + () => ({ + status, + hydrate, + // Prefer the store entry because WebSocket updates replace the fetched + // snapshot while the disclosure remains mounted. Fall back to the local + // snapshot for compatibility with callers/tests that provide no store + // entry (for example, a request that completed before state hydration). + automationOptions: storedAutomationOptions ?? hydratedAutomationOptions, + }), + [status, hydrate, storedAutomationOptions, hydratedAutomationOptions], + ); } diff --git a/docs/plans/task-surface-render-isolation/plan.md b/docs/plans/task-surface-render-isolation/plan.md new file mode 100644 index 0000000000..037aae6550 --- /dev/null +++ b/docs/plans/task-surface-render-isolation/plan.md @@ -0,0 +1,174 @@ +--- +spec: docs/specs/ui/requirements/file-tree-chat-context.md +created: 2026-08-31 +status: in_progress +--- + +# Fix Plan: Task Surface Render Isolation + +## Outcome + +Keep file-tree, sidebar, pull-request, and plugin behavior unchanged. Reduce React work by isolating +unchanged rows, virtualizing large file trees, and stabilizing derived props. + +## Confirmed evidence + +The enhanced Chrome trace covers 5.74 seconds. Main-thread `RunTask` work used 3.56 seconds, or +approximately 62 percent of the window. Three application stalls lasted 978 ms, 841 ms, and 837 ms. + +The trace showed three broad React render waves. Each wave rendered approximately 602 file-tree +rows and 48 sidebar task rows. It also recreated the context-menu provider stack for each mounted +file row. + +Changed-prop diagnostics identified these unstable values: + +- File-tree rows received new `onToggleExpand` and `onDrop` functions. +- Sidebar rows received a new `onBulkMove` function. +- Plugin actions received an equal but new `sessionIds` array. +- Pull-request indicators received equal but new hydration and empty-list values. + +Layout, paint, and minor garbage collection used less than 100 ms combined. The trace also included +React DevTools and CPU-profiler overhead. Therefore, final measurements use a production build +without extensions. + +## Technical direction + +### Stable row inputs + +- Remove complete aggregate objects from file-tree callback dependencies. +- Memoize `TreeNodeItem` around all inputs that change its output or actions. +- Stabilize sidebar selection callbacks before they cross the `TaskSwitcher` boundary. +- Add render-count tests that use controlled state changes instead of timing assertions. + +### Virtual file tree + +- Use the existing `@tanstack/react-virtual` dependency. +- Keep `visibleRows` as the ordered flat tree model. +- Keep the current file-tree viewport as the only scroll owner. +- Measure rows because edit controls and mobile actions can change row height. +- Update reveal and restore flows so offscreen target rows mount before DOM access. + +### Stable contribution props + +- Keep plugin `sessionIds` stable while their ordered values remain equal. +- Memoize the pull-request hydration aggregate around its semantic inputs. +- Reuse a module-level empty pull-request list. + +## Files + +### File-tree render isolation + +- `apps/web/components/task/file-browser.tsx` +- `apps/web/components/task/file-browser-hooks.ts` +- `apps/web/components/task/file-browser-parts.tsx` +- `apps/web/components/task/file-browser-render-identity.test.tsx` +- Existing focused file-browser tests + +### File-tree virtualization + +- `apps/web/components/task/file-browser-parts.tsx` +- `apps/web/e2e/tests/task/large-file-tree-virtualization-helpers.ts` +- `apps/web/e2e/tests/task/large-file-tree-virtualization.spec.ts` +- `apps/web/e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts` +- Existing desktop and mobile file-tree tests + +### Sidebar render isolation + +- `apps/web/components/task/task-session-sidebar-selection.tsx` +- `apps/web/components/task/task-switcher.tsx` +- `apps/web/components/task/task-session-sidebar-selection.test.ts` +- `apps/web/components/task/task-switcher.test.tsx` + +### Contribution prop stability + +- `apps/web/components/task/task-top-bar-plugin-actions.tsx` +- `apps/web/components/task/task-top-bar-plugin-actions.test.tsx` +- `apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.ts` +- `apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx` +- `apps/web/components/github/pr-task-icon.tsx` +- `apps/web/components/github/pr-task-icon.render.test.tsx` + +### Durable artifacts + +- `docs/specs/ui/system-design/task-surface-render-isolation.md` +- `docs/plans/task-surface-render-isolation/plan.md` +- Four work orders in this plan directory + +No backend, API, database, localization, dependency, or public-documentation change is required. + +## Mobile design contract + +The Files panel stays in its current task surface. Its existing viewport remains the only vertical +scroll owner, and the page does not gain horizontal overflow. + +Each mounted file row keeps the visible mobile action and 44 CSS pixel target. Offscreen rows mount +through normal scrolling or explicit virtualizer reveal. + +The sidebar row remains the primary mobile tap action. Its explicit task menu and responsive menu +surface remain unchanged. + +## Test strategy + +- Unit tests prove stable callback, aggregate, array, and empty-list identities. +- Component tests prove that unrelated owner updates do not rerender unchanged rows. +- Desktop E2E tests use at least 600 generated file entries. +- Mobile E2E tests prove bounded mounted rows, scrolling, file opening, and touch actions. +- Existing behavior tests cover expansion, selection, drag and drop, context actions, and sidebar + navigation. + +## Verification + +Run the commands from the repository root unless a step changes directories. + +```bash +cd apps +rtk pnpm --filter @kandev/web test components/task/file-browser-render-identity.test.tsx components/task/file-browser-actions.test.ts components/task/file-browser-toggle-expand.test.ts +rtk pnpm --filter @kandev/web test components/task/task-session-sidebar-selection.test.ts components/task/task-switcher.test.tsx +rtk pnpm --filter @kandev/web test components/task/task-top-bar-plugin-actions.test.tsx hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx components/github/pr-task-icon.render.test.tsx +rtk pnpm --filter @kandev/web typecheck +rtk pnpm --filter @kandev/web lint +cd .. +rtk make build-web +cd apps/web +rtk pnpm e2e:run --no-build --project chromium tests/task/large-file-tree-virtualization.spec.ts +rtk pnpm e2e:run --no-build --project mobile-chrome tests/task/mobile-large-file-tree-virtualization.spec.ts tests/task/mobile-file-tree-chat-context.spec.ts +rtk pnpm run lint:e2e-sleeps -- e2e/tests/task/large-file-tree-virtualization.spec.ts e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts +cd ../.. +rtk git diff --check +``` + +## Implementation waves + +1. [Stabilize file-tree row inputs](task-01-stabilize-file-tree-rows.md) +2. [Virtualize the large file tree](task-02-virtualize-file-tree.md) +3. [Stabilize sidebar row inputs](task-03-stabilize-sidebar-rows.md) +4. [Stabilize contribution props](task-04-stabilize-contribution-props.md) + +The waves are sequential because each task uses the same web validation environment. Tasks 3 and +4 do not depend on file-tree code, but this plan does not authorize subagents. + +## Performance evaluation + +After Wave 4, capture the same interaction from a warmed production build. Disable browser +extensions and do not include profiler startup in the selected window. + +Compare these values with the original trace: + +- Total main-thread `RunTask` time and long-task count. +- React renders for `TreeNodeItem`, `TaskRow`, plugin actions, and pull-request icons. +- Mounted file rows and menu providers before and after scrolling. +- Layout, paint, and garbage-collection time as regression guards. + +The trace comparison is diagnostic evidence. It is not a CI timing threshold. + +## Risks and exclusions + +- Incorrect memo equality can show stale selection, expansion, drag, or active-file state. +- Virtual rows have variable height during rename and on touch layouts. +- Reveal logic must mount an offscreen row before it requests a DOM element. +- Overscan must keep scrolling smooth without recreating hundreds of menu providers. +- Store selectors must not return a new array on every snapshot read. +- This plan does not consolidate all file-row menus into one shared menu. +- This plan does not change WebSocket update frequency or backend state shapes. + +No ADR is required. The design uses the existing virtual-list dependency and UI ownership rules. diff --git a/docs/plans/task-surface-render-isolation/task-01-stabilize-file-tree-rows.md b/docs/plans/task-surface-render-isolation/task-01-stabilize-file-tree-rows.md new file mode 100644 index 0000000000..a542ccc358 --- /dev/null +++ b/docs/plans/task-surface-render-isolation/task-01-stabilize-file-tree-rows.md @@ -0,0 +1,62 @@ +--- +id: "01-stabilize-file-tree-rows" +title: "Stabilize file-tree row inputs" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/requirements/file-tree-chat-context.md" +system_design: "../../specs/ui/system-design/task-surface-render-isolation.md" +--- + +# Task 01: Stabilize file-tree row inputs + +## Acceptance + +- `onToggleExpand` and `onDrop` keep their identities during unrelated file-browser renders. +- `useFileBrowserTree` keeps stable aggregate identities when its fields do not change. +- `TreeNodeItem` skips renders when all visible and interactive inputs remain equal. +- Selection, expansion, active-file, drag, edit, and context-action changes still update the affected + row. +- Existing file-tree behavior remains unchanged. + +This task preserves `AC-UI-FILE-TREE-CHAT-CONTEXT-001.1`, `.2`, and `.7`. + +## TDD sequence + +1. Add a render-count test that fails after an unrelated owner update. +2. Add identity tests for the tree aggregate and the two traced callbacks. +3. Stabilize callback dependencies and the hook result. +4. Add the memoized row boundary with complete semantic inputs. +5. Run the focused file-tree regression tests. + +## Verification + +```bash +cd apps +rtk pnpm --filter @kandev/web test components/task/file-browser-render-identity.test.tsx components/task/file-browser-actions.test.ts components/task/file-browser-toggle-expand.test.ts components/task/file-browser-restore-expanded.test.tsx +rtk pnpm --filter @kandev/web typecheck +``` + +## Files likely touched + +- `apps/web/components/task/file-browser.tsx` +- `apps/web/components/task/file-browser-hooks.ts` +- `apps/web/components/task/file-browser-parts.tsx` +- `apps/web/components/task/file-browser-render-identity.test.tsx` +- Existing focused file-browser tests + +## Dependencies + +None. + +## Inputs + +- `REQ-UI-FILE-TREE-CHAT-CONTEXT-001` +- Task Surface Render Isolation system design +- Trace evidence for `onToggleExpand`, `onDrop`, and complete row render waves + +## Output contract + +Report the stable dependency strategy, the row equality inputs, changed files, focused test results, +and risks. Do not add time-based unit assertions. diff --git a/docs/plans/task-surface-render-isolation/task-02-virtualize-file-tree.md b/docs/plans/task-surface-render-isolation/task-02-virtualize-file-tree.md new file mode 100644 index 0000000000..8f319e8c27 --- /dev/null +++ b/docs/plans/task-surface-render-isolation/task-02-virtualize-file-tree.md @@ -0,0 +1,68 @@ +--- +id: "02-virtualize-file-tree" +title: "Virtualize the large file tree" +status: done +wave: 2 +depends_on: + - "01-stabilize-file-tree-rows" +plan: "plan.md" +spec: "../../specs/ui/requirements/file-tree-chat-context.md" +system_design: "../../specs/ui/system-design/task-surface-render-isolation.md" +--- + +# Task 02: Virtualize the large file tree + +## Acceptance + +- A tree with at least 600 visible entries mounts fewer than 80 row components at one time. +- The current file-tree viewport remains the only vertical scroll owner. +- Scrolling can reach and open the first and last generated entries. +- Active-file reveal mounts and shows an offscreen target before DOM-based focus work. +- Expansion, filtering, selection, drag and drop, rename, and context actions keep their behavior. +- Mobile rows keep a visible 44 CSS pixel action and a viewport-contained responsive menu. + +This task preserves `AC-UI-FILE-TREE-CHAT-CONTEXT-001.1`, `.2`, and `.7`. It also preserves +`AC-UI-MOBILE-TASK-NAVIGATION-001.1`, `.3`, and `.7`. + +## TDD sequence + +1. Add an E2E fixture that creates at least 600 deterministic file entries. +2. Add a failing desktop test for the mounted-row bound and last-row access. +3. Add a failing mobile test for the row bound, touch action, and file opening. +4. Add `useVirtualizer` to `FileTreeView` with measurement and overscan. +5. Update reveal and restoration flows for offscreen rows. +6. Run the existing mobile context-action test with the new E2E cases. + +## Verification + +```bash +rtk make build-web +cd apps/web +rtk pnpm e2e:run --no-build --project chromium tests/task/large-file-tree-virtualization.spec.ts +rtk pnpm e2e:run --no-build --project mobile-chrome tests/task/mobile-large-file-tree-virtualization.spec.ts tests/task/mobile-file-tree-chat-context.spec.ts +rtk pnpm run lint:e2e-sleeps -- e2e/tests/task/large-file-tree-virtualization.spec.ts e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts +``` + +## Files likely touched + +- `apps/web/components/task/file-browser-parts.tsx` +- File-browser reveal or scroll helpers, if required +- `apps/web/e2e/tests/task/large-file-tree-virtualization-helpers.ts` +- `apps/web/e2e/tests/task/large-file-tree-virtualization.spec.ts` +- `apps/web/e2e/tests/task/mobile-large-file-tree-virtualization.spec.ts` + +## Dependencies + +- Task 01 supplies stable row callbacks and a memoized row boundary. + +## Inputs + +- `REQ-UI-FILE-TREE-CHAT-CONTEXT-001` +- `REQ-UI-MOBILE-TASK-NAVIGATION-001` +- Existing Kanban virtual-list implementation and large-column E2E fixture +- Existing mobile file-tree context-action E2E coverage + +## Output contract + +Report the scroll-element choice, estimate, measurement strategy, overscan, mounted-row count, +changed files, E2E results, and remaining edge cases. diff --git a/docs/plans/task-surface-render-isolation/task-03-stabilize-sidebar-rows.md b/docs/plans/task-surface-render-isolation/task-03-stabilize-sidebar-rows.md new file mode 100644 index 0000000000..cb2acf8070 --- /dev/null +++ b/docs/plans/task-surface-render-isolation/task-03-stabilize-sidebar-rows.md @@ -0,0 +1,63 @@ +--- +id: "03-stabilize-sidebar-rows" +title: "Stabilize sidebar row inputs" +status: done +wave: 3 +depends_on: + - "02-virtualize-file-tree" +plan: "plan.md" +spec: "../../specs/ui/requirements/sidebar-task-row-presentation.md" +system_design: "../../specs/ui/system-design/task-surface-render-isolation.md" +--- + +# Task 03: Stabilize sidebar row inputs + +## Acceptance + +- `onBulkMove` keeps its identity during unrelated sidebar owner renders. +- `TaskSwitcher` skips an unrelated owner render when its semantic props remain equal. +- A task-row update rerenders the affected row and preserves current selection behavior. +- Fine-pointer trailing content and the mobile primary row action remain unchanged. + +This task preserves `AC-UI-SIDEBAR-TASK-ROW-PRESENTATION-001.8` and `.12`. It also preserves +`AC-UI-MOBILE-TASK-NAVIGATION-001.1`, `.2`, `.3`, and `.7`. + +## TDD sequence + +1. Add a failing identity test for `onBulkMove`. +2. Add a failing render-count test for an unrelated sidebar owner update. +3. Derive the callback from stable action functions and current selection data. +4. Add or refine memoized boundaries only where the failing test requires them. +5. Run the focused selection and task-switcher tests. + +## Verification + +```bash +cd apps +rtk pnpm --filter @kandev/web test components/task/task-session-sidebar-selection.test.ts components/task/task-switcher.test.tsx components/task/task-switcher-context-menu.test.tsx +rtk pnpm --filter @kandev/web typecheck +``` + +## Files likely touched + +- `apps/web/components/task/task-session-sidebar-selection.tsx` +- `apps/web/components/task/task-switcher.tsx` +- `apps/web/components/task/task-switcher-row.tsx` +- `apps/web/components/task/task-session-sidebar-selection.test.ts` +- `apps/web/components/task/task-switcher.test.tsx` + +## Dependencies + +- Task 02 completes the higher-impact file-tree path before this sidebar refinement. + +## Inputs + +- `REQ-UI-SIDEBAR-TASK-ROW-PRESENTATION-001` +- `REQ-UI-MOBILE-TASK-NAVIGATION-001` +- Sidebar Task Row Presentation system design +- Trace evidence for `onBulkMove` and repeated task-row renders + +## Output contract + +Report the callback dependency change, memoized boundary, render-count result, changed files, +focused test results, and remaining risks. diff --git a/docs/plans/task-surface-render-isolation/task-04-stabilize-contribution-props.md b/docs/plans/task-surface-render-isolation/task-04-stabilize-contribution-props.md new file mode 100644 index 0000000000..8bfd5c4be0 --- /dev/null +++ b/docs/plans/task-surface-render-isolation/task-04-stabilize-contribution-props.md @@ -0,0 +1,66 @@ +--- +id: "04-stabilize-contribution-props" +title: "Stabilize contribution props" +status: done +wave: 4 +depends_on: + - "03-stabilize-sidebar-rows" +plan: "plan.md" +spec: "../../specs/ui/requirements/sidebar-task-row-presentation.md" +system_design: "../../specs/ui/system-design/task-surface-render-isolation.md" +--- + +# Task 04: Stabilize contribution props + +## Acceptance + +- Plugin `sessionIds` keeps its identity while the ordered identifiers remain equal. +- The plugin contribution does not rerender for session-state changes outside its slot props. +- Pull-request hydration returns one stable aggregate while its semantic inputs remain equal. +- Pull-request indicators reuse a stable empty list and still update when pull-request data changes. +- Plugin host APIs, provider status derivation, and visible task-row output remain unchanged. + +This task preserves `AC-UI-SIDEBAR-TASK-ROW-PRESENTATION-001.8` and `.12` for pull-request +indicators. The plugin portion is an internal render refinement with no public contract change. + +## TDD sequence + +1. Add a failing plugin render-count test for a session-object update with unchanged identifiers. +2. Add a failing hook identity test for unchanged pull-request hydration inputs. +3. Add a failing pull-request icon test for the stable empty-list path. +4. Stabilize each derived value at its owning component or hook boundary. +5. Run the focused tests, typecheck, and frontend lint. + +## Verification + +```bash +cd apps +rtk pnpm --filter @kandev/web test components/task/task-top-bar-plugin-actions.test.tsx hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx components/github/pr-task-icon.render.test.tsx +rtk pnpm --filter @kandev/web typecheck +rtk pnpm --filter @kandev/web lint +``` + +## Files likely touched + +- `apps/web/components/task/task-top-bar-plugin-actions.tsx` +- `apps/web/components/task/task-top-bar-plugin-actions.test.tsx` +- `apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.ts` +- `apps/web/hooks/domains/github/use-task-pr-tooltip-hydration.test.tsx` +- `apps/web/components/github/pr-task-icon.tsx` +- `apps/web/components/github/pr-task-icon.render.test.tsx` + +## Dependencies + +- Task 03 completes sidebar callback stabilization before shared row indicators change. + +## Inputs + +- `REQ-UI-SIDEBAR-TASK-ROW-PRESENTATION-001` +- Task Surface Render Isolation system design +- Existing plugin host API and pull-request status behavior +- Trace evidence for equal `sessionIds`, hydration, and pull-request props + +## Output contract + +Report each stable-value strategy, render-count results, changed files, focused test results, final +frontend checks, and remaining risks. Do not change plugin or integration contracts. diff --git a/docs/specs/ui/README.md b/docs/specs/ui/README.md index c51ed3732b..fefd2828e2 100644 --- a/docs/specs/ui/README.md +++ b/docs/specs/ui/README.md @@ -175,6 +175,7 @@ UI owns responsive behavior; other systems own behavior/state. - [Resizable Markdown Table Columns](system-design/resizable-markdown-tables.md) - [Session](system-design/session-start-composer-readiness.md) - [Task Layout Profiles](system-design/task-layout-profiles.md) +- [Task surface](system-design/task-surface-render-isolation.md) - [Task Agent Tab Reconciliation](system-design/task-agent-tab-reconciliation.md) - [Threads Deck](system-design/threads-conversation-deck.md) - [Command-panel Sidebar Task Reveal](system-design/command-panel-sidebar-task-reveal.md) diff --git a/docs/specs/ui/system-design/task-surface-render-isolation.md b/docs/specs/ui/system-design/task-surface-render-isolation.md new file mode 100644 index 0000000000..978dbad0ff --- /dev/null +++ b/docs/specs/ui/system-design/task-surface-render-isolation.md @@ -0,0 +1,164 @@ +--- +status: draft +system: ui +requirements: + - REQ-UI-FILE-TREE-CHAT-CONTEXT-001 + - REQ-UI-SIDEBAR-TASK-ROW-PRESENTATION-001 + - REQ-UI-MOBILE-TASK-NAVIGATION-001 +--- + +# Task Surface Render Isolation System Design + +## Purpose and boundaries + +The UI system owns render isolation for task surfaces with many repeated rows. This design covers +the file tree, sidebar task rows, pull-request indicators, and task-top-bar plugin contributions. + +The task, integration, and plugin systems still own their data and actions. This design does not +change their contracts, persistence, or user-visible behavior. + +The browser trace from 2026-08-31 showed three full render waves in 5.74 seconds. Each wave rendered +approximately 602 file rows and 48 sidebar rows. Layout, paint, and garbage collection were minor +costs, so this design targets React work and mounted component count. + +## Requirement mapping + +| Requirement | Design sections | +| --- | --- | +| `REQ-UI-FILE-TREE-CHAT-CONTEXT-001` | [File-tree rows](#file-tree-rows), [File-tree virtualization](#file-tree-virtualization), [Responsive behavior](#responsive-behavior) | +| `REQ-UI-SIDEBAR-TASK-ROW-PRESENTATION-001` | [Sidebar rows](#sidebar-rows), [Derived contribution props](#derived-contribution-props), [Responsive behavior](#responsive-behavior) | +| `REQ-UI-MOBILE-TASK-NAVIGATION-001` | [Responsive behavior](#responsive-behavior), [Failure and recovery](#failure-and-recovery) | + +## Components and responsibilities + +- `useFileBrowserTree` owns stable file-tree state and action identities. +- `FileTreeView` owns row-window calculation and the single file-tree scroll owner. +- `TreeNodeItem` owns one file or directory row and skips semantically unchanged renders. +- `FileContextMenu` preserves the established pointer, keyboard, and touch actions for mounted rows. +- `useSidebarSelection` owns stable selection actions for the task switcher. +- `TaskSwitcher` and `TaskRow` skip work when their rendered inputs do not change. +- `TaskTopBarPluginActions` derives stable session identifiers for plugin slot props. +- `useTaskPRTooltipHydration` returns a stable value while its status and actions do not change. +- `PRTaskIcon` reuses stable empty and unchanged pull-request inputs. + +## Render identity contract + +A parent render must not change a row callback when the callback behavior is unchanged. A hook must +not return a new aggregate object when all fields retain the same identity and value. + +List rows use memoized component boundaries. Each row receives the smallest required state. A row +rerenders when its node, selection, expansion, active-file, drag, or action state changes. + +Render-count tests prove identity behavior. The tests use controlled rerenders and do not use +elapsed-time thresholds. + +## File-tree rows + +File-tree actions read changing tree data from stable refs or functional state transitions. The +callbacks do not depend on the complete `treeState` aggregate object. + +`TreeNodeItem` uses a memoized boundary with explicit, stable inputs. Selection or expansion of one +node does not rerender unrelated visible rows. File opening, drag and drop, rename, download, +deletion, multi-selection, and chat-context actions keep their current behavior. + +Each mounted row keeps its own context-menu trigger. A shared menu is not required for this change. +Virtualization limits the number of mounted menu providers. + +## File-tree virtualization + +`FileTreeView` uses the existing `@tanstack/react-virtual` dependency. The flattened `visibleRows` +array remains the source for order, nesting depth, expansion, and filtering. + +The existing file-tree viewport remains the only vertical scroll owner. The virtualizer positions +only the current row window and a small overscan window inside that viewport. + +Rows use measured sizes because editing controls and touch layouts can change row height. The +virtualizer uses an estimated size before measurement. It updates positions after a measured size +changes. + +Active-file reveal and restored scroll operations resolve a row index from `visibleRows`. They use +the virtualizer scroll API before they query a row element. This sequence makes an offscreen row +available before focus or selection work uses its element. + +## Sidebar rows + +`useSidebarSelection` derives `onBulkMove` from stable action functions and current selection data. +It does not depend on a newly allocated selection aggregate. + +`TaskSwitcher` receives stable callbacks during unrelated parent renders. A memoized task row can +then isolate row work when task data and row state do not change. + +The task row remains the primary mobile tap target. The visible mobile action and fine-pointer +hover behavior remain unchanged. + +## Derived contribution props + +Plugin slot props use a stable session-identifier array while the ordered identifiers do not +change. Session status updates cannot rerender the plugin contribution when its slot input stays +equal. + +Pull-request tooltip hydration returns one stable aggregate while its status, hydration action, +and automation options stay equal. Pull-request indicators also reuse one empty array constant. + +These changes do not alter plugin host APIs or integration data. They only stabilize values at UI +component boundaries. + +## Control flow + +1. A store or local state update renders a task-surface owner. +2. Stable selectors and callbacks preserve identities for unchanged inputs. +3. Memoized boundaries skip unchanged rows and contributions. +4. The file-tree virtualizer calculates the required row window from the scroll position. +5. React renders only changed components inside the active row window. + +## Responsive behavior + +Desktop and mobile use the same flattened file-tree model and virtual row window. The layout does +not create a second vertical or horizontal scroll owner. + +Each mounted mobile file row keeps its visible action trigger and 44 CSS pixel touch target. The +responsive menu stays inside the viewport and keeps its safe-area behavior. + +Each mobile sidebar row remains the primary navigation action. Its explicit task menu stays visible +and touch-reachable. Virtualization does not change the sidebar composition. + +## Failure and recovery + +If row measurement is unavailable, the virtualizer uses the estimated row size. If a target row is +offscreen, reveal logic scrolls to its index before it requests the element. + +An empty tree renders the current empty state. A changed filter or expansion state recalculates the +flat row list and virtual range from the current scroll owner. + +Identity optimizations must fail toward an extra render, not stale UI. Equality logic must include +every input that changes visible output or interaction behavior. + +## Persistence + +This design adds no persisted state. Existing expansion, selection, file, sidebar, plugin, and +pull-request state keeps its current owner and lifetime. + +## Security + +This design adds no trust boundary or data access. Virtual rows use the same authorized file and +task data as the current complete list. + +## Observability + +Unit tests record render counts for controlled updates. Browser E2E tests record the number of +mounted rows for a large generated file tree. + +Final performance evaluation uses a warmed production build without React DevTools or CPU profiler +startup in the measured window. The report includes main-thread work, long tasks, and mounted-row +counts. Development traces remain diagnostic evidence only. + +## Related decisions + +No ADR is required. This design applies the repository's existing virtual-list dependency and UI +ownership model. + +## Related designs + +- [Sidebar Task Row Presentation](sidebar-task-row-presentation.md) +- [PR Task Status Summary](pr-task-status-summary.md) +- [Mobile Task Chrome](mobile-task-chrome.md)