diff --git a/apps/server/src/auth/RpcAuthorization.ts b/apps/server/src/auth/RpcAuthorization.ts index fb753b9aa4b..c1d9f786adb 100644 --- a/apps/server/src/auth/RpcAuthorization.ts +++ b/apps/server/src/auth/RpcAuthorization.ts @@ -75,6 +75,7 @@ export const RPC_REQUIRED_SCOPES = { [WS_METHODS.vcsSwitchRef]: AuthOrchestrationOperateScope, [WS_METHODS.vcsInit]: AuthOrchestrationOperateScope, [WS_METHODS.reviewGetDiffPreview]: AuthReviewWriteScope, + [WS_METHODS.reviewGetDiffFileContents]: AuthReviewWriteScope, [WS_METHODS.terminalOpen]: AuthTerminalOperateScope, [WS_METHODS.terminalAttach]: AuthTerminalOperateScope, [WS_METHODS.terminalWrite]: AuthTerminalOperateScope, diff --git a/apps/server/src/review/ReviewService.test.ts b/apps/server/src/review/ReviewService.test.ts index 839eb73b2bb..01a4692264e 100644 --- a/apps/server/src/review/ReviewService.test.ts +++ b/apps/server/src/review/ReviewService.test.ts @@ -57,6 +57,39 @@ describe("ReviewService", () => { }).pipe(Effect.provide(NodeServices.layer)), ); + it.effect("attributes file-content workspace violations to the file-content operation", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const workspaceRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-workspace-" }); + const outsideRoot = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-outside-" }); + const baseDir = yield* fs.makeTempDirectoryScoped({ prefix: "t3-review-base-" }); + const detectCalls: Array<{ readonly cwd: string }> = []; + + const error = yield* Effect.gen(function* () { + const review = yield* ReviewService.ReviewService; + return yield* review + .getDiffFileContents({ + cwd: outsideRoot, + sourceKind: "working-tree", + changeType: "change", + baseRef: "HEAD", + headRef: null, + oldPath: "file.ts", + newPath: "file.ts", + }) + .pipe(Effect.flip); + }).pipe(Effect.provide(makeLayer({ workspaceRoot, baseDir, detectCalls }))); + + assert.strictEqual(error._tag, "VcsRepositoryDetectionError"); + assert.strictEqual(error.operation, "ReviewService.getDiffFileContents"); + assert.match( + "detail" in error ? error.detail : "", + /must stay within the configured workspace root/, + ); + assert.deepStrictEqual(detectCalls, []); + }).pipe(Effect.provide(NodeServices.layer)), + ); + it.effect("allows diff preview cwd inside the configured workspace root", () => Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; diff --git a/apps/server/src/review/ReviewService.ts b/apps/server/src/review/ReviewService.ts index db1dc5bc8d2..275dcb41661 100644 --- a/apps/server/src/review/ReviewService.ts +++ b/apps/server/src/review/ReviewService.ts @@ -8,6 +8,8 @@ import * as Path from "effect/Path"; import { VcsRepositoryDetectionError, VcsUnsupportedOperationError, + type ReviewDiffFileContentsInput, + type ReviewDiffFileContentsResult, type ReviewDiffPreviewError, type ReviewDiffPreviewInput, type ReviewDiffPreviewResult, @@ -23,6 +25,9 @@ export class ReviewService extends Context.Service< readonly getDiffPreview: ( input: ReviewDiffPreviewInput, ) => Effect.Effect; + readonly getDiffFileContents: ( + input: ReviewDiffFileContentsInput, + ) => Effect.Effect; } >()("t3/review/ReviewService") {} @@ -58,6 +63,7 @@ export const make = Effect.gen(function* () { }; const assertWorkspaceBoundCwd = Effect.fn("ReviewService.assertWorkspaceBoundCwd")(function* ( + operation: "ReviewService.getDiffPreview" | "ReviewService.getDiffFileContents", cwd: string, ) { const [candidate, workspaceRoot, worktreesRoot] = yield* Effect.all([ @@ -71,16 +77,19 @@ export const make = Effect.gen(function* () { } return yield* new VcsRepositoryDetectionError({ - operation: "ReviewService.getDiffPreview", + operation, cwd, - detail: "Review diff preview cwd must stay within the configured workspace root.", + detail: + operation === "ReviewService.getDiffPreview" + ? "Review diff preview cwd must stay within the configured workspace root." + : "Review diff file contents cwd must stay within the configured workspace root.", }); }); const getDiffPreview: ReviewService["Service"]["getDiffPreview"] = Effect.fn( "ReviewService.getDiffPreview", )(function* (input) { - yield* assertWorkspaceBoundCwd(input.cwd); + yield* assertWorkspaceBoundCwd("ReviewService.getDiffPreview", input.cwd); const handle = yield* vcsRegistry.detect({ cwd: input.cwd, requestedKind: "auto" }); if (!handle) { @@ -106,8 +115,26 @@ export const make = Effect.gen(function* () { return yield* getDriverDiffPreview(input); }); + const getDiffFileContents: ReviewService["Service"]["getDiffFileContents"] = Effect.fn( + "ReviewService.getDiffFileContents", + )(function* (input) { + yield* assertWorkspaceBoundCwd("ReviewService.getDiffFileContents", input.cwd); + + const handle = yield* vcsRegistry.detect({ cwd: input.cwd, requestedKind: "auto" }); + if (handle?.kind !== "git") { + return yield* new VcsUnsupportedOperationError({ + operation: "ReviewService.getDiffFileContents", + kind: handle?.kind ?? "unknown", + detail: "Unchanged diff expansion currently requires a Git repository.", + }); + } + + return yield* git.getReviewDiffFileContents(input); + }); + return ReviewService.of({ getDiffPreview, + getDiffFileContents, }); }); diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts index 74d3fd2d594..59fecc4e234 100644 --- a/apps/server/src/server.test.ts +++ b/apps/server/src/server.test.ts @@ -5229,6 +5229,11 @@ it.layer(NodeServices.layer)("server router seam", (it) => { }, ], }), + getDiffFileContents: () => + Effect.succeed({ + oldContents: "before\n", + newContents: "after\n", + }), }, }, }); @@ -5343,6 +5348,22 @@ it.layer(NodeServices.layer)("server router seam", (it) => { ), ); assert.equal(diffPreview.sources[0]?.diff, "dirty-diff"); + + const diffFileContents = yield* Effect.scoped( + withWsRpcClient(wsUrl, (client) => + client[WS_METHODS.reviewGetDiffFileContents]({ + cwd: "/tmp/repo", + sourceKind: "working-tree", + changeType: "change", + baseRef: "HEAD", + headRef: null, + oldPath: "README.md", + newPath: "README.md", + }), + ), + ); + assert.equal(diffFileContents.oldContents, "before\n"); + assert.equal(diffFileContents.newContents, "after\n"); }).pipe(Effect.provide(NodeHttpServer.layerTest)), ); diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts index 55aa8f38835..192efe5a7d0 100644 --- a/apps/server/src/vcs/GitVcsDriver.ts +++ b/apps/server/src/vcs/GitVcsDriver.ts @@ -20,6 +20,8 @@ import { type VcsCreateWorktreeResult, type ReviewDiffPreviewInput, type ReviewDiffPreviewResult, + type ReviewDiffFileContentsInput, + type ReviewDiffFileContentsResult, type VcsInitInput, type VcsListRefsInput, type VcsListRefsResult, @@ -221,6 +223,9 @@ export class GitVcsDriver extends Context.Service< readonly getReviewDiffPreview: ( input: ReviewDiffPreviewInput, ) => Effect.Effect; + readonly getReviewDiffFileContents: ( + input: ReviewDiffFileContentsInput, + ) => Effect.Effect; readonly readConfigValue: ( cwd: string, key: string, diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 24d53cd4846..b14d1fdbaf3 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -803,6 +803,97 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { ); }), ); + + it.effect("loads full file contents for working-tree diff expansion", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + const pathService = yield* Path.Path; + yield* writeTextFile(cwd, "nested/.keep", ""); + yield* writeTextFile(cwd, "README.md", "# changed\nunchanged context\n"); + + const contents = yield* driver.getReviewDiffFileContents({ + cwd: pathService.join(cwd, "nested"), + sourceKind: "working-tree", + changeType: "change", + baseRef: "HEAD", + headRef: null, + oldPath: "README.md", + newPath: "README.md", + }); + + assert.strictEqual(contents.oldContents, "# test\n"); + assert.strictEqual(contents.newContents, "# changed\nunchanged context\n"); + }), + ); + + it.effect("loads new and deleted files without reading their missing side", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + const fileSystem = yield* FileSystem.FileSystem; + const pathService = yield* Path.Path; + yield* writeTextFile(cwd, "added.ts", "export const added = true;\n"); + yield* fileSystem.remove(pathService.join(cwd, "README.md")); + + const [added, deleted] = yield* Effect.all([ + driver.getReviewDiffFileContents({ + cwd, + sourceKind: "working-tree", + changeType: "new", + baseRef: "HEAD", + headRef: null, + oldPath: "added.ts", + newPath: "added.ts", + }), + driver.getReviewDiffFileContents({ + cwd, + sourceKind: "working-tree", + changeType: "deleted", + baseRef: "HEAD", + headRef: null, + oldPath: "README.md", + newPath: "README.md", + }), + ]); + + assert.deepStrictEqual(added, { + oldContents: "", + newContents: "export const added = true;\n", + }); + assert.deepStrictEqual(deleted, { + oldContents: "# test\n", + newContents: "", + }); + }), + ); + + it.effect("loads merge-base and head contents for branch diff expansion", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const { initialBranch } = yield* initRepoWithCommit(cwd); + const driver = yield* GitVcsDriver.GitVcsDriver; + yield* git(cwd, ["checkout", "-b", "feature/context"]); + yield* writeTextFile(cwd, "README.md", "# branch change\nunchanged context\n"); + yield* git(cwd, ["add", "README.md"]); + yield* git(cwd, ["commit", "-m", "change readme"]); + + const contents = yield* driver.getReviewDiffFileContents({ + cwd, + sourceKind: "branch-range", + changeType: "change", + baseRef: initialBranch, + headRef: "feature/context", + oldPath: "README.md", + newPath: "README.md", + }); + + assert.strictEqual(contents.oldContents, "# test\n"); + assert.strictEqual(contents.newContents, "# branch change\nunchanged context\n"); + }), + ); }); describe("repository status", () => { diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index e44dc048634..bcbb1694103 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -21,6 +21,7 @@ import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import { GitCommandError, + type ReviewDiffFileContentsInput, type ReviewDiffPreviewInput, type ReviewDiffPreviewSource, type VcsRef, @@ -46,6 +47,7 @@ const RANGE_DIFF_SUMMARY_MAX_OUTPUT_BYTES = 19_000; const RANGE_DIFF_PATCH_MAX_OUTPUT_BYTES = 59_000; const REVIEW_DIFF_PATCH_MAX_OUTPUT_BYTES = 120_000; const REVIEW_UNTRACKED_DIFF_MAX_OUTPUT_BYTES = 80_000; +const REVIEW_DIFF_FILE_MAX_OUTPUT_BYTES = 1024 * 1024; const WORKSPACE_FILES_MAX_OUTPUT_BYTES = 120_000; const STATUS_UPSTREAM_REFRESH_INTERVAL = Duration.seconds(15); const STATUS_UPSTREAM_REFRESH_TIMEOUT = Duration.seconds(5); @@ -2270,6 +2272,157 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* }; }); + const reviewDiffFileError = ( + input: ReviewDiffFileContentsInput, + detail: string, + cause?: unknown, + ) => + new GitCommandError({ + operation: "GitVcsDriver.getReviewDiffFileContents", + command: "git", + cwd: input.cwd, + detail, + ...(cause === undefined ? {} : { cause }), + }); + + const readReviewFileAtRevision = Effect.fn("readReviewFileAtRevision")(function* ( + input: ReviewDiffFileContentsInput, + revision: string, + relativePath: string, + ) { + const result = yield* executeGit( + "GitVcsDriver.getReviewDiffFileContents.revision", + input.cwd, + ["show", `${revision}:${relativePath}`], + { maxOutputBytes: REVIEW_DIFF_FILE_MAX_OUTPUT_BYTES }, + ); + if (result.stdout.includes("\0")) { + return yield* reviewDiffFileError(input, `Cannot expand binary file '${relativePath}'.`); + } + return result.stdout; + }); + + const readWorkingTreeReviewFile = Effect.fn("readWorkingTreeReviewFile")(function* ( + input: ReviewDiffFileContentsInput, + repositoryRoot: string, + ) { + const requestedPath = path.resolve(repositoryRoot, input.newPath); + const relativeRequestedPath = path.relative(repositoryRoot, requestedPath); + if ( + relativeRequestedPath === ".." || + relativeRequestedPath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativeRequestedPath) + ) { + return yield* reviewDiffFileError( + input, + `Diff file '${input.newPath}' resolves outside the review workspace.`, + ); + } + + const [realRepositoryRoot, realTarget] = yield* Effect.all([ + fileSystem.realPath(repositoryRoot), + fileSystem.realPath(requestedPath), + ]).pipe( + Effect.mapError((cause) => + reviewDiffFileError(input, `Could not resolve diff file '${input.newPath}'.`, cause), + ), + ); + const relativeRealPath = path.relative(realRepositoryRoot, realTarget); + if ( + relativeRealPath === ".." || + relativeRealPath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativeRealPath) + ) { + return yield* reviewDiffFileError( + input, + `Diff file '${input.newPath}' resolves outside the review workspace.`, + ); + } + + const info = yield* fileSystem + .stat(realTarget) + .pipe( + Effect.mapError((cause) => + reviewDiffFileError(input, `Could not inspect diff file '${input.newPath}'.`, cause), + ), + ); + if (info.type !== "File") { + return yield* reviewDiffFileError(input, `Diff path '${input.newPath}' is not a file.`); + } + if (info.size > BigInt(REVIEW_DIFF_FILE_MAX_OUTPUT_BYTES)) { + return yield* reviewDiffFileError( + input, + `Diff file '${input.newPath}' exceeds the 1 MB expansion limit.`, + ); + } + + const bytes = yield* fileSystem + .readFile(realTarget) + .pipe( + Effect.mapError((cause) => + reviewDiffFileError(input, `Could not read diff file '${input.newPath}'.`, cause), + ), + ); + if (bytes.includes(0)) { + return yield* reviewDiffFileError(input, `Cannot expand binary file '${input.newPath}'.`); + } + return new TextDecoder("utf-8").decode(bytes); + }); + + const getReviewDiffFileContents = Effect.fn("getReviewDiffFileContents")(function* ( + input: ReviewDiffFileContentsInput, + ) { + if (input.sourceKind === "working-tree") { + const repositoryRoot = yield* runGitStdout( + "GitVcsDriver.getReviewDiffFileContents.repositoryRoot", + input.cwd, + ["rev-parse", "--show-toplevel"], + ).pipe(Effect.map((value) => value.trim())); + if (repositoryRoot.length === 0) { + return yield* reviewDiffFileError(input, "Could not resolve the Git repository root."); + } + const [oldContents, newContents] = yield* Effect.all( + [ + input.changeType === "new" + ? Effect.succeed("") + : readReviewFileAtRevision(input, input.baseRef ?? "HEAD", input.oldPath), + input.changeType === "deleted" + ? Effect.succeed("") + : readWorkingTreeReviewFile(input, repositoryRoot), + ], + { concurrency: 2 }, + ); + return { oldContents, newContents }; + } + + if (!input.baseRef || !input.headRef) { + return yield* reviewDiffFileError( + input, + "Branch diff file expansion requires both base and head refs.", + ); + } + const mergeBase = yield* runGitStdout( + "GitVcsDriver.getReviewDiffFileContents.mergeBase", + input.cwd, + ["merge-base", input.baseRef, input.headRef], + ).pipe(Effect.map((value) => value.trim())); + if (mergeBase.length === 0) { + return yield* reviewDiffFileError(input, "Could not resolve the branch comparison base."); + } + const [oldContents, newContents] = yield* Effect.all( + [ + input.changeType === "new" + ? Effect.succeed("") + : readReviewFileAtRevision(input, mergeBase, input.oldPath), + input.changeType === "deleted" + ? Effect.succeed("") + : readReviewFileAtRevision(input, input.headRef, input.newPath), + ], + { concurrency: 2 }, + ); + return { oldContents, newContents }; + }); + const readConfigValue: GitVcsDriver.GitVcsDriver["Service"]["readConfigValue"] = (cwd, key) => runGitStdout("GitVcsDriver.readConfigValue", cwd, ["config", "--get", key], true).pipe( Effect.map((stdout) => stdout.trim()), @@ -2903,6 +3056,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* pullCurrentBranch: (cwd) => withListRefsInvalidation(cwd, pullCurrentBranch(cwd)), readRangeContext, getReviewDiffPreview, + getReviewDiffFileContents, readConfigValue, listRefs, createWorktree: (input) => withListRefsInvalidation(input.cwd, createWorktree(input)), diff --git a/apps/server/src/ws.ts b/apps/server/src/ws.ts index 909a51a4cf5..4ece17cc1d5 100644 --- a/apps/server/src/ws.ts +++ b/apps/server/src/ws.ts @@ -1854,6 +1854,12 @@ const makeWsRpcLayer = ( observeRpcEffect(WS_METHODS.reviewGetDiffPreview, review.getDiffPreview(input), { "rpc.aggregate": "review", }), + [WS_METHODS.reviewGetDiffFileContents]: (input) => + observeRpcEffect( + WS_METHODS.reviewGetDiffFileContents, + review.getDiffFileContents(input), + { "rpc.aggregate": "review" }, + ), [WS_METHODS.terminalOpen]: (input) => observeRpcEffect(WS_METHODS.terminalOpen, terminalManager.open(input), { "rpc.aggregate": "terminal", diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 836687af3df..94904cd8f32 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -5643,7 +5643,14 @@ function ChatViewContent(props: ChatViewProps) { /> ); const panelLayoutControls = ( -
+
{rightPanelOpen && !shouldUsePlanSidebarSheet ? ( (null); + const lastCompletedTurnRefreshRef = useRef<{ + readonly threadKey: string | null; + readonly turnId: TurnId | null; + } | null>(null); const routeThreadRef = useParams({ strict: false, @@ -230,6 +396,7 @@ export default function DiffPanel({ activeThread?.environmentId ?? null, serverConfig?.availableEditors ?? [], ); + const getDiffFileContents = useAtomCommand(reviewEnvironment.diffFileContents); const gitStatusQuery = useEnvironmentQuery( activeThread !== null && activeThread !== undefined && activeCwd != null ? vcsEnvironment.status({ @@ -298,6 +465,7 @@ export default function DiffPanel({ const collapseScopeKey = routeThreadRef ? `${routeThreadRef.environmentId}:${routeThreadRef.threadId}:${reviewSectionId}` : null; + const codeViewMountKey = `${collapseScopeKey ?? reviewSectionId}:${codeViewRevision}`; const collapsedDiffFileKeys = collapsedDiffFiles.scopeKey === collapseScopeKey ? collapsedDiffFiles.fileKeys @@ -360,9 +528,93 @@ export default function DiffPanel({ const branchDiffPreview = shouldRetryBranchDiffAtEnvironmentCwd ? fallbackBranchDiffPreview : primaryBranchDiffPreview; + const refreshBranchDiffPreview = branchDiffPreview.refresh; + const canRefreshGitDiff = + isGitRepo && selectedTurnId === null && activeThread != null && activeCwd != null; + const activeThreadRefreshKey = routeThreadRef + ? `${routeThreadRef.environmentId}:${routeThreadRef.threadId}` + : null; + + useEffect(() => { + if (!canRefreshGitDiff) return; + const refreshOnFocus = () => refreshBranchDiffPreview(); + window.addEventListener("focus", refreshOnFocus); + return () => window.removeEventListener("focus", refreshOnFocus); + }, [canRefreshGitDiff, refreshBranchDiffPreview]); + + useEffect(() => { + const current = { + threadKey: activeThreadRefreshKey, + turnId: latestTurn?.turnId ?? null, + }; + const previous = lastCompletedTurnRefreshRef.current; + lastCompletedTurnRefreshRef.current = current; + if ( + !canRefreshGitDiff || + previous === null || + previous.threadKey !== current.threadKey || + previous.turnId === current.turnId + ) { + return; + } + refreshBranchDiffPreview(); + }, [activeThreadRefreshKey, canRefreshGitDiff, latestTurn?.turnId, refreshBranchDiffPreview]); + const selectedGitSource = branchDiffPreview.data?.sources.find( (source) => source.kind === (selectedGitScope === "unstaged" ? "working-tree" : "branch-range"), ); + const loadDiffFiles = useMemo(() => { + const preview = branchDiffPreview.data; + if (selectedTurnId !== null || !activeThread || !preview || !selectedGitSource) { + return undefined; + } + + const source = selectedGitSource; + return async (fileDiff) => { + const newPath = resolveFileDiffPath(fileDiff); + const oldPath = fileDiff.prevName + ? resolveFileDiffPath({ ...fileDiff, name: fileDiff.prevName }) + : newPath; + const result = await getDiffFileContents({ + environmentId: activeThread.environmentId, + input: { + cwd: preview.cwd, + sourceKind: source.kind, + changeType: fileDiff.type, + baseRef: source.baseRef, + headRef: source.headRef, + oldPath, + newPath, + }, + }); + if (result._tag !== "Success") { + throw squashAtomCommandFailure(result); + } + + const newFile = { + name: newPath, + contents: result.value.newContents, + cacheKey: `${source.diffHash}:new:${newPath}`, + }; + if (fileDiff.type === "rename-pure") { + return { oldFile: null, newFile }; + } + return { + oldFile: { + name: oldPath, + contents: result.value.oldContents, + cacheKey: `${source.diffHash}:old:${oldPath}`, + }, + newFile, + }; + }; + }, [ + activeThread, + branchDiffPreview.data, + getDiffFileContents, + selectedGitSource, + selectedTurnId, + ]); const localBranchRefs = useEnvironmentQuery( selectedTurnId === null && selectedGitScope === "branch" && @@ -439,10 +691,17 @@ export default function DiffPanel({ }), ); }, [renderablePatch]); + const renderableFileEntries = useMemo( + () => + renderableFiles.map((fileDiff) => ({ + fileDiff, + fileKey: buildFileDiffRenderKey(fileDiff), + })), + [renderableFiles], + ); const codeViewFiles = useMemo( () => - renderableFiles.map((fileDiff) => { - const fileKey = buildFileDiffRenderKey(fileDiff); + renderableFileEntries.map(({ fileDiff, fileKey }) => { return { fileDiff, filePath: resolveFileDiffPath(fileDiff), @@ -450,18 +709,19 @@ export default function DiffPanel({ collapsed: collapsedDiffFileKeys.has(fileKey), }; }), - [collapsedDiffFileKeys, renderableFiles], + [collapsedDiffFileKeys, renderableFileEntries], ); const diffFileKeys = useMemo(() => codeViewFiles.map((file) => file.fileKey), [codeViewFiles]); const allDiffFilesCollapsed = areAllDiffFilesCollapsed(diffFileKeys, collapsedDiffFileKeys); const diffLineStat = useMemo(() => getDiffLineStat(renderableFiles), [renderableFiles]); + const selectedDiffFileKey = selectedFilePath + ? (codeViewFiles.find((candidate) => candidate.filePath === selectedFilePath)?.fileKey ?? null) + : null; useEffect(() => { - if (!selectedFilePath) return; - const file = codeViewFiles.find((candidate) => candidate.filePath === selectedFilePath); - if (!file) return; - codeViewRef.current?.scrollTo({ type: "item", id: file.fileKey, align: "start" }); - }, [codeViewFiles, selectedFilePath, selectedFileRevealRequestId]); + if (!selectedDiffFileKey) return; + codeViewRef.current?.scrollTo({ type: "item", id: selectedDiffFileKey, align: "start" }); + }, [codeViewMountKey, selectedDiffFileKey, selectedFileRevealRequestId]); const openDiffFile = useCallback( (filePath: string) => { @@ -506,6 +766,7 @@ export default function DiffPanel({ ); const toggleDiffFileCollapse = useCallback(() => { + setCodeViewRevision((current) => current + 1); setCollapsedDiffFiles((current) => { const currentKeys = current.scopeKey === collapseScopeKey ? current.fileKeys : EMPTY_COLLAPSED_DIFF_FILE_KEYS; @@ -724,23 +985,45 @@ export default function DiffPanel({ layout="inline" /> )} + {canRefreshGitDiff && ( + + + } + > + + + + {branchDiffPreview.isPending ? "Refreshing diff…" : "Refresh diff"} + + + )} {codeViewFiles.length > 0 && ( } > {allDiffFilesCollapsed ? ( - + ) : ( - + )} @@ -749,9 +1032,8 @@ export default function DiffPanel({ )} { const next = value[0]; @@ -760,11 +1042,11 @@ export default function DiffPanel({ } }} > - - + + - - + + @@ -772,8 +1054,8 @@ export default function DiffPanel({ render={ { setWordWrap(Boolean(pressed)); @@ -781,7 +1063,7 @@ export default function DiffPanel({ /> } > - + {wordWrap ? "Disable line wrapping" : "Enable line wrapping"} @@ -794,8 +1076,8 @@ export default function DiffPanel({ aria-label={ diffIgnoreWhitespace ? "Show whitespace changes" : "Hide whitespace changes" } - variant="outline" - size="xs" + variant="ghost" + size="sm" pressed={diffIgnoreWhitespace} onPressedChange={(pressed) => { setDiffIgnoreWhitespace(Boolean(pressed)); @@ -803,7 +1085,7 @@ export default function DiffPanel({ /> } > - + {diffIgnoreWhitespace ? "Show whitespace changes" : "Hide whitespace changes"} @@ -876,7 +1158,7 @@ export default function DiffPanel({ >
diff --git a/apps/web/src/components/DiffPanelShell.tsx b/apps/web/src/components/DiffPanelShell.tsx index e727a80055d..c13af4d9560 100644 --- a/apps/web/src/components/DiffPanelShell.tsx +++ b/apps/web/src/components/DiffPanelShell.tsx @@ -10,7 +10,8 @@ export type DiffPanelMode = "inline" | "sheet" | "sidebar" | "embedded"; function getDiffPanelHeaderRowClassName(mode: DiffPanelMode) { const shouldUseDragRegion = isElectron && mode !== "sheet" && mode !== "embedded"; return cn( - "flex items-center justify-between gap-2 px-4", + "flex items-center justify-between gap-2", + mode === "embedded" ? "px-2" : "px-4", shouldUseDragRegion ? "drag-region h-[52px] border-b border-border wco:h-[env(titlebar-area-height)] wco:pr-[calc(100vw-env(titlebar-area-width)-env(titlebar-area-x)+1em)]" : "surface-subheader", @@ -59,30 +60,53 @@ export function DiffPanelHeaderSkeleton() { ); } +function DiffFileHeaderSkeleton({ titleClassName }: { titleClassName: string }) { + return ( +
+
+ +
+ + +
+ + +
+
+ ); +} + +function DiffCodeLineSkeleton({ contentClassName }: { contentClassName: string }) { + return ( +
+ + +
+ ); +} + export function DiffPanelLoadingState(props: { label: string }) { return ( -
-
-
- - -
-
-
- - - - - -
- {props.label} -
+
+ +
+
+ +
+
+
+ + +
+ + + {props.label}
); } diff --git a/apps/web/src/components/RightPanelTabs.tsx b/apps/web/src/components/RightPanelTabs.tsx index fe652b6fde7..2b9fa1660d0 100644 --- a/apps/web/src/components/RightPanelTabs.tsx +++ b/apps/web/src/components/RightPanelTabs.tsx @@ -221,14 +221,14 @@ function surfaceTitle( function PreviewFavicon({ url }: { url: string | null }) { const faviconUrl = faviconUrlForOrigin(url, 32); const [failedUrl, setFailedUrl] = useState(null); - if (!faviconUrl || failedUrl === faviconUrl) return ; + if (!faviconUrl || failedUrl === faviconUrl) return ; return ( setFailedUrl(faviconUrl)} /> ); @@ -250,22 +250,22 @@ function SurfaceIcon({ return ; } case "diff": - return ; + return ; case "files": - return ; + return ; case "file": return ( ); case "terminal": - return ; + return ; case "plan": - return ; + return ; } } @@ -358,7 +358,7 @@ export function RightPanelTabs(props: RightPanelTabsProps) {
handleTabAuxClick(event, surface)} onContextMenu={(event) => void handleTabContextMenu(event, surface)} className={cn( - "group flex h-7 min-w-25 max-w-44 shrink-0 items-center gap-1.5 rounded-md px-2 text-sm", + "group/tab flex h-6 max-w-36 shrink-0 items-center gap-0.5 rounded-md pr-2 pl-1.5 text-xs", active ? "bg-accent text-foreground" : "text-muted-foreground hover:bg-accent/60 hover:text-foreground", )} > + props.onActivate(surface)} > - {title} } /> {title} -
); })} {props.surfaces.length > 0 ? ( - + {props.layoutControls}
-
+
{props.activeSurfaceId === null ? ( ({ + codeViewOptions: null as Record | null, +})); + +vi.mock("@pierre/diffs/react", () => ({ + CodeView: (props: { options: Record }) => { + testState.codeViewOptions = props.options; + return null; + }, +})); + +vi.mock("~/composerDraftStore", () => ({ + useComposerDraftStore: (selector: (store: Record) => unknown) => + selector({ + addReviewComment: vi.fn(), + removeReviewComment: vi.fn(), + getComposerDraft: () => undefined, + }), +})); + +vi.mock("../files/LocalCommentAnnotation", () => ({ + LocalCommentAnnotation: () => null, +})); + +vi.mock("../files/fileCommentAnnotations", () => ({ + nextFileCommentId: () => "comment-test", +})); + +import { AnnotatableCodeView } from "./AnnotatableCodeView"; + +describe("AnnotatableCodeView", () => { + beforeEach(() => { + testState.codeViewOptions = null; + }); + + it("opens comments from Pierre's gutter action without ending line selection", () => { + renderToStaticMarkup( + null} + />, + ); + + expect(testState.codeViewOptions).toMatchObject({ + enableGutterUtility: true, + enableLineSelection: true, + onGutterUtilityClick: expect.any(Function), + }); + expect(testState.codeViewOptions).not.toHaveProperty("onLineSelectionEnd"); + }); +}); diff --git a/apps/web/src/components/diffs/AnnotatableCodeView.tsx b/apps/web/src/components/diffs/AnnotatableCodeView.tsx index 6cea64fb570..0decaa3acf5 100644 --- a/apps/web/src/components/diffs/AnnotatableCodeView.tsx +++ b/apps/web/src/components/diffs/AnnotatableCodeView.tsx @@ -71,6 +71,7 @@ function appendAnnotationEntry( } interface AnnotatableCodeViewProps { + codeViewKey: string; files: ReadonlyArray<{ fileDiff: FileDiffMetadata; filePath: string; @@ -95,6 +96,7 @@ interface DiffSelectionContext { } export function AnnotatableCodeView({ + codeViewKey, files, sectionId, sectionTitle, @@ -117,6 +119,7 @@ export function AnnotatableCodeView({ fileKey: string; annotation: DiffCommentLineAnnotation; } | null>(null); + const [draftText, setDraftText] = useState(""); const filesByKey = useMemo(() => new Map(files.map((file) => [file.fileKey, file])), [files]); const items = useMemo[]>( @@ -167,6 +170,7 @@ export function AnnotatableCodeView({ setSelectedLines(null); if (draft?.annotation.metadata.entries.some((entry) => entry.id === entryId)) { setDraft(null); + setDraftText(""); } else { removeReviewComment(composerDraftTarget, entryId); } @@ -193,6 +197,7 @@ export function AnnotatableCodeView({ if (comment) addReviewComment(composerDraftTarget, comment); setSelectedLines(null); setDraft(null); + setDraftText(""); }, [addReviewComment, composerDraftTarget, draft, filesByKey, sectionId, sectionTitle], ); @@ -215,6 +220,7 @@ export function AnnotatableCodeView({ text: "", }); if (!comment) return; + setDraftText(""); setDraft({ fileKey: item.id, annotation: { @@ -232,6 +238,7 @@ export function AnnotatableCodeView({ const hasOpenComment = draft !== null; return ( + key={codeViewKey} {...(viewerRef ? { ref: viewerRef } : {})} {...(className ? { className } : {})} items={items} @@ -241,28 +248,34 @@ export function AnnotatableCodeView({ ...options, enableGutterUtility: !hasOpenComment, enableLineSelection: !hasOpenComment, - onLineSelectionEnd: beginComment, + onGutterUtilityClick: beginComment, }} renderHeaderPrefix={(item) => item.type === "diff" ? renderHeaderPrefix(item.fileDiff, item.id, item.collapsed === true) : null } - renderAnnotation={(annotation) => ( -
- {annotation.metadata.entries.map((entry) => ( - removeEntry(entry.id)} - onComment={(text) => submitEntry(entry.id, text)} - onDelete={() => removeEntry(entry.id)} - /> - ))} -
- )} + renderAnnotation={(annotation) => { + const hasDraft = annotation.metadata.entries.some((entry) => entry.kind === "draft"); + return ( +
+ {annotation.metadata.entries.map((entry) => ( + removeEntry(entry.id)} + onComment={(text) => submitEntry(entry.id, text)} + onDelete={() => removeEntry(entry.id)} + /> + ))} +
+ ); + }} /> ); } diff --git a/apps/web/src/components/files/LocalCommentAnnotation.test.tsx b/apps/web/src/components/files/LocalCommentAnnotation.test.tsx new file mode 100644 index 00000000000..f24460ca47f --- /dev/null +++ b/apps/web/src/components/files/LocalCommentAnnotation.test.tsx @@ -0,0 +1,66 @@ +import { renderToStaticMarkup } from "react-dom/server"; +import { describe, expect, it, vi } from "vite-plus/test"; + +import { LocalCommentAnnotation } from "./LocalCommentAnnotation"; + +const callbacks = { + onTextChange: vi.fn(), + onCancel: vi.fn(), + onComment: vi.fn(), + onDelete: vi.fn(), +}; + +describe("LocalCommentAnnotation", () => { + it("renders the draft composer directly in the selected diff", () => { + const markup = renderToStaticMarkup( + , + ); + + expect(markup).toContain("font-sans"); + expect(markup).not.toContain("chat-composer-glass"); + expect(markup).not.toContain("font-mono"); + expect(markup).not.toContain("Local comment"); + expect(markup).not.toContain("on +78"); + expect(markup).toContain("⌘/Ctrl Enter to send"); + expect(markup).toContain("Add a comment…"); + expect(markup).toContain(">Comment"); + expect(markup).toContain("autofocus"); + const textareaControl = markup.match(/]*data-slot="textarea-control"[^>]*>/)?.[0]; + expect(textareaControl).toBeDefined(); + expect(textareaControl).not.toContain("ring-ring"); + expect(markup).toContain("cursor-text"); + }); + + it("renders a saved comment without a nested card or redundant range label", () => { + const markup = renderToStaticMarkup( + , + ); + + expect(markup).toContain("font-sans"); + expect(markup).not.toContain("chat-composer-glass"); + expect(markup).not.toContain("on +78"); + expect(markup).toContain("Please keep this branch explicit."); + expect(markup).toContain('aria-label="Delete comment"'); + expect(markup).toContain("border-s-2"); + expect(markup).toContain("bg-primary/[0.045]"); + expect(markup).toContain("lucide-message-circle"); + }); + + it("renders draft text owned by the annotation wrapper", () => { + const markup = renderToStaticMarkup( + , + ); + + expect(markup).toContain("Keep this unsaved draft"); + }); +}); diff --git a/apps/web/src/components/files/LocalCommentAnnotation.tsx b/apps/web/src/components/files/LocalCommentAnnotation.tsx index a2765b02d37..19581a74da3 100644 --- a/apps/web/src/components/files/LocalCommentAnnotation.tsx +++ b/apps/web/src/components/files/LocalCommentAnnotation.tsx @@ -8,6 +8,7 @@ interface LocalCommentAnnotationProps { kind: "draft" | "comment"; rangeLabel: string; text: string; + onTextChange?: (text: string) => void; onCancel: () => void; onComment: (text: string) => void; onDelete: () => void; @@ -16,32 +17,34 @@ interface LocalCommentAnnotationProps { export function LocalCommentAnnotation({ kind, rangeLabel, - text: savedText, + text, + onTextChange, onCancel, onComment, onDelete, }: LocalCommentAnnotationProps) { - const [text, setText] = useState(""); + const [localDraftText, setLocalDraftText] = useState(""); + const displayedText = kind === "draft" && !onTextChange ? localDraftText : text; if (kind === "comment") { return (
event.stopPropagation()} > -
- - Local comment - {rangeLabel} - -
-

- {savedText} -

+
); } @@ -49,39 +52,49 @@ export function LocalCommentAnnotation({ return (
event.stopPropagation()} > -
- - Local comment -
-
Comment on lines {rangeLabel}