diff --git a/src/browser/features/RightSidebar/CodeReview/HunkViewer.tsx b/src/browser/features/RightSidebar/CodeReview/HunkViewer.tsx index f5c0cfded8..7fece9db01 100644 --- a/src/browser/features/RightSidebar/CodeReview/HunkViewer.tsx +++ b/src/browser/features/RightSidebar/CodeReview/HunkViewer.tsx @@ -247,11 +247,11 @@ export const HunkViewer = React.memo( const hasManualState = hunkId in expandStateMap; const manualExpandState = expandStateMap[hunkId]; - // Agent-flagged hunks should default to expanded even when they're "large" - // or in a heavy review where everything else is collapsed — otherwise the - // assisted-review focus signal gets buried. + // Agent-flagged hunks should default to expanded even when they're already + // read, "large", or in a heavy review where everything else is collapsed — + // otherwise the assisted-review focus signal gets buried. const shouldAutoExpand = - !isRead && + (!isRead || isAssisted) && (!isLargeHunk || Boolean(visibleNewLineRange)) && (!preferCollapsed || Boolean(isSelected) || Boolean(visibleNewLineRange)); diff --git a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts index 2903124d0f..944488afe5 100644 --- a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts +++ b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.assistedStats.test.ts @@ -1,7 +1,13 @@ import { describe, expect, test } from "bun:test"; import type { AssistedReviewHunk, DiffHunk } from "@/common/types/review"; -import { countUnreadAssistedHunks } from "./ReviewPanel"; +import { + buildReviewDiffPathFilter, + buildReviewDiffPathFilterSpecs, + countUnreadAssistedHunks, + getEffectiveReviewFrontendFilters, + getEffectiveReviewIncludeUncommitted, +} from "./ReviewPanel"; function hunk(overrides: Partial): DiffHunk { return { @@ -43,3 +49,128 @@ describe("countUnreadAssistedHunks", () => { expect(countUnreadAssistedHunks(hunks, assisted, () => false)).toBe(1); }); }); + +describe("buildReviewDiffPathFilter", () => { + test("assisted mode fetches agent-pinned files instead of the stale selected file", () => { + const pathFilter = buildReviewDiffPathFilter({ + isImmersive: false, + assistedOnly: true, + assistedHunks: [ + { path: "src/agent.ts" }, + { path: "src/agent.ts", range: { start: 3, end: 4 } }, + ], + selectedFilePath: "src/user-selected.ts", + selectedDiffPath: "src/user-selected.ts", + workspaceMetadata: null, + repoRootProjectPath: "/repo", + }); + + expect(pathFilter).toBe(" -- 'src/agent.ts'"); + }); + + test("non-assisted mode preserves the selected file pathspec", () => { + const pathFilter = buildReviewDiffPathFilter({ + isImmersive: false, + assistedOnly: false, + assistedHunks: [{ path: "src/agent.ts" }], + selectedFilePath: "src/user-selected.ts", + selectedDiffPath: "src/user-selected.ts", + workspaceMetadata: null, + repoRootProjectPath: "/repo", + }); + + expect(pathFilter).toBe(" -- 'src/user-selected.ts'"); + }); +}); + +describe("buildReviewDiffPathFilterSpecs", () => { + const workspaceMetadata = { + projects: [ + { projectName: "project-a", projectPath: "/repo/project-a" }, + { projectName: "project-b", projectPath: "/repo/project-b" }, + ], + }; + + test("assisted mode roots each multi-project pathspec in the pinned file's repository", () => { + const specs = buildReviewDiffPathFilterSpecs({ + isImmersive: false, + assistedOnly: true, + assistedHunks: [{ path: "project-b/src/agent.ts" }, { path: "project-a/src/main.ts" }], + selectedFilePath: "project-b/src/stale-selection.ts", + selectedDiffPath: "src/stale-selection.ts", + selectedRepoRootProjectPath: "/repo/project-b", + workspaceMetadata, + projectPath: "/repo/project-a", + }); + + expect(specs).toEqual([ + { + repoRootProjectPath: "/repo/project-b", + pathFilter: " -- 'src/agent.ts'", + selectedFilePath: "src/agent.ts", + }, + { + repoRootProjectPath: "/repo/project-a", + pathFilter: " -- 'src/main.ts'", + selectedFilePath: "src/main.ts", + }, + ]); + }); + + test("non-assisted mode keeps selected file rooting for truncation recovery", () => { + const specs = buildReviewDiffPathFilterSpecs({ + isImmersive: false, + assistedOnly: false, + assistedHunks: [{ path: "project-a/src/main.ts" }], + selectedFilePath: "project-b/src/user-selected.ts", + selectedDiffPath: "src/user-selected.ts", + selectedRepoRootProjectPath: "/repo/project-b", + workspaceMetadata, + projectPath: "/repo/project-a", + }); + + expect(specs).toEqual([ + { + repoRootProjectPath: "/repo/project-b", + pathFilter: " -- 'src/user-selected.ts'", + selectedFilePath: "project-b/src/user-selected.ts", + }, + ]); + }); +}); + +describe("getEffectiveReviewIncludeUncommitted", () => { + test("assisted mode includes working-tree edits even when the user toggle is off", () => { + expect( + getEffectiveReviewIncludeUncommitted({ assistedOnly: true, includeUncommitted: false }) + ).toBe(true); + }); + + test("non-assisted mode keeps the user include-uncommitted toggle", () => { + expect( + getEffectiveReviewIncludeUncommitted({ assistedOnly: false, includeUncommitted: false }) + ).toBe(false); + }); +}); + +describe("getEffectiveReviewFrontendFilters", () => { + test("assisted mode bypasses user filters that could hide accepted pins", () => { + expect( + getEffectiveReviewFrontendFilters({ + assistedOnly: true, + showReadHunks: false, + searchTerm: "does-not-match", + }) + ).toEqual({ showReadHunks: true, searchTerm: "" }); + }); + + test("non-assisted mode keeps user filters", () => { + expect( + getEffectiveReviewFrontendFilters({ + assistedOnly: false, + showReadHunks: false, + searchTerm: "needle", + }) + ).toEqual({ showReadHunks: false, searchTerm: "needle" }); + }); +}); diff --git a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx index 21a317cbc4..2bea468f1a 100644 --- a/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/browser/features/RightSidebar/CodeReview/ReviewPanel.tsx @@ -75,6 +75,7 @@ import type { ReviewSortOrder, } from "@/common/types/review"; import type { FileTreeNode } from "@/common/utils/git/numstatParser"; +import type { FrontendWorkspaceMetadata } from "@/common/types/workspace"; import { matchesKeybind, KEYBINDS, @@ -409,6 +410,32 @@ function parseReviewDiffCacheValue(params: { return { hunks: allHunks, truncationWarning, diagnosticInfo }; } +function mergeReviewDiffCacheValues( + values: readonly ReviewPanelDiffCacheValue[] +): ReviewPanelDiffCacheValue { + const hunks = values.flatMap((value) => value.hunks); + const truncationWarnings = values.flatMap((value) => + value.truncationWarning ? [value.truncationWarning] : [] + ); + const diagnostics = values.flatMap((value) => + value.diagnosticInfo ? [value.diagnosticInfo] : [] + ); + + return { + hunks, + truncationWarning: truncationWarnings.length > 0 ? truncationWarnings.join("\n") : null, + diagnosticInfo: + diagnostics.length > 0 + ? { + command: diagnostics.map((info) => info.command).join("\n\n"), + outputLength: diagnostics.reduce((sum, info) => sum + info.outputLength, 0), + fileDiffCount: diagnostics.reduce((sum, info) => sum + info.fileDiffCount, 0), + hunkCount: diagnostics.reduce((sum, info) => sum + info.hunkCount, 0), + } + : null, + }; +} + export function countUnreadAssistedHunks( hunks: readonly DiffHunk[], assistedHunks: readonly AssistedReviewHunk[], @@ -424,6 +451,126 @@ export function countUnreadAssistedHunks( return count; } +interface ReviewDiffPathFilterSpec { + repoRootProjectPath: string | null | undefined; + pathFilter: string; + /** Truthy when this request is path-filtered enough to suppress full-diff truncation messaging. */ + selectedFilePath: string | null; +} + +function toPathFilter(pathspecs: readonly string[]): string { + const uniquePathspecs = Array.from(new Set(pathspecs.filter((pathspec) => pathspec.length > 0))); + return uniquePathspecs.length > 0 + ? ` -- ${uniquePathspecs.map((pathspec) => shellQuote(pathspec)).join(" ")}` + : ""; +} + +export function buildReviewDiffPathFilterSpecs(params: { + isImmersive: boolean; + assistedOnly: boolean; + assistedHunks: readonly AssistedReviewHunk[]; + selectedFilePath: string | null; + selectedDiffPath: string; + selectedRepoRootProjectPath?: string | null; + workspaceMetadata: Pick | null | undefined; + projectPath: string; +}): ReviewDiffPathFilterSpec[] { + if (!params.assistedOnly) { + return [ + { + repoRootProjectPath: + params.selectedFilePath && !params.isImmersive + ? (params.selectedRepoRootProjectPath ?? params.projectPath) + : params.projectPath, + pathFilter: + params.selectedFilePath && !params.isImmersive + ? toPathFilter([params.selectedDiffPath]) + : "", + selectedFilePath: + params.selectedFilePath && !params.isImmersive ? params.selectedFilePath : null, + }, + ]; + } + + const pathspecsByRepoRoot = new Map< + string, + { repoRootProjectPath: string | null | undefined; pathspecs: string[] } + >(); + + for (const hunk of params.assistedHunks) { + // In multi-project workspaces, assisted pins use workspace-relative paths + // like `project-b/src/file.ts`. Fetch each repo-root group from the owning + // checkout so an accepted pin cannot disappear just because the Review pane + // was currently rooted to another project. + const repoRootProjectPath = + resolveRepoRootProjectPath(params.workspaceMetadata, hunk.path) ?? params.projectPath; + const key = repoRootProjectPath ?? ""; + const pathspec = normalizeRepoRootFilePath( + params.workspaceMetadata, + hunk.path, + repoRootProjectPath + ); + const existing = pathspecsByRepoRoot.get(key); + if (existing) { + existing.pathspecs.push(pathspec); + } else { + pathspecsByRepoRoot.set(key, { repoRootProjectPath, pathspecs: [pathspec] }); + } + } + + if (pathspecsByRepoRoot.size === 0) { + return [{ repoRootProjectPath: params.projectPath, pathFilter: "", selectedFilePath: null }]; + } + + return Array.from(pathspecsByRepoRoot.values()).map((spec) => ({ + repoRootProjectPath: spec.repoRootProjectPath, + pathFilter: toPathFilter(spec.pathspecs), + selectedFilePath: spec.pathspecs.length > 0 ? spec.pathspecs[0] : null, + })); +} + +export function buildReviewDiffPathFilter(params: { + isImmersive: boolean; + assistedOnly: boolean; + assistedHunks: readonly AssistedReviewHunk[]; + selectedFilePath: string | null; + selectedDiffPath: string; + workspaceMetadata: Pick | null | undefined; + repoRootProjectPath: string | null | undefined; +}): string { + return ( + buildReviewDiffPathFilterSpecs({ + ...params, + projectPath: params.repoRootProjectPath ?? "", + })[0]?.pathFilter ?? "" + ); +} + +export function getEffectiveReviewIncludeUncommitted(params: { + assistedOnly: boolean; + includeUncommitted: boolean; +}): boolean { + // Agent-pinned regions commonly point at the agent's latest working-tree edits. + // Assisted mode opts into uncommitted changes so accepted pins cannot disappear + // solely because the user's review toggle was left off. + return params.assistedOnly ? true : params.includeUncommitted; +} + +export function getEffectiveReviewFrontendFilters(params: { + assistedOnly: boolean; + showReadHunks: boolean; + searchTerm: string; +}): { showReadHunks: boolean; searchTerm: string } { + if (params.assistedOnly) { + // An agent pin is an explicit request to put that hunk in front of the user. + // Ignore stale user-side visibility filters while Assisted is on so a + // successful `review_pane_update` cannot be hidden by search/read state. + return { showReadHunks: true, searchTerm: "" }; + } + + return { showReadHunks: params.showReadHunks, searchTerm: params.searchTerm }; +} + interface ReviewAssistedStatsReporterProps { workspaceId: string; workspacePath: string; @@ -473,43 +620,67 @@ export const ReviewAssistedStatsReporter: React.FC ({ + ...spec, + diffCommand: buildGitDiffCommand( + diffBase, + effectiveIncludeUncommitted, + spec.pathFilter, + "diff" + ), + })); const loadUnreadAssisted = async () => { try { - await ensureOriginFetched({ - api, - workspaceId, - diffBase, - refreshToken: 0, - originFetchRef, - repoRootProjectPath: projectPath, - }); - if (cancelled) return; + const values = await Promise.all( + diffRequests.map(async (request) => { + await ensureOriginFetched({ + api, + workspaceId, + diffBase, + refreshToken: 0, + originFetchRef, + repoRootProjectPath: request.repoRootProjectPath, + }); + if (cancelled) return null; + + // Deliberately bypass `reviewPanelCache`: this reporter is mounted + // specifically so agent flags show up while the Review panel is not, + // and a stale panel cache would hide newly edited assisted hunks. + const result = await api.workspace.executeBash({ + workspaceId, + script: request.diffCommand, + options: repoRootBashOptions(30, request.repoRootProjectPath), + }); + if (cancelled || !result.success || !result.data.success) { + return null; + } - // Deliberately bypass `reviewPanelCache`: this reporter is mounted - // specifically so agent flags show up while the Review panel is not, - // and a stale panel cache would hide newly edited assisted hunks. - const result = await api.workspace.executeBash({ - workspaceId, - script: diffCommand, - options: repoRootBashOptions(30, projectPath), - }); + return parseReviewDiffCacheValue({ + result, + workspaceMetadata, + workspaceId, + repoRootProjectPath: request.repoRootProjectPath, + diffCommand: request.diffCommand, + selectedFilePath: request.selectedFilePath, + isImmersive: true, + }); + }) + ); if (cancelled) return; - if (!result.success || !result.data.success) { - onUnreadAssistedChange(0); - return; - } - - const data = parseReviewDiffCacheValue({ - result, - workspaceMetadata, - workspaceId, - repoRootProjectPath: projectPath, - diffCommand, - selectedFilePath: null, - isImmersive: true, - }); + const data = mergeReviewDiffCacheValues(values.filter((value) => value !== null)); onUnreadAssistedChange(countUnreadAssistedHunks(data.hunks, assistedHunks, isRead)); } catch { if (!cancelled) onUnreadAssistedChange(0); @@ -1203,30 +1374,57 @@ export const ReviewPanel: React.FC = ({ lastDiffRefreshTriggerRef.current = refreshTrigger; const isManualRefresh = refreshTrigger !== 0 && prevRefreshTrigger !== refreshTrigger; - const pathFilter = selectedFilePath && !isImmersive ? ` -- "${selectedDiffPath}"` : ""; - const diffRepoRootProjectPath = !isImmersive - ? (selectedRepoRootProjectPath ?? projectPath) - : projectPath; - - const diffCommand = buildGitDiffCommand( - filters.diffBase, - filters.includeUncommitted, - pathFilter, - "diff" - ); + const effectiveIncludeUncommitted = getEffectiveReviewIncludeUncommitted({ + assistedOnly: filters.assistedOnly, + includeUncommitted: filters.includeUncommitted, + }); - const cacheKey = makeReviewPanelCacheKey({ - workspaceId, - workspacePath, - gitCommand: diffCommand, - repoRootProjectPath: diffRepoRootProjectPath, + const diffRequests = buildReviewDiffPathFilterSpecs({ + isImmersive, + assistedOnly: filters.assistedOnly, + assistedHunks, + selectedFilePath, + selectedDiffPath, + selectedRepoRootProjectPath, + workspaceMetadata: workspaceMetadata.get(workspaceId), + projectPath, + }).map((spec) => { + const diffCommand = buildGitDiffCommand( + filters.diffBase, + effectiveIncludeUncommitted, + spec.pathFilter, + "diff" + ); + return { + ...spec, + diffCommand, + cacheKey: makeReviewPanelCacheKey({ + workspaceId, + workspacePath, + gitCommand: diffCommand, + repoRootProjectPath: spec.repoRootProjectPath, + }), + }; }); // Fast path: use cached diff when switching workspaces (unless user explicitly refreshed // or tools completed while we were unmounted). if (!isManualRefresh && !skipCacheOnMountRef.current) { - const cached = reviewPanelCache.get(cacheKey) as ReviewPanelDiffCacheValue | undefined; - if (cached) { + const cachedValues: ReviewPanelDiffCacheValue[] = []; + let allRequestsCached = true; + for (const request of diffRequests) { + const cached = reviewPanelCache.get(request.cacheKey) as + | ReviewPanelDiffCacheValue + | undefined; + if (!cached) { + allRequestsCached = false; + break; + } + cachedValues.push(cached); + } + + if (allRequestsCached) { + const cached = mergeReviewDiffCacheValues(cachedValues); setDiagnosticInfo(cached.diagnosticInfo); setDiffState({ status: "loaded", @@ -1263,39 +1461,44 @@ export const ReviewPanel: React.FC = ({ const loadDiff = async () => { try { - await ensureOriginFetched({ - api, - workspaceId, - diffBase: filters.diffBase, - refreshToken: refreshTrigger, - originFetchRef, - repoRootProjectPath: diffRepoRootProjectPath, - }); - if (cancelled) return; - - // Git-level filters (affect what data is fetched): - // - diffBase: what to diff against - // - includeUncommitted: include working directory changes - // - selectedFilePath: ESSENTIAL for truncation - if full diff is cut off, - // path filter lets us retrieve specific file's hunks - const data = await executeWorkspaceBashAndCache({ - api, - workspaceId, - script: diffCommand, - cacheKey, - timeoutSecs: 30, - repoRootProjectPath: diffRepoRootProjectPath, - parse: (result) => - parseReviewDiffCacheValue({ - result, - workspaceMetadata, + const values = await Promise.all( + diffRequests.map(async (request) => { + await ensureOriginFetched({ + api, workspaceId, - repoRootProjectPath: diffRepoRootProjectPath, - diffCommand, - selectedFilePath, - isImmersive, - }), - }); + diffBase: filters.diffBase, + refreshToken: refreshTrigger, + originFetchRef, + repoRootProjectPath: request.repoRootProjectPath, + }); + if (cancelled) return null; + + // Git-level filters (affect what data is fetched): + // - diffBase: what to diff against + // - includeUncommitted: include working directory changes + // - selectedFilePath / assisted pins: ESSENTIAL for truncation and + // multi-project parity; path filters retrieve specific files' hunks. + return executeWorkspaceBashAndCache({ + api, + workspaceId, + script: request.diffCommand, + cacheKey: request.cacheKey, + timeoutSecs: 30, + repoRootProjectPath: request.repoRootProjectPath, + parse: (result) => + parseReviewDiffCacheValue({ + result, + workspaceMetadata, + workspaceId, + repoRootProjectPath: request.repoRootProjectPath, + diffCommand: request.diffCommand, + selectedFilePath: request.selectedFilePath, + isImmersive, + }), + }); + }) + ); + const data = mergeReviewDiffCacheValues(values.filter((value) => value !== null)); if (cancelled) return; @@ -1336,6 +1539,8 @@ export const ReviewPanel: React.FC = ({ workspaceMetadata, filters.diffBase, filters.includeUncommitted, + filters.assistedOnly, + assistedHunks, selectedFilePath, selectedRepoRootProjectPath, selectedDiffPath, @@ -1453,10 +1658,16 @@ export const ReviewPanel: React.FC = ({ ? (hunk: DiffHunk) => assistedMatchByHunkId.has(hunk.id) : undefined; - const filtered = applyFrontendFilters(hunks, { + const effectiveFrontendFilters = getEffectiveReviewFrontendFilters({ + assistedOnly: filters.assistedOnly, showReadHunks: filters.showReadHunks, - isRead, searchTerm: debouncedSearchTerm, + }); + + const filtered = applyFrontendFilters(hunks, { + showReadHunks: effectiveFrontendFilters.showReadHunks, + isRead, + searchTerm: effectiveFrontendFilters.searchTerm, useRegex: searchState.useRegex, matchCase: searchState.matchCase, isAssisted: assistedPredicate, @@ -2003,14 +2214,14 @@ export const ReviewPanel: React.FC = ({ ) : filteredHunks.length === 0 ? (
- {debouncedSearchTerm.trim() - ? `No hunks match "${debouncedSearchTerm}". Try a different search term.` - : selectedFilePath - ? `No hunks in ${selectedFilePath}. Try selecting a different file.` - : filters.assistedOnly - ? assistedHunks.length === 0 - ? "The agent hasn't pinned any hunks. Turn off Assisted to see all hunks." - : "None of the agent-flagged hunks match the current diff. Turn off Assisted, or try a different diff base." + {filters.assistedOnly + ? assistedHunks.length === 0 + ? "The agent hasn't pinned any hunks. Turn off Assisted to see all hunks." + : "None of the agent-flagged hunks match the current diff. Turn off Assisted, or try a different diff base." + : debouncedSearchTerm.trim() + ? `No hunks match "${debouncedSearchTerm}". Try a different search term.` + : selectedFilePath + ? `No hunks in ${selectedFilePath}. Try selecting a different file.` : "No hunks match the current filters. Try adjusting your filter settings."}