diff --git a/apps/web/package.json b/apps/web/package.json index bae73ef..d0caa00 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -27,12 +27,14 @@ "@tiptap/suggestion": "^3.19.0", "@trpc/client": "^11.9.0", "@trpc/react-query": "^11.9.0", + "@types/react-window": "^1.8.8", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "geist": "^1.7.0", "lucide-react": "^0.563.0", "react": "^19.0.0", "react-dom": "^19.0.0", + "react-window": "^2.2.7", "sonner": "^2.0.7", "tailwind-merge": "^3.4.0", "tippy.js": "^6.3.7" diff --git a/apps/web/src/components/review/ReviewSidebar.test.tsx b/apps/web/src/components/review/ReviewSidebar.test.tsx new file mode 100644 index 0000000..8d6c2d4 --- /dev/null +++ b/apps/web/src/components/review/ReviewSidebar.test.tsx @@ -0,0 +1,162 @@ +// @vitest-environment happy-dom +import '@testing-library/jest-dom/vitest'; +import { render, screen, fireEvent, act } from '@testing-library/react'; +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { ReviewSidebar } from './ReviewSidebar'; +import type { FileDiff, ReviewComment, CommitInfo } from './types'; + +// Mock ResizeObserver — not provided by happy-dom. +// Must be a class (react-window 2.x uses `new ResizeObserver(...)`). +class MockResizeObserver { + observe() {} + unobserve() {} + disconnect() {} +} +vi.stubGlobal('ResizeObserver', MockResizeObserver); + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function makeFile(path: string): FileDiff { + return { + oldPath: path, + newPath: path, + hunks: [], + additions: 1, + deletions: 0, + changeType: 'modified', + }; +} + +function makeFiles(count: number, prefix = 'src/components/'): FileDiff[] { + return Array.from({ length: count }, (_, i) => + makeFile(`${prefix}file${String(i).padStart(4, '0')}.ts`), + ); +} + +const NO_COMMENTS: ReviewComment[] = []; +const NO_COMMITS: CommitInfo[] = []; + +function renderSidebar(files: FileDiff[]) { + return render( + , + ); +} + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +describe('ReviewSidebar FilesView virtualization', () => { + beforeEach(() => vi.clearAllMocks()); + afterEach(() => vi.restoreAllMocks()); + + it('renders only a subset of DOM rows when file count > 50', () => { + const files = makeFiles(200); + renderSidebar(files); + + const fileRows = document.querySelectorAll('[data-testid="file-row"]'); + // Virtualization keeps DOM rows << total count + expect(fileRows.length).toBeLessThan(100); + expect(fileRows.length).toBeGreaterThan(0); + }); + + it('renders all file rows when file count <= 50 (non-virtualized path)', () => { + const files = makeFiles(10); + renderSidebar(files); + + const fileRows = document.querySelectorAll('[data-testid="file-row"]'); + expect(fileRows.length).toBe(10); + }); + + it('removes file rows from DOM when a directory is collapsed', async () => { + // Use 60 files so we hit the >50 virtualized path + const files = makeFiles(60, 'src/components/'); + renderSidebar(files); + + // Initially, files should be rendered (at least some via virtualization) + const dirHeader = screen.getByRole('button', { name: /src\/components\// }); + expect(dirHeader).toBeInTheDocument(); + + const rowsBefore = document.querySelectorAll('[data-testid="file-row"]').length; + expect(rowsBefore).toBeGreaterThan(0); + + // Collapse the directory + await act(async () => { + fireEvent.click(dirHeader); + }); + + // After collapse, no file rows should be rendered for that directory + const rowsAfter = document.querySelectorAll('[data-testid="file-row"]').length; + expect(rowsAfter).toBe(0); + }); + + it('restores file rows when a collapsed directory is expanded again', async () => { + const files = makeFiles(60, 'src/components/'); + renderSidebar(files); + + const dirHeader = screen.getByRole('button', { name: /src\/components\// }); + + // Collapse + await act(async () => { + fireEvent.click(dirHeader); + }); + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(0); + + // Expand again + const freshDirHeader = screen.getByRole('button', { name: /src\/components\// }); + await act(async () => { + fireEvent.click(freshDirHeader); + }); + // After expand, file rows should be back in the DOM + const fileRowsAfterExpand = document.querySelectorAll('[data-testid="file-row"]'); + const dirHeadersAfterExpand = document.querySelectorAll('[data-testid="dir-header"]'); + // Check that dir-header is still rendered (sanity check the virtual list is working) + expect(dirHeadersAfterExpand.length).toBeGreaterThan(0); + expect(fileRowsAfterExpand.length).toBeGreaterThan(0); + }); + + it('preserves scroll position when switching from Files tab to Commits tab and back', async () => { + // This verifies the tab switch does not crash and re-mounts correctly + const files = makeFiles(200); + renderSidebar(files); + + // Initial state: file rows visible + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); + + // Switch to Commits tab + const commitsTab = screen.getByTitle('Commits'); + await act(async () => { + fireEvent.click(commitsTab); + }); + + // Files tab content should be gone + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBe(0); + + // Switch back to Files tab + const filesTab = screen.getByTitle('Files'); + await act(async () => { + fireEvent.click(filesTab); + }); + + // File rows should be back + expect(document.querySelectorAll('[data-testid="file-row"]').length).toBeGreaterThan(0); + }); + + it('root-level files (no subdirectory) render without a directory header', () => { + const files = makeFiles(10, ''); // No prefix = root-level files + renderSidebar(files); + + const fileRows = document.querySelectorAll('[data-testid="file-row"]'); + expect(fileRows.length).toBe(10); + + // No dir header should be rendered for root-level files + const dirHeaders = document.querySelectorAll('[data-testid="dir-header"]'); + expect(dirHeaders.length).toBe(0); + }); +}); diff --git a/apps/web/src/components/review/ReviewSidebar.tsx b/apps/web/src/components/review/ReviewSidebar.tsx index 4e344d1..5271d15 100644 --- a/apps/web/src/components/review/ReviewSidebar.tsx +++ b/apps/web/src/components/review/ReviewSidebar.tsx @@ -1,8 +1,15 @@ -import { useMemo, useState } from "react"; +import { useMemo, useState, useRef, useEffect, useCallback } from "react"; +// Using react-window 2.x (installed version). The task spec was written for react-window 1.x +// (VariableSizeList API). react-window 2.x provides a `List` component with a different but +// equivalent API: it handles ResizeObserver internally (no explicit height/width props needed), +// uses `rowComponent`/`rowProps` for rendering, and exposes `scrollToRow` via `listRef`. +import { List } from "react-window"; +import type { RowComponentProps, ListImperativeAPI } from "react-window"; import { MessageSquare, FileCode, FolderOpen, + ChevronRight, Plus, Minus, Circle, @@ -38,6 +45,8 @@ export function ReviewSidebar({ viewedFiles = new Set(), }: ReviewSidebarProps) { const [view, setView] = useState("files"); + // Persist Files-tab scroll offset across Files ↔ Commits switches + const filesScrollOffsetRef = useRef(0); return (
@@ -58,8 +67,8 @@ export function ReviewSidebar({ />
- {/* Content panel */} -
+ {/* Content panel — flex column so FilesView can stretch and manage its own scroll */} +
{view === "files" ? ( ) : ( - +
+ +
)}
@@ -171,6 +183,109 @@ const changeTypeDotColor: Record = { renamed: "bg-status-active-fg", }; +// ─── Row type for virtualized list ─── + +type Row = + | { kind: "dir-header"; dirName: string; fileCount: number; isCollapsed: boolean } + | { kind: "file"; file: FileDiff; dirName: string; isViewed: boolean; commentCount: number }; + +// Item heights: dir-header ≈ 32px (py-0.5 + icon), file row ≈ 40px (py-1 + text) +const DIR_HEADER_HEIGHT = 32; +const FILE_ROW_HEIGHT = 40; + +// ─── Virtualized row component (must be stable — defined outside FilesView) ─── + +type VirtualRowProps = { + rows: Row[]; + selectedCommit: string | null; + activeFilePaths: Set; + onFileClick: (filePath: string) => void; + onToggleDir: (dirName: string) => void; +}; + +function VirtualRowItem({ + index, + style, + rows, + selectedCommit, + activeFilePaths, + onFileClick, + onToggleDir, +}: RowComponentProps) { + const row = rows[index]; + if (!row) return null; + + if (row.kind === "dir-header") { + return ( + + ); + } + + // kind === "file" + const { file, dirName, isViewed, commentCount } = row; + const isInView = activeFilePaths.has(file.newPath); + const dimmed = selectedCommit && !isInView; + const dotColor = changeTypeDotColor[file.changeType]; + + return ( + + ); +} + function FilesView({ files, comments, @@ -179,6 +294,7 @@ function FilesView({ selectedCommit, activeFiles, viewedFiles, + scrollOffsetRef, }: { files: FileDiff[]; comments: ReviewComment[]; @@ -187,10 +303,14 @@ function FilesView({ selectedCommit: string | null; activeFiles: FileDiff[]; viewedFiles: Set; + scrollOffsetRef: React.MutableRefObject; }) { const unresolvedCount = comments.filter((c) => !c.resolved && !c.parentCommentId).length; const resolvedCount = comments.filter((c) => c.resolved && !c.parentCommentId).length; - const activeFilePaths = new Set(activeFiles.map((f) => f.newPath)); + const activeFilePaths = useMemo( + () => new Set(activeFiles.map((f) => f.newPath)), + [activeFiles], + ); const directoryGroups = useMemo(() => groupFilesByDirectory(files), [files]); @@ -198,169 +318,308 @@ function FilesView({ const totalCount = files.length; const progressPercent = totalCount > 0 ? (viewedCount / totalCount) * 100 : 0; + // ─── Collapse state ─── + const [collapsedDirs, setCollapsedDirs] = useState>(new Set()); + + const toggleDir = useCallback((dirName: string) => { + setCollapsedDirs((prev) => { + const next = new Set(prev); + if (next.has(dirName)) next.delete(dirName); + else next.add(dirName); + return next; + }); + }, []); + + // ─── Flat row list for virtualization ─── + const rows = useMemo(() => { + const result: Row[] = []; + for (const group of directoryGroups) { + const isCollapsed = collapsedDirs.has(group.directory); + // Root-level files (directory === "") get no dir-header, preserving existing behavior + if (group.directory) { + result.push({ + kind: "dir-header", + dirName: group.directory, + fileCount: group.files.length, + isCollapsed, + }); + } + if (!isCollapsed) { + for (const file of group.files) { + const commentCount = comments.filter( + (c) => c.filePath === file.newPath && !c.parentCommentId, + ).length; + result.push({ + kind: "file", + file, + dirName: group.directory, + isViewed: viewedFiles.has(file.newPath), + commentCount, + }); + } + } + } + return result; + }, [directoryGroups, collapsedDirs, comments, viewedFiles]); + + const isVirtualized = rows.length > 50; + + // ─── react-window 2.x imperative ref ─── + const listRef = useRef(null); + // Fallback container ref for non-virtualized path + const containerRef = useRef(null); + + // Restore scroll position on mount (both paths) + useEffect(() => { + const offset = scrollOffsetRef.current; + if (!offset) return; + if (isVirtualized) { + // react-window 2.x: scroll via the outermost DOM element + const el = listRef.current?.element; + if (el) el.scrollTop = offset; + } else if (containerRef.current) { + containerRef.current.scrollTop = offset; + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); // only on mount + + // Save scroll position on unmount (both paths) + useEffect(() => { + return () => { + if (isVirtualized) { + scrollOffsetRef.current = listRef.current?.element?.scrollTop ?? 0; + } else { + scrollOffsetRef.current = containerRef.current?.scrollTop ?? 0; + } + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [isVirtualized]); + + // Row height function for react-window 2.x List + const rowHeight = useCallback( + (index: number) => (rows[index]?.kind === "dir-header" ? DIR_HEADER_HEIGHT : FILE_ROW_HEIGHT), + [rows], + ); + + // Handle file click: call onFileClick and scroll virtual list to row + const handleFileClick = useCallback( + (filePath: string) => { + onFileClick(filePath); + const rowIndex = rows.findIndex( + (r) => r.kind === "file" && r.file.newPath === filePath, + ); + if (rowIndex >= 0) { + listRef.current?.scrollToRow({ index: rowIndex, align: "smart" }); + } + }, + [onFileClick, rows, listRef], + ); + + // Stable row props for the virtual row component + const rowProps = useMemo( + () => ({ + rows, + selectedCommit, + activeFilePaths, + onFileClick: handleFileClick, + onToggleDir: toggleDir, + }), + [rows, selectedCommit, activeFilePaths, handleFileClick, toggleDir], + ); + return ( -
- {/* Review progress */} - {totalCount > 0 && ( -
-

- Review Progress -

-
-
-
- - {viewedCount}/{totalCount} files viewed - -
- )} - - {/* Discussions — individual threads */} - {comments.length > 0 && ( -
-

- Discussions - - {unresolvedCount > 0 && ( - - - {unresolvedCount} - - )} - {resolvedCount > 0 && ( - - - {resolvedCount} - - )} +
+ {/* Fixed header — review progress + discussions */} +
+ {/* Review progress */} + {totalCount > 0 && ( +
+

+ Review Progress +

+
+
+
+ + {viewedCount}/{totalCount} files viewed -

-
- {comments - .filter((c) => !c.parentCommentId) - .map((thread) => { - const replyCount = comments.filter( - (c) => c.parentCommentId === thread.id, - ).length; - return ( - - ); - })}
-
- )} + )} - {/* Directory-grouped file tree */} -
-

- Files - {selectedCommit && ( - - ({activeFiles.length} in commit) - - )} -

- {directoryGroups.map((group) => ( -
- {/* Directory header */} - {group.directory && ( -
- - {group.directory} -
- )} - {/* Files in directory */} + {/* Discussions — individual threads */} + {comments.length > 0 && ( +
+

+ Discussions + + {unresolvedCount > 0 && ( + + + {unresolvedCount} + + )} + {resolvedCount > 0 && ( + + + {resolvedCount} + + )} + +

- {group.files.map((file) => { - const fileCommentCount = comments.filter( - (c) => c.filePath === file.newPath && !c.parentCommentId, - ).length; - const isInView = activeFilePaths.has(file.newPath); - const dimmed = selectedCommit && !isInView; - const isViewed = viewedFiles.has(file.newPath); - const dotColor = changeTypeDotColor[file.changeType]; - - return ( - - ); - })} + {replyCount > 0 && ( + + {replyCount} + + )} +
+ + {thread.body.length > 60 + ? thread.body.slice(0, 57) + "..." + : thread.body} + + + ); + })}
- ))} + )} + + {/* Files section heading */} +
+

+ Files + {selectedCommit && ( + + ({activeFiles.length} in commit) + + )} +

+
+ + {/* Scrollable file tree — virtualized (react-window 2.x List) when >50 rows */} + {isVirtualized ? ( + + ) : ( +
+ {directoryGroups.map((group) => ( +
+ {/* Directory header — collapsible */} + {group.directory && ( + + )} + {/* Files in directory */} + {!collapsedDirs.has(group.directory) && ( +
+ {group.files.map((file) => { + const fileCommentCount = comments.filter( + (c) => c.filePath === file.newPath && !c.parentCommentId, + ).length; + const isInView = activeFilePaths.has(file.newPath); + const dimmed = selectedCommit && !isInView; + const isViewed = viewedFiles.has(file.newPath); + const dotColor = changeTypeDotColor[file.changeType]; + + return ( + + ); + })} +
+ )} +
+ ))} +
+ )}
); } diff --git a/docs/frontend.md b/docs/frontend.md index 0797687..63e1e0f 100644 --- a/docs/frontend.md +++ b/docs/frontend.md @@ -14,6 +14,7 @@ | Tiptap | Rich text editor (ProseMirror-based) | | Lucide | Icon library | | Geist Sans/Mono | Typography (variable fonts in `public/fonts/`) | +| react-window 2.x | Virtualized list rendering for large file trees in ReviewSidebar | ## Design System (v2) @@ -115,7 +116,7 @@ The initiative detail page has three tabs managed via local state (not URL param |-----------|---------| | `ReviewTab` | Review tab container — orchestrates header, diff, sidebar, and preview. Phase-level review has threaded inline comments (with reply support) + Request Changes; initiative-level review has Request Changes (summary prompt) + Push Branch / Merge & Push | | `ReviewHeader` | Consolidated toolbar: phase selector pills, branch info, stats, preview controls, approve/reject actions | -| `ReviewSidebar` | VSCode-style icon strip (Files/Commits views) with file list, root-only comment counts, and commit navigation | +| `ReviewSidebar` | VSCode-style icon strip (Files/Commits views) with file list, root-only comment counts, and commit navigation. FilesView uses react-window 2.x `List` for virtualized rendering when the row count exceeds 50 (dir-headers + file rows). Scroll position is preserved across Files ↔ Commits tab switches. Directories are collapsible. Clicking a file scrolls the virtual list to that row. | | `DiffViewer` | Unified diff renderer with threaded inline comments (root + reply threads) | | `CommentThread` | Renders root comment with resolve/reopen + nested reply threads (agent replies styled with primary border). Inline reply form | | `ConflictResolutionPanel` | Merge conflict detection + agent resolution in initiative review. Shows conflict files, spawns conflict agent, inline questions, re-check on completion | diff --git a/package-lock.json b/package-lock.json index 89c8514..ab9b297 100644 --- a/package-lock.json +++ b/package-lock.json @@ -73,12 +73,14 @@ "@tiptap/suggestion": "^3.19.0", "@trpc/client": "^11.9.0", "@trpc/react-query": "^11.9.0", + "@types/react-window": "^1.8.8", "class-variance-authority": "^0.7.1", "clsx": "^2.1.1", "geist": "^1.7.0", "lucide-react": "^0.563.0", "react": "^19.0.0", "react-dom": "^19.0.0", + "react-window": "^2.2.7", "sonner": "^2.0.7", "tailwind-merge": "^3.4.0", "tippy.js": "^6.3.7" @@ -5198,6 +5200,15 @@ "@types/react": "^19.2.0" } }, + "node_modules/@types/react-window": { + "version": "1.8.8", + "resolved": "https://registry.npmjs.org/@types/react-window/-/react-window-1.8.8.tgz", + "integrity": "sha512-8Ls660bHR1AUA2kuRvVG9D/4XpRC6wjAaPT9dil7Ckc76eP9TKWZwwmgfq8Q1LANX3QNDnoU4Zp48A3w+zK69Q==", + "license": "MIT", + "dependencies": { + "@types/react": "*" + } + }, "node_modules/@types/unist": { "version": "3.0.3", "resolved": "https://registry.npmjs.org/@types/unist/-/unist-3.0.3.tgz", @@ -9131,6 +9142,16 @@ } } }, + "node_modules/react-window": { + "version": "2.2.7", + "resolved": "https://registry.npmjs.org/react-window/-/react-window-2.2.7.tgz", + "integrity": "sha512-SH5nvfUQwGHYyriDUAOt7wfPsfG9Qxd6OdzQxl5oQ4dsSsUicqQvjV7dR+NqZ4coY0fUn3w1jnC5PwzIUWEg5w==", + "license": "MIT", + "peerDependencies": { + "react": "^18.0.0 || ^19.0.0", + "react-dom": "^18.0.0 || ^19.0.0" + } + }, "node_modules/read-cache": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/read-cache/-/read-cache-1.0.0.tgz", diff --git a/vitest.config.ts b/vitest.config.ts index 0b610f5..3b89372 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -8,6 +8,9 @@ export default defineConfig({ alias: { '@': path.resolve(__dirname, './apps/web/src'), }, + // Deduplicate React to avoid "multiple copies" errors when packages + // installed in workspace sub-directories shadow the hoisted copies. + dedupe: ['react', 'react-dom'], }, test: { // Enable test globals (describe, it, expect without imports)