Skip to content

Commit 9b7c39d

Browse files
authored
fix(review): move hide-whitespace to server-side git diff -w (#635 follow-up) (#638)
The client-side approach from PR #635 normalized file contents before diffing, which destroyed all indentation. Move whitespace handling to the server by threading a `-w` flag through the git diff pipeline. - Add `GitDiffOptions` to review-core.ts, inject `-w` in all 7 diff type paths + untracked file diffs - Thread options through git.ts → vcs.ts → review.ts / Pi server - Read `hideWhitespace` from ~/.plannotator/config.json on startup so the initial diff already respects the persisted preference - Accept `hideWhitespace` in `/api/diff/switch`, echo in responses - Client toggles trigger a lightweight server refetch that preserves the active file (no panel reset) - Handle edge case where current file disappears when `-w` removes whitespace-only diffs - Remove broken client-side parseDiffFromFile/normalize hack - Update API docs in AGENTS.md Closes the indentation bug reported by @zeroZshadow on PR #631. For provenance purposes, this commit was AI assisted.
1 parent 657ccca commit 9b7c39d

14 files changed

Lines changed: 152 additions & 82 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,8 +223,8 @@ During normal plan review, an Archive sidebar tab provides the same browsing via
223223

224224
| Endpoint | Method | Purpose |
225225
| --------------------- | ------ | ------------------------------------------ |
226-
| `/api/diff` | GET | Returns `{ rawPatch, gitRef, origin, diffType, base, gitContext }` |
227-
| `/api/diff/switch` | POST | Switch diff type and/or base branch (body: `{ diffType, base? }`) |
226+
| `/api/diff` | GET | Returns `{ rawPatch, gitRef, origin, diffType, base, hideWhitespace, gitContext }` |
227+
| `/api/diff/switch` | POST | Switch diff type, base branch, or whitespace mode (body: `{ diffType, base?, hideWhitespace? }`) |
228228
| `/api/file-content` | GET | Returns `{ oldContent, newContent }` for expandable diff context (`?path=&oldPath=&base=`) |
229229
| `/api/git-add` | POST | Stage/unstage a file (body: `{ filePath, undo? }`) |
230230
| `/api/feedback` | POST | Submit review (body: feedback, annotations, agentSwitch) |

apps/hook/server/index.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -500,8 +500,11 @@ if (args[0] === "sessions") {
500500
} else {
501501
// --- Local Review Mode ---
502502
gitContext = await getVcsContext();
503-
initialDiffType = gitContext.vcsType === "p4" ? "p4-default" : resolveDefaultDiffType(loadConfig());
504-
const diffResult = await runVcsDiff(initialDiffType, gitContext.defaultBranch);
503+
const config = loadConfig();
504+
initialDiffType = gitContext.vcsType === "p4" ? "p4-default" : resolveDefaultDiffType(config);
505+
const diffResult = await runVcsDiff(initialDiffType, gitContext.defaultBranch, undefined, {
506+
hideWhitespace: config.diffOptions?.hideWhitespace ?? false,
507+
});
505508
rawPatch = diffResult.patch;
506509
gitRef = diffResult.label;
507510
diffError = diffResult.error;

apps/opencode-plugin/commands.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,11 @@ export async function handleReviewCommand(
9292
client.app.log({ level: "info", message: "Opening code review UI..." });
9393

9494
gitContext = await getGitContext(directory);
95-
userDiffType = resolveDefaultDiffType(loadConfig());
96-
const diffResult = await runGitDiffWithContext(userDiffType, gitContext);
95+
const config = loadConfig();
96+
userDiffType = resolveDefaultDiffType(config);
97+
const diffResult = await runGitDiffWithContext(userDiffType, gitContext, {
98+
hideWhitespace: config.diffOptions?.hideWhitespace ?? false,
99+
});
97100
rawPatch = diffResult.patch;
98101
gitRef = diffResult.label;
99102
diffError = diffResult.error;

apps/pi-extension/plannotator-browser.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -352,8 +352,11 @@ export async function openCodeReview(
352352
const cwd = options.cwd ?? ctx.cwd;
353353
gitCtx = await getGitContext(cwd);
354354
const defaultBranch = options.defaultBranch ?? gitCtx.defaultBranch;
355-
diffType = options.diffType ?? resolveDefaultDiffType(loadConfig());
356-
const result = await runGitDiff(diffType, defaultBranch, cwd);
355+
const config = loadConfig();
356+
diffType = options.diffType ?? resolveDefaultDiffType(config);
357+
const result = await runGitDiff(diffType, defaultBranch, cwd, {
358+
hideWhitespace: config.diffOptions?.hideWhitespace ?? false,
359+
});
357360
rawPatch = result.patch;
358361
gitRef = result.label;
359362
diffError = result.error;

apps/pi-extension/server/serverReview.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import os from "node:os";
66
import { Readable } from "node:stream";
77

88
import { contentHash, deleteDraft } from "../generated/draft.js";
9-
import { saveConfig, detectGitUser, getServerConfig } from "../generated/config.js";
9+
import { loadConfig, saveConfig, detectGitUser, getServerConfig } from "../generated/config.js";
1010

1111
export type {
1212
DiffOption,
@@ -27,6 +27,7 @@ import {
2727
type DiffType,
2828
type GitCommandResult,
2929
type GitContext,
30+
type GitDiffOptions,
3031
detectRemoteDefaultBranch,
3132
getFileContentsForDiff as getFileContentsForDiffCore,
3233
getGitContext as getGitContextCore,
@@ -174,8 +175,9 @@ export function runGitDiff(
174175
diffType: DiffType,
175176
defaultBranch = "main",
176177
cwd?: string,
178+
options?: GitDiffOptions,
177179
): Promise<{ patch: string; label: string; error?: string }> {
178-
return runGitDiffCore(reviewRuntime, diffType, defaultBranch, cwd);
180+
return runGitDiffCore(reviewRuntime, diffType, defaultBranch, cwd, options);
179181
}
180182

181183
export async function startReviewServer(options: {
@@ -269,6 +271,7 @@ export async function startReviewServer(options: {
269271
let currentGitRef = options.gitRef;
270272
let currentDiffType: DiffType = options.diffType || "uncommitted";
271273
let currentError = options.error;
274+
let currentHideWhitespace = loadConfig().diffOptions?.hideWhitespace ?? false;
272275
let originalPRPatch = options.rawPatch;
273276
let originalPRGitRef = options.gitRef;
274277
let originalPRError = options.error;
@@ -606,6 +609,7 @@ export async function startReviewServer(options: {
606609
// Echo the active base so page refresh/reconnect rehydrates the
607610
// picker to what the server is actually using, not the detected default.
608611
base: hasLocalAccess ? currentBase : undefined,
612+
hideWhitespace: currentHideWhitespace,
609613
gitContext: hasLocalAccess ? options.gitContext : undefined,
610614
sharingEnabled,
611615
shareBaseUrl,
@@ -637,13 +641,18 @@ export async function startReviewServer(options: {
637641
json(res, { error: "Missing diffType" }, 400);
638642
return;
639643
}
644+
if (typeof body.hideWhitespace === "boolean") {
645+
currentHideWhitespace = body.hideWhitespace;
646+
}
640647
const detectedBase = options.gitContext?.defaultBranch || "main";
641648
const base = resolveBaseBranch(
642649
typeof body.base === "string" ? body.base : undefined,
643650
detectedBase,
644651
);
645652
const defaultCwd = options.gitContext?.cwd;
646-
const result = await runGitDiff(newType, base, defaultCwd);
653+
const result = await runGitDiff(newType, base, defaultCwd, {
654+
hideWhitespace: currentHideWhitespace,
655+
});
647656
currentPatch = result.patch;
648657
currentGitRef = result.label;
649658
currentDiffType = newType;
@@ -674,6 +683,7 @@ export async function startReviewServer(options: {
674683
// confirm the request landed (and pick it up when the client
675684
// didn't supply one and we fell back to detected default).
676685
base: currentBase,
686+
hideWhitespace: currentHideWhitespace,
677687
...(updatedContext ? { gitContext: updatedContext } : {}),
678688
...(currentError ? { error: currentError } : {}),
679689
});

packages/review-editor/App.tsx

Lines changed: 67 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,7 +1016,7 @@ const ReviewApp: React.FC = () => {
10161016
// Shared helper: fetch a diff switch and update state.
10171017
// Returns true on success, false on failure — callers that optimistically
10181018
// updated UI state (e.g. the base picker) can use this to revert.
1019-
const fetchDiffSwitch = useCallback(async (fullDiffType: string, baseOverride?: string): Promise<boolean> => {
1019+
const fetchDiffSwitch = useCallback(async (fullDiffType: string, baseOverride?: string, options?: { preserveFile?: boolean }): Promise<boolean> => {
10201020
setIsLoadingDiff(true);
10211021
try {
10221022
const res = await fetch('/api/diff/switch', {
@@ -1027,6 +1027,7 @@ const ReviewApp: React.FC = () => {
10271027
// Server ignores base for modes that don't use it (uncommitted/staged/etc),
10281028
// so forwarding unconditionally is safe and keeps the request shape uniform.
10291029
...((baseOverride ?? selectedBase) && { base: baseOverride ?? selectedBase }),
1030+
hideWhitespace: diffHideWhitespace,
10301031
}),
10311032
});
10321033

@@ -1042,46 +1043,59 @@ const ReviewApp: React.FC = () => {
10421043
};
10431044

10441045
const nextFiles = parseDiffToFiles(data.rawPatch);
1045-
dockApi?.getPanel(REVIEW_DIFF_PANEL_ID)?.api.close();
1046-
needsInitialDiffPanel.current = true;
1047-
setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef, diffType: data.diffType } : prev);
1048-
setFiles(nextFiles);
1049-
setDiffType(data.diffType);
1050-
// Adopt the server's echoed base. The server trusts whatever we sent
1051-
// verbatim — this sync just makes sure selectedBase and committedBase
1052-
// match the server's view (important when the caller didn't send a
1053-
// base and the server used the detected default instead).
1054-
if (data.base) {
1055-
setSelectedBase(data.base);
1056-
setCommittedBase(data.base);
1057-
}
1058-
// Merge only the per-cwd fields so the sidebar reflects the worktree
1059-
// we're now in. Keep the original `worktrees` list (already filtered to
1060-
// exclude the server's startup cwd — replacing it with the new context's
1061-
// list would duplicate the "Main repo" entry) and `availableBranches`
1062-
// (shared across worktrees of the same repo).
1063-
//
1064-
// IMPORTANT: we deliberately do NOT overwrite `currentBranch`. The
1065-
// WorktreePicker's top "launch" row uses it as a label, and that row
1066-
// represents the cwd plannotator was launched in — not whichever
1067-
// worktree is currently active. Freezing `currentBranch` at its
1068-
// initial-load value keeps that label truthful. `defaultBranch` and
1069-
// `diffOptions` update because they describe the active diff, which
1070-
// other UI (empty-state text, diff-type picker) should see fresh.
1071-
if (data.gitContext) {
1072-
setGitContext((prev) => {
1073-
if (!prev) return data.gitContext!;
1074-
return {
1075-
...prev,
1076-
defaultBranch: data.gitContext!.defaultBranch,
1077-
diffOptions: data.gitContext!.diffOptions,
1078-
};
1079-
});
1046+
1047+
if (options?.preserveFile) {
1048+
// Whitespace toggle: update patch in-place, keep the active file.
1049+
// If the current file was removed (whitespace-only), retarget the
1050+
// dock panel to the first remaining file.
1051+
setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef } : prev);
1052+
setFiles(nextFiles);
1053+
const currentPath = files[activeFileIndex]?.path;
1054+
const nextIdx = currentPath ? nextFiles.findIndex(f => f.path === currentPath) : -1;
1055+
if (nextIdx !== -1) {
1056+
setActiveFileIndex(nextIdx);
1057+
} else if (nextFiles.length > 0) {
1058+
setActiveFileIndex(0);
1059+
openDiffFile(nextFiles[0].path);
1060+
}
1061+
} else {
1062+
dockApi?.getPanel(REVIEW_DIFF_PANEL_ID)?.api.close();
1063+
needsInitialDiffPanel.current = true;
1064+
setDiffData(prev => prev ? { ...prev, rawPatch: data.rawPatch, gitRef: data.gitRef, diffType: data.diffType } : prev);
1065+
setFiles(nextFiles);
1066+
setDiffType(data.diffType);
1067+
if (data.base) {
1068+
setSelectedBase(data.base);
1069+
setCommittedBase(data.base);
1070+
}
1071+
// Merge only the per-cwd fields so the sidebar reflects the worktree
1072+
// we're now in. Keep the original `worktrees` list (already filtered to
1073+
// exclude the server's startup cwd — replacing it with the new context's
1074+
// list would duplicate the "Main repo" entry) and `availableBranches`
1075+
// (shared across worktrees of the same repo).
1076+
//
1077+
// IMPORTANT: we deliberately do NOT overwrite `currentBranch`. The
1078+
// WorktreePicker's top "launch" row uses it as a label, and that row
1079+
// represents the cwd plannotator was launched in — not whichever
1080+
// worktree is currently active. Freezing `currentBranch` at its
1081+
// initial-load value keeps that label truthful. `defaultBranch` and
1082+
// `diffOptions` update because they describe the active diff, which
1083+
// other UI (empty-state text, diff-type picker) should see fresh.
1084+
if (data.gitContext) {
1085+
setGitContext((prev) => {
1086+
if (!prev) return data.gitContext!;
1087+
return {
1088+
...prev,
1089+
defaultBranch: data.gitContext!.defaultBranch,
1090+
diffOptions: data.gitContext!.diffOptions,
1091+
};
1092+
});
1093+
}
1094+
setActiveFileIndex(0);
1095+
setPendingSelection(null);
1096+
resetStagedFiles();
10801097
}
1081-
setActiveFileIndex(0);
1082-
setPendingSelection(null);
10831098
setDiffError(data.error || null);
1084-
resetStagedFiles();
10851099
return true;
10861100
} catch (err) {
10871101
console.error('Failed to switch diff:', err);
@@ -1090,7 +1104,7 @@ const ReviewApp: React.FC = () => {
10901104
} finally {
10911105
setIsLoadingDiff(false);
10921106
}
1093-
}, [dockApi, resetStagedFiles, selectedBase]);
1107+
}, [dockApi, resetStagedFiles, selectedBase, diffHideWhitespace, files, activeFileIndex, openDiffFile]);
10941108

10951109
// Switch the base branch the current diff compares against.
10961110
// Only triggers a refetch when the active mode actually uses a base.
@@ -1130,6 +1144,18 @@ const ReviewApp: React.FC = () => {
11301144
await fetchDiffSwitch(fullDiffType);
11311145
}, [activeWorktreePath, activeDiffBase, fetchDiffSwitch]);
11321146

1147+
// Re-fetch diff when hideWhitespace toggles so the server applies git diff -w.
1148+
// Preserves the active file since only whitespace hunks change.
1149+
const hideWhitespaceInitialized = useRef(false);
1150+
useEffect(() => {
1151+
if (!origin || !gitContext) return;
1152+
if (!hideWhitespaceInitialized.current) {
1153+
hideWhitespaceInitialized.current = true;
1154+
return;
1155+
}
1156+
fetchDiffSwitch(diffType, selectedBase, { preserveFile: true });
1157+
}, [diffHideWhitespace, origin]); // eslint-disable-line react-hooks/exhaustive-deps
1158+
11331159
// Select annotation - switches file if needed and scrolls to it
11341160
const handleSelectAnnotation = useCallback((id: string | null) => {
11351161
if (!id) {
@@ -1192,7 +1218,6 @@ const ReviewApp: React.FC = () => {
11921218
lineDiffType: diffLineDiffType,
11931219
disableLineNumbers: !diffShowLineNumbers,
11941220
disableBackground: !diffShowBackground,
1195-
hideWhitespace: diffHideWhitespace,
11961221
fontFamily: diffFontFamily || undefined,
11971222
fontSize: diffFontSize || undefined,
11981223
// Only propagate base for modes where it affects old/new content. Avoids
@@ -1249,7 +1274,7 @@ const ReviewApp: React.FC = () => {
12491274
openTourPanel: handleOpenTour,
12501275
}), [
12511276
files, activeFileIndex, diffStyle, diffOverflow, diffIndicators,
1252-
diffLineDiffType, diffShowLineNumbers, diffShowBackground, diffHideWhitespace,
1277+
diffLineDiffType, diffShowLineNumbers, diffShowBackground,
12531278
diffFontFamily, diffFontSize, activeDiffBase, committedBase, feedbackDiffContext, prReviewScopeLabel, prDiffScope,
12541279
allAnnotations, externalAnnotations,
12551280
selectedAnnotationId, pendingSelection, handleLineSelection,

packages/review-editor/components/DiffViewer.tsx

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import React, { useMemo, useRef, useEffect, useLayoutEffect, useCallback, useState } from 'react';
22
import { FileDiff, type DiffLineAnnotation } from '@pierre/diffs/react';
3-
import { getSingularPatch, processFile, parseDiffFromFile } from '@pierre/diffs';
3+
import { getSingularPatch, processFile } from '@pierre/diffs';
44
import { CodeAnnotation, CodeAnnotationType, SelectedLineRange, DiffAnnotationMetadata, TokenAnnotationMeta, ConventionalLabel, ConventionalDecoration } from '@plannotator/ui/types';
55
import type { DiffTokenEventBaseProps } from '@pierre/diffs';
66
import { useTheme } from '@plannotator/ui/components/ThemeProvider';
@@ -127,7 +127,6 @@ interface DiffViewerProps {
127127
lineDiffType?: 'word-alt' | 'word' | 'char' | 'none';
128128
disableLineNumbers?: boolean;
129129
disableBackground?: boolean;
130-
hideWhitespace?: boolean;
131130
fontFamily?: string;
132131
fontSize?: string;
133132
annotations: CodeAnnotation[];
@@ -173,7 +172,6 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
173172
lineDiffType,
174173
disableLineNumbers,
175174
disableBackground,
176-
hideWhitespace,
177175
fontFamily,
178176
fontSize,
179177
annotations,
@@ -294,18 +292,9 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
294292

295293
// Re-parse the patch with full file contents so hunk indices are computed
296294
// against the complete file (isPartial: false), enabling expansion.
297-
// When hideWhitespace is on, recompute the diff from file contents to
298-
// exclude whitespace-only changes (like GitHub's ?w=1).
299295
const augmentedDiff = useMemo(() => {
300296
if (!fileContents || fileContents.forPath !== filePath || (fileContents.old == null && fileContents.new == null)) return fileDiff;
301297
try {
302-
if (hideWhitespace && fileContents.old != null && fileContents.new != null) {
303-
const normalize = (s: string) => s.split('\n').map(l => l.replace(/\s+/g, ' ').trim()).join('\n');
304-
return parseDiffFromFile(
305-
{ name: oldPath || filePath, contents: normalize(fileContents.old) },
306-
{ name: filePath, contents: normalize(fileContents.new) },
307-
);
308-
}
309298
const result = processFile(patch, {
310299
oldFile: fileContents.old != null ? { name: oldPath || filePath, contents: fileContents.old } : undefined,
311300
newFile: fileContents.new != null ? { name: filePath, contents: fileContents.new } : undefined,
@@ -314,7 +303,7 @@ export const DiffViewer: React.FC<DiffViewerProps> = ({
314303
} catch {
315304
return fileDiff;
316305
}
317-
}, [patch, filePath, oldPath, fileContents, fileDiff, hideWhitespace]);
306+
}, [patch, filePath, oldPath, fileContents, fileDiff]);
318307

319308
const previousScrollFilePathRef = useRef(filePath);
320309
useLayoutEffect(() => {

packages/review-editor/dock/ReviewStateContext.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ export interface ReviewState {
2626
lineDiffType?: 'word-alt' | 'word' | 'char' | 'none';
2727
disableLineNumbers?: boolean;
2828
disableBackground?: boolean;
29-
hideWhitespace?: boolean;
3029
fontFamily?: string;
3130
fontSize?: string;
3231
/** User-selected base branch; feeds the `base` query param on file-content fetches. */

packages/review-editor/dock/panels/ReviewDiffPanel.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ export const ReviewDiffPanel: React.FC<IDockviewPanelProps> = (props) => {
7878
lineDiffType={state.lineDiffType}
7979
disableLineNumbers={state.disableLineNumbers}
8080
disableBackground={state.disableBackground}
81-
hideWhitespace={state.hideWhitespace}
8281
fontFamily={state.fontFamily}
8382
fontSize={state.fontSize}
8483
annotations={fileAnnotations}

packages/server/git.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
type DiffType,
1212
type GitCommandResult,
1313
type GitContext,
14+
type GitDiffOptions,
1415
type ReviewGitRuntime,
1516
type WorktreeInfo,
1617
getCurrentBranch as getCurrentBranchCore,
@@ -31,6 +32,7 @@ export type {
3132
DiffType,
3233
DiffResult,
3334
GitContext,
35+
GitDiffOptions,
3436
WorktreeInfo,
3537
} from "@plannotator/shared/review-core";
3638

@@ -92,15 +94,17 @@ export function runGitDiff(
9294
diffType: DiffType,
9395
defaultBranch: string = "main",
9496
cwd?: string,
97+
options?: GitDiffOptions,
9598
): Promise<DiffResult> {
96-
return runGitDiffCore(runtime, diffType, defaultBranch, cwd);
99+
return runGitDiffCore(runtime, diffType, defaultBranch, cwd, options);
97100
}
98101

99102
export function runGitDiffWithContext(
100103
diffType: DiffType,
101104
gitContext: GitContext,
105+
options?: GitDiffOptions,
102106
): Promise<DiffResult> {
103-
return runGitDiffWithContextCore(runtime, diffType, gitContext);
107+
return runGitDiffWithContextCore(runtime, diffType, gitContext, options);
104108
}
105109

106110
export function getFileContentsForDiff(

0 commit comments

Comments
 (0)