From 8eb0850c51482a5ad652a51455625df454d12374 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 2 Jun 2026 17:07:00 +0100 Subject: [PATCH] feat(web): resizable review dialog sidebar with persistence (#1245) --- apps/web/app/layout.tsx | 9 + apps/web/components/review/review-dialog.tsx | 26 ++- .../review/review-sidebar-resize.spec.ts | 154 +++++++++++++++ .../hooks/use-review-sidebar-resize.test.ts | 184 ++++++++++++++++++ apps/web/hooks/use-review-sidebar-resize.ts | 128 ++++++++++++ 5 files changed, 498 insertions(+), 3 deletions(-) create mode 100644 apps/web/e2e/tests/review/review-sidebar-resize.spec.ts create mode 100644 apps/web/hooks/use-review-sidebar-resize.test.ts create mode 100644 apps/web/hooks/use-review-sidebar-resize.ts diff --git a/apps/web/app/layout.tsx b/apps/web/app/layout.tsx index 31a6159ce9..f063936b65 100644 --- a/apps/web/app/layout.tsx +++ b/apps/web/app/layout.tsx @@ -52,6 +52,15 @@ export default async function RootLayout({ + {/* Preload the Seti icon webfont so file-tree glyphs (review dialog, + file browser) don't flash blank on first render. */} + {apiPort || debugMode ? ( diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index 831c03e28a..50e2036c19 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -8,6 +8,7 @@ import type { PRDiffFile } from "@/lib/types/github"; import { useCommentsStore, isDiffComment } from "@/lib/state/slices/comments"; import { useSessionFileReviews } from "@/hooks/use-session-file-reviews"; import { useGitOperations } from "@/hooks/use-git-operations"; +import { useReviewSidebarResize } from "@/hooks/use-review-sidebar-resize"; import { useAppStore } from "@/components/state-provider"; import { useToast } from "@/components/toast-provider"; import { ReviewTopBar } from "./review-top-bar"; @@ -418,13 +419,18 @@ function useReviewDialogState(props: ReviewDialogProps) { export const ReviewDialog = memo(function ReviewDialog(props: ReviewDialogProps) { const { open, onOpenChange, sessionId, baseBranch, onOpenFile } = props; const s = useReviewDialogState(props); + const splitRowRef = useRef(null); + const sidebar = useReviewSidebarResize(splitRowRef, open); return ( Review Changes -
-
+
+
+
{s.filteredFiles.length > 0 ? ( window.dispatchEvent(new CustomEvent("open-review-dialog"))); + const dialog = testPage.getByRole("dialog", { name: "Review Changes" }); + await expect(dialog).toBeVisible({ timeout: 10_000 }); + return dialog; +} + +async function getSidebarWidth(sidebar: Locator): Promise { + const box = await sidebar.boundingBox(); + if (!box) throw new Error("sidebar has no bounding box"); + return Math.round(box.width); +} + +// boundingBox returns floats and sub-pixel rounding can shift the measured +// width by a pixel across renderers, so compare with a small tolerance. +function expectWidthNear(actual: number, expected: number, tolerance = 1) { + expect(actual).toBeGreaterThanOrEqual(expected - tolerance); + expect(actual).toBeLessThanOrEqual(expected + tolerance); +} + +async function dragHandle(testPage: Page, handle: Locator, deltaX: number) { + const box = await handle.boundingBox(); + if (!box) throw new Error("resize handle has no bounding box"); + const cx = box.x + box.width / 2; + const cy = box.y + box.height / 2; + await testPage.mouse.move(cx, cy); + await testPage.mouse.down(); + await testPage.mouse.move(cx + deltaX, cy, { steps: 10 }); + await testPage.mouse.up(); +} + +test.describe("Review dialog sidebar resize", () => { + test.describe.configure({ retries: 2, timeout: 120_000 }); + + test("dragging the handle resizes the sidebar and persists to sessionStorage", async ({ + testPage, + apiClient, + seedData, + }) => { + await seedReviewTask(testPage, apiClient, seedData); + const dialog = await openDialogWithChanges(testPage); + + const sidebar = dialog.getByTestId("review-dialog-sidebar"); + const handle = dialog.getByTestId("review-dialog-sidebar-resize"); + await expect(sidebar).toBeVisible(); + await expect(handle).toBeVisible(); + + const before = await getSidebarWidth(sidebar); + expectWidthNear(before, 220); // default width + + await dragHandle(testPage, handle, 120); // drag right by 120px + + await expect + .poll(async () => getSidebarWidth(sidebar), { timeout: 5_000 }) + .toBeGreaterThan(before + 80); + const after = await getSidebarWidth(sidebar); + + // sessionStorage persistence (within 1px of the rendered width) + const stored = await testPage.evaluate( + (key) => window.sessionStorage.getItem(key), + STORAGE_KEY, + ); + expect(stored).not.toBeNull(); + expectWidthNear(Number(stored), after); + }); + + test("clamps to max width when dragged far past the limit", async ({ + testPage, + apiClient, + seedData, + }) => { + await seedReviewTask(testPage, apiClient, seedData); + const dialog = await openDialogWithChanges(testPage); + const sidebar = dialog.getByTestId("review-dialog-sidebar"); + const handle = dialog.getByTestId("review-dialog-sidebar-resize"); + + await dragHandle(testPage, handle, 2000); // way past max=600 + + // At default Playwright viewport (~1280px) the dialog is ~1024px, so the + // (containerWidth - minDiffPaneWidth) clamp (~704) is wider than the + // hard max (600); we should land on the hard max. + await expect + .poll(async () => getSidebarWidth(sidebar), { timeout: 5_000 }) + .toBeGreaterThanOrEqual(599); + const finalWidth = await getSidebarWidth(sidebar); + expectWidthNear(finalWidth, 600); + }); + + test("persisted width is reapplied when the dialog is reopened in the same tab", async ({ + testPage, + apiClient, + seedData, + }) => { + // Pre-seed sessionStorage via addInitScript so the value is written + // before any page script runs. The ReviewDialog is mounted as soon as + // the session layout renders (see dockview-desktop-layout.tsx) — well + // before this test could `page.evaluate` after navigation — so the + // hook's useState initializer reads the seeded value on first mount. + await testPage.addInitScript( + ({ key, val }) => { + try { + sessionStorage.setItem(key, val); + } catch { + // sessionStorage may be unavailable in some contexts; ignore. + } + }, + { key: STORAGE_KEY, val: "340" }, + ); + await seedReviewTask(testPage, apiClient, seedData); + + const dialog = await openDialogWithChanges(testPage); + const sidebar = dialog.getByTestId("review-dialog-sidebar"); + await expect + .poll(async () => getSidebarWidth(sidebar), { timeout: 5_000 }) + .toBeGreaterThanOrEqual(339); + expectWidthNear(await getSidebarWidth(sidebar), 340); + }); +}); diff --git a/apps/web/hooks/use-review-sidebar-resize.test.ts b/apps/web/hooks/use-review-sidebar-resize.test.ts new file mode 100644 index 0000000000..bada8c8ae1 --- /dev/null +++ b/apps/web/hooks/use-review-sidebar-resize.test.ts @@ -0,0 +1,184 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useRef } from "react"; +import { + useReviewSidebarResize, + clampSidebarWidth, + REVIEW_SIDEBAR_LIMITS, +} from "./use-review-sidebar-resize"; + +function mouseMove(clientX: number) { + document.dispatchEvent(new MouseEvent("mousemove", { clientX, bubbles: true })); +} + +function mouseUp() { + document.dispatchEvent(new MouseEvent("mouseup", { bubbles: true })); +} + +function reactMouse(clientX: number) { + return { clientX, preventDefault: () => {} } as unknown as React.MouseEvent; +} + +describe("clampSidebarWidth", () => { + it("returns the value when within [min, max]", () => { + expect(clampSidebarWidth(250)).toBe(250); + }); + + it("clamps below min", () => { + expect(clampSidebarWidth(50)).toBe(REVIEW_SIDEBAR_LIMITS.minWidth); + }); + + it("clamps above max", () => { + expect(clampSidebarWidth(9999)).toBe(REVIEW_SIDEBAR_LIMITS.maxWidth); + }); + + it("clamps against (containerWidth - minDiffPaneWidth) when it bites first", () => { + // container 700 - minDiffPane 320 → effective max 380 + expect(clampSidebarWidth(500, 700)).toBe(700 - REVIEW_SIDEBAR_LIMITS.minDiffPaneWidth); + }); + + it("does not let containerWidth force effective max below min", () => { + // container 200 - minDiff 320 = -120 → floor at MIN_SIDEBAR_WIDTH + expect(clampSidebarWidth(500, 200)).toBe(REVIEW_SIDEBAR_LIMITS.minWidth); + }); + + it("does not let containerWidth raise effective max above hard max", () => { + // container 5000 - 320 = 4680 → capped by hard max + expect(clampSidebarWidth(9999, 5000)).toBe(REVIEW_SIDEBAR_LIMITS.maxWidth); + }); +}); + +describe("useReviewSidebarResize", () => { + beforeEach(() => { + window.sessionStorage.clear(); + document.body.style.cursor = ""; + document.body.style.userSelect = ""; + }); + + afterEach(() => { + document.body.style.cursor = ""; + document.body.style.userSelect = ""; + }); + + it("defaults to DEFAULT_SIDEBAR_WIDTH when sessionStorage is empty", () => { + const { result } = renderHook(() => useReviewSidebarResize()); + expect(result.current.width).toBe(REVIEW_SIDEBAR_LIMITS.defaultWidth); + expect(typeof result.current.resizeHandleProps.onMouseDown).toBe("function"); + }); + + it("reads a previously persisted width from sessionStorage on mount", () => { + window.sessionStorage.setItem(REVIEW_SIDEBAR_LIMITS.storageKey, "350"); + const { result } = renderHook(() => useReviewSidebarResize()); + expect(result.current.width).toBe(350); + }); + + it("clamps a persisted width that's outside [min, max] on mount", () => { + window.sessionStorage.setItem(REVIEW_SIDEBAR_LIMITS.storageKey, "9999"); + const { result } = renderHook(() => useReviewSidebarResize()); + expect(result.current.width).toBe(REVIEW_SIDEBAR_LIMITS.maxWidth); + }); + + it("falls back to default when sessionStorage contains a non-numeric value", () => { + window.sessionStorage.setItem(REVIEW_SIDEBAR_LIMITS.storageKey, "not-a-number"); + const { result } = renderHook(() => useReviewSidebarResize()); + expect(result.current.width).toBe(REVIEW_SIDEBAR_LIMITS.defaultWidth); + }); + + it("updates width live during mousemove and clamps the result", () => { + const { result } = renderHook(() => useReviewSidebarResize()); + act(() => result.current.resizeHandleProps.onMouseDown(reactMouse(300))); + // move right by 80px → 220 + 80 = 300 + act(() => mouseMove(380)); + expect(result.current.width).toBe(300); + + // overshoot past max → clamps to maxWidth + act(() => mouseMove(2000)); + expect(result.current.width).toBe(REVIEW_SIDEBAR_LIMITS.maxWidth); + }); + + it("persists final width to sessionStorage on mouseup", () => { + const { result } = renderHook(() => useReviewSidebarResize()); + act(() => result.current.resizeHandleProps.onMouseDown(reactMouse(300))); + act(() => mouseMove(380)); // → 300 + act(() => mouseUp()); + + expect(window.sessionStorage.getItem(REVIEW_SIDEBAR_LIMITS.storageKey)).toBe("300"); + expect(result.current.width).toBe(300); + }); + + it("stops responding to mousemove after mouseup", () => { + const { result } = renderHook(() => useReviewSidebarResize()); + act(() => result.current.resizeHandleProps.onMouseDown(reactMouse(300))); + act(() => mouseMove(380)); + expect(result.current.width).toBe(300); + + act(() => mouseUp()); + act(() => mouseMove(500)); // would push to 420 if still listening + + expect(result.current.width).toBe(300); + }); + + it("sets body cursor + userSelect during drag and restores them on mouseup", () => { + const { result } = renderHook(() => useReviewSidebarResize()); + act(() => result.current.resizeHandleProps.onMouseDown(reactMouse(300))); + expect(document.body.style.cursor).toBe("col-resize"); + expect(document.body.style.userSelect).toBe("none"); + + act(() => mouseUp()); + expect(document.body.style.cursor).toBe(""); + expect(document.body.style.userSelect).toBe(""); + }); + + it("restores body styles when the component unmounts mid-drag", () => { + const { result, unmount } = renderHook(() => useReviewSidebarResize()); + act(() => result.current.resizeHandleProps.onMouseDown(reactMouse(300))); + expect(document.body.style.cursor).toBe("col-resize"); + + act(() => unmount()); + + expect(document.body.style.cursor).toBe(""); + expect(document.body.style.userSelect).toBe(""); + // After unmount, the now-detached window listeners must not be able to + // mutate any state — a stray mousemove should be a no-op. + act(() => mouseMove(800)); + // Nothing to assert on result.current after unmount, but the absence of + // a thrown error from the listener confirms cleanup ran. + }); + + it("clamps width against the container width when one is provided", () => { + // jsdom doesn't ship ResizeObserver; the initial reclamp still runs. + window.sessionStorage.setItem(REVIEW_SIDEBAR_LIMITS.storageKey, "500"); + const { result } = renderHook(() => { + const ref = useRef(null); + // Simulate a container element that reports a 600px width. + const fakeEl = { getBoundingClientRect: () => ({ width: 600 }) as DOMRect }; + (ref as React.MutableRefObject).current = + fakeEl as unknown as HTMLDivElement; + return useReviewSidebarResize(ref); + }); + // 600 - 320 = 280 → stored 500 should be clamped down to 280 + expect(result.current.width).toBe(600 - REVIEW_SIDEBAR_LIMITS.minDiffPaneWidth); + }); + + it("reclamps when the dialog transitions from closed to open", () => { + // Mirrors the real lifecycle: Radix doesn't mount the DialogContent + // portal while open=false, so containerRef.current is null on first + // effect run. When open flips to true, the effect must re-fire and + // pick up the now-populated element. + window.sessionStorage.setItem(REVIEW_SIDEBAR_LIMITS.storageKey, "500"); + const ref = { current: null } as React.MutableRefObject; + const { result, rerender } = renderHook(({ open }) => useReviewSidebarResize(ref, open), { + initialProps: { open: false }, + }); + // Closed → stored width returned unclamped by container (since no el). + expect(result.current.width).toBe(500); + + // Dialog opens: portal mounts, ref populates with a 600px container. + const fakeEl = { getBoundingClientRect: () => ({ width: 600 }) as DOMRect }; + ref.current = fakeEl as unknown as HTMLDivElement; + rerender({ open: true }); + + // Effect must re-run and reclamp to 600 - 320 = 280. + expect(result.current.width).toBe(600 - REVIEW_SIDEBAR_LIMITS.minDiffPaneWidth); + }); +}); diff --git a/apps/web/hooks/use-review-sidebar-resize.ts b/apps/web/hooks/use-review-sidebar-resize.ts new file mode 100644 index 0000000000..607497ee48 --- /dev/null +++ b/apps/web/hooks/use-review-sidebar-resize.ts @@ -0,0 +1,128 @@ +"use client"; + +import { useCallback, useEffect, useRef, useState } from "react"; + +const SIDEBAR_WIDTH_KEY = "review-dialog-sidebar-width"; +const DEFAULT_SIDEBAR_WIDTH = 220; +const MIN_SIDEBAR_WIDTH = 160; +const MAX_SIDEBAR_WIDTH = 600; +// Reserve at least this much horizontal space for the diff pane so a +// previously-saved 600px sidebar can't push the diff list to ~0 on a small +// desktop viewport (e.g. the dialog is 80vw → 512px at the sm breakpoint). +const MIN_DIFF_PANE_WIDTH = 320; + +export const REVIEW_SIDEBAR_LIMITS = { + storageKey: SIDEBAR_WIDTH_KEY, + defaultWidth: DEFAULT_SIDEBAR_WIDTH, + minWidth: MIN_SIDEBAR_WIDTH, + maxWidth: MAX_SIDEBAR_WIDTH, + minDiffPaneWidth: MIN_DIFF_PANE_WIDTH, +} as const; + +export function clampSidebarWidth(w: number, containerWidth?: number): number { + const effectiveMax = containerWidth + ? Math.max(MIN_SIDEBAR_WIDTH, Math.min(MAX_SIDEBAR_WIDTH, containerWidth - MIN_DIFF_PANE_WIDTH)) + : MAX_SIDEBAR_WIDTH; + return Math.max(MIN_SIDEBAR_WIDTH, Math.min(effectiveMax, w)); +} + +function readStoredWidth(): number { + if (typeof window === "undefined") return DEFAULT_SIDEBAR_WIDTH; + try { + const stored = window.sessionStorage.getItem(SIDEBAR_WIDTH_KEY); + const parsed = stored ? Number(stored) : NaN; + return Number.isFinite(parsed) ? clampSidebarWidth(parsed) : DEFAULT_SIDEBAR_WIDTH; + } catch { + return DEFAULT_SIDEBAR_WIDTH; + } +} + +function persistWidth(width: number) { + try { + window.sessionStorage.setItem(SIDEBAR_WIDTH_KEY, String(width)); + } catch { + // sessionStorage may be unavailable (private mode, quota); ignore. + } +} + +function restoreBodyStyles() { + document.body.style.cursor = ""; + document.body.style.userSelect = ""; +} + +export function useReviewSidebarResize( + containerRef?: React.RefObject, + open = true, +) { + const [width, setWidth] = useState(readStoredWidth); + const isDragging = useRef(false); + const startX = useRef(0); + const startWidth = useRef(0); + + // Re-clamp when the container becomes available or resizes so a stored + // width carried over from a wider viewport gets pulled down on a narrower + // one (rather than overflowing the dialog). `open` is in the dep array + // because the container element only exists once Radix mounts the + // DialogContent portal — on the closed → open transition, `containerRef` + // (a stable ref *object*) doesn't change identity, so the effect needs + // `open` to re-fire and pick up the now-populated `current`. + useEffect(() => { + if (!open) return; + const el = containerRef?.current; + if (!el) return; + const reclamp = () => { + const cw = el.getBoundingClientRect().width; + setWidth((w) => clampSidebarWidth(w, cw)); + }; + reclamp(); + if (typeof ResizeObserver === "undefined") return; + const ro = new ResizeObserver(reclamp); + ro.observe(el); + return () => ro.disconnect(); + }, [containerRef, open]); + + const handleMouseDown = useCallback( + (e: React.MouseEvent) => { + e.preventDefault(); + isDragging.current = true; + startX.current = e.clientX; + startWidth.current = width; + document.body.style.cursor = "col-resize"; + document.body.style.userSelect = "none"; + }, + [width], + ); + + useEffect(() => { + const handleMouseMove = (e: MouseEvent) => { + if (!isDragging.current) return; + const cw = containerRef?.current?.getBoundingClientRect().width; + const delta = e.clientX - startX.current; + setWidth(clampSidebarWidth(startWidth.current + delta, cw)); + }; + const handleMouseUp = () => { + if (!isDragging.current) return; + isDragging.current = false; + restoreBodyStyles(); + setWidth((current) => { + persistWidth(current); + return current; + }); + }; + document.addEventListener("mousemove", handleMouseMove); + document.addEventListener("mouseup", handleMouseUp); + return () => { + document.removeEventListener("mousemove", handleMouseMove); + document.removeEventListener("mouseup", handleMouseUp); + if (isDragging.current) { + isDragging.current = false; + restoreBodyStyles(); + } + }; + }, [containerRef]); + + return { + width, + resizeHandleProps: { onMouseDown: handleMouseDown }, + }; +}