From eb1210a329904b799d37ac3334f51d4e204e02d7 Mon Sep 17 00:00:00 2001 From: Ammar Date: Thu, 7 May 2026 11:04:43 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20keep=20immersive=20sideba?= =?UTF-8?q?r=20review=20click=20on=20its=20hunk?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking a pending review in the immersive Notes sidebar called navigateToReview, which resolved the target hunk from allHunks and called onSelectHunk. ReviewPanel's selection-validity effect then clobbered the selection on the next render because it only validated against filteredHunks — and the review's hunk had typically been hidden by hide-read after the user submitted the review. The immersive view snapped back to the first visible hunk and the review was nowhere on screen. In immersive mode, validate the selected hunk against the unfiltered hunks so explicit sidebar navigations stick. The non-immersive panel path is unchanged because it only ever renders filteredHunks. Adds two regression tests in ImmersiveReviewView.test.tsx: a contract test for navigateToReview (catches regressions inside the immersive view) and a parent-harness integration test that mirrors ReviewPanel's selection-validity effect (catches regressions in the parent reset). --- .../CodeReview/ImmersiveReviewView.test.tsx | 205 ++++++++++++++++-- .../RightSidebar/CodeReview/ReviewPanel.tsx | 18 +- 2 files changed, 207 insertions(+), 16 deletions(-) diff --git a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx index 92049456a8..9a2eb6b801 100644 --- a/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ImmersiveReviewView.test.tsx @@ -1,11 +1,11 @@ import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; import { cleanup, fireEvent, render } from "@testing-library/react"; import { GlobalWindow } from "happy-dom"; -import type { ComponentProps } from "react"; +import { useEffect, useState, type ComponentProps } from "react"; import { ThemeProvider } from "@/browser/contexts/ThemeContext"; import type { FileTreeNode } from "@/common/utils/git/numstatParser"; -import type { DiffHunk } from "@/common/types/review"; +import type { DiffHunk, Review } from "@/common/types/review"; interface MockApiClient { workspace: { @@ -49,7 +49,10 @@ function createHunk(overrides: Partial = {}): DiffHunk { } function createFileTree(filePath: string): FileTreeNode { - const segments = filePath.split("/"); + return createFileTreeForPaths([filePath]); +} + +function createFileTreeForPaths(filePaths: string[]): FileTreeNode { const root: FileTreeNode = { name: "", path: "", @@ -57,18 +60,24 @@ function createFileTree(filePath: string): FileTreeNode { children: [], }; - let current = root; - for (const [index, segment] of segments.entries()) { - const isLastSegment = index === segments.length - 1; - const path = segments.slice(0, index + 1).join("/"); - const nextNode: FileTreeNode = { - name: segment, - path, - isDirectory: !isLastSegment, - children: [], - }; - current.children.push(nextNode); - current = nextNode; + for (const filePath of filePaths) { + const segments = filePath.split("/"); + let current = root; + for (const [index, segment] of segments.entries()) { + const isLastSegment = index === segments.length - 1; + const path = segments.slice(0, index + 1).join("/"); + let next = current.children.find((child) => child.path === path); + if (!next) { + next = { + name: segment, + path, + isDirectory: !isLastSegment, + children: [], + }; + current.children.push(next); + } + current = next; + } } return root; @@ -218,4 +227,170 @@ describe("ImmersiveReviewView", () => { expect(view.queryByTestId("immersive-review-complete")).toBeNull(); expect(view.getByText("No hunks for this file")).toBeTruthy(); }); + + test("clicking a sidebar review selects its hunk even when hidden by the active filter", () => { + // Repro for: clicking a pending review in the immersive sidebar should + // jump back to the hunk the review was attached to. Previously, when + // hide-read (or any other frontend filter) had removed the review's hunk + // from the visible list, the navigation handler still computed the right + // target hunk id from `allHunks` — but the parent panel reset the + // selection on the next render because it validated against the filtered + // hunks. Lock in the immersive contract by asserting the explicit target + // hunk id propagates out of `onSelectHunk`. + const visibleHunk = createHunk({ + id: "hunk-visible", + filePath: "src/visible.ts", + newStart: 1, + newLines: 1, + oldStart: 1, + oldLines: 1, + header: "@@ -1 +1 @@", + content: "-old visible\n+new visible", + }); + const reviewedHunk = createHunk({ + id: "hunk-reviewed", + filePath: "src/reviewed.ts", + newStart: 1, + newLines: 1, + oldStart: 1, + oldLines: 1, + header: "@@ -1 +1 @@", + content: "-old reviewed\n+new reviewed", + }); + const pendingReview: Review = { + id: "review-1", + data: { + filePath: reviewedHunk.filePath, + lineRange: "+1", + selectedCode: "// sample", + userNote: "Take another look here", + }, + status: "pending", + createdAt: 1000, + }; + const reviewsByFilePath = new Map([[reviewedHunk.filePath, [pendingReview]]]); + const onSelectHunk = mock((_hunkId: string | null) => undefined); + + const view = renderImmersiveReview({ + fileTree: createFileTreeForPaths([visibleHunk.filePath, reviewedHunk.filePath]), + // visibleHunk is the only currently-visible hunk (hide-read or search has + // removed reviewedHunk), but reviewedHunk still exists in the diff. + hunks: [visibleHunk], + allHunks: [visibleHunk, reviewedHunk], + isRead: (hunkId) => hunkId === reviewedHunk.id, + selectedHunkId: visibleHunk.id, + onSelectHunk, + reviewsByFilePath, + isTouchImmersive: false, + }); + + const noteCard = view.container.querySelector('[data-note-index="0"]'); + expect(noteCard).toBeTruthy(); + + onSelectHunk.mockClear(); + fireEvent.click(noteCard!); + + const selectedIds = onSelectHunk.mock.calls.map(([hunkId]) => hunkId); + expect(selectedIds).toContain(reviewedHunk.id); + // The view must not silently fall back to the first visible hunk. + expect(selectedIds).not.toEqual([visibleHunk.id]); + }); + + test("parent panel keeps the explicit sidebar selection in immersive mode after click", () => { + // End-to-end repro that mirrors how ReviewPanel hosts ImmersiveReviewView: + // selectedHunkId lives in the parent and a useEffect re-validates it + // whenever filtered hunks change. With the immersive-aware fix the parent + // only resets when the hunk vanishes from the diff entirely, so clicking a + // pending review for a hidden hunk keeps the immersive view on that hunk's + // file (instead of bouncing back to the first visible hunk). + const visibleHunk = createHunk({ + id: "hunk-visible", + filePath: "src/visible.ts", + newStart: 1, + newLines: 1, + oldStart: 1, + oldLines: 1, + header: "@@ -1 +1 @@", + content: "-old visible\n+new visible", + }); + const reviewedHunk = createHunk({ + id: "hunk-reviewed", + filePath: "src/reviewed.ts", + newStart: 1, + newLines: 1, + oldStart: 1, + oldLines: 1, + header: "@@ -1 +1 @@", + content: "-old reviewed\n+new reviewed", + }); + const pendingReview: Review = { + id: "review-1", + data: { + filePath: reviewedHunk.filePath, + lineRange: "+1", + selectedCode: "// sample", + userNote: "Take another look here", + }, + status: "pending", + createdAt: 1000, + }; + const reviewsByFilePath = new Map([[reviewedHunk.filePath, [pendingReview]]]); + + const filteredHunks = [visibleHunk]; + const allHunks = [visibleHunk, reviewedHunk]; + + function ParentPanelHarness() { + const [selectedHunkId, setSelectedHunkId] = useState(visibleHunk.id); + + // Mirrors ReviewPanel's selection-validity effect with the immersive + // branch. Keep the explicit selection even when it's been hidden by an + // active filter, since the immersive view supports rendering it from + // `allHunks`. Switching `allHunks.some` back to `filteredHunks.some` + // here reproduces the original bug and makes this test fail. + useEffect(() => { + if (filteredHunks.length === 0) return; + const selectionExists = + selectedHunkId && allHunks.some((hunk) => hunk.id === selectedHunkId); + if (!selectionExists) { + setSelectedHunkId(filteredHunks[0].id); + } + }, [selectedHunkId]); + + return ( + hunkId === reviewedHunk.id} + onToggleRead={mock(() => undefined)} + onMarkFileAsRead={mock(() => undefined)} + selectedHunkId={selectedHunkId} + onSelectHunk={setSelectedHunkId} + onExit={mock(() => undefined)} + isTouchImmersive={false} + reviewsByFilePath={reviewsByFilePath} + firstSeenMap={{}} + /> + ); + } + + const view = render( + + + + ); + + // Sanity-check the initial state: we start on the visible hunk's file. + expect(view.container.textContent ?? "").toContain(visibleHunk.filePath); + + const noteCard = view.container.querySelector('[data-note-index="0"]'); + expect(noteCard).toBeTruthy(); + fireEvent.click(noteCard!); + + // After the click the immersive header switches to the reviewed file — + // the parent panel must NOT have reset the selection back to the first + // visible hunk. + expect(view.container.textContent ?? "").toContain(reviewedHunk.filePath); + }); }); diff --git a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx index baf7e9f3af..703c6187e8 100644 --- a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx @@ -1248,14 +1248,30 @@ export const ReviewPanel: React.FC = ({ // Ensure selectedHunkId is valid after filtering/sorting: // - If no selection or selection not in filtered list, select first visible hunk // - This runs after sorting, so we always select the top-most hunk in current order + // + // Immersive review can intentionally navigate to a hunk that is hidden by + // the active filter (e.g. clicking a pending review whose hunk has been + // marked read while hide-read is on). The immersive view falls back to + // `allHunks` for those selections, so when we're in immersive mode we only + // reset when the hunk has truly disappeared from the diff (e.g. after a + // refresh removed it). The non-immersive panel only ever renders + // `filteredHunks`, so it keeps the original auto-advance to a visible hunk. useEffect(() => { if (filteredHunks.length === 0) return; + if (isImmersive) { + const selectionExists = selectedHunkId && hunks.some((h) => h.id === selectedHunkId); + if (!selectionExists) { + setSelectedHunkId(filteredHunks[0].id); + } + return; + } + const selectionValid = selectedHunkId && filteredHunks.some((h) => h.id === selectedHunkId); if (!selectionValid) { setSelectedHunkId(filteredHunks[0].id); } - }, [filteredHunks, selectedHunkId, setSelectedHunkId]); + }, [filteredHunks, hunks, isImmersive, selectedHunkId, setSelectedHunkId]); // Memoize search config to prevent re-creating object on every render // This allows React.memo on HunkViewer to work properly