From 1762c1a2892e4d76e25814f7a8a48e07ffe16850 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Sun, 14 Jun 2026 16:17:30 +0300 Subject: [PATCH] Polish diff file actions --- .../src/components/views/DiffHunkActions.tsx | 158 --------- packages/ui/src/components/views/DiffView.tsx | 326 +++++++++++++----- .../src/components/views/PierreDiffViewer.tsx | 6 +- packages/vscode/src/gitService.ts | 70 ++++ packages/web/server/lib/git/service.js | 44 ++- packages/web/server/lib/git/service.test.js | 19 + 6 files changed, 370 insertions(+), 253 deletions(-) delete mode 100644 packages/ui/src/components/views/DiffHunkActions.tsx diff --git a/packages/ui/src/components/views/DiffHunkActions.tsx b/packages/ui/src/components/views/DiffHunkActions.tsx deleted file mode 100644 index 12388e40..00000000 --- a/packages/ui/src/components/views/DiffHunkActions.tsx +++ /dev/null @@ -1,158 +0,0 @@ -import React from 'react'; - -import type { FileDiffMetadata } from '@pierre/diffs'; -import { Button } from '@/components/ui/button'; -import { Icon } from '@/components/icon/Icon'; -import { useRuntimeAPIs } from '@/hooks/useRuntimeAPIs'; -import { useI18n } from '@/lib/i18n'; -import { cn } from '@/lib/utils'; -import { extractHunkPatch } from '@/lib/diff/patchFileDiff'; - -type HunkAction = 'stage' | 'unstage' | 'discard'; - -interface DiffHunkActionsProps { - patch: string; - fileDiff: FileDiffMetadata | undefined; - directory: string; - filePath: string; - staged: boolean; - onApplied: (action: HunkAction) => void; -} - -export const DiffHunkActions = React.memo(({ - patch, - fileDiff, - directory, - filePath, - staged, - onApplied, -}) => { - const { t } = useI18n(); - const { git } = useRuntimeAPIs(); - const [busyKey, setBusyKey] = React.useState(null); - const [error, setError] = React.useState(null); - - const hunks = fileDiff?.hunks; - if (!hunks || hunks.length === 0 || !patch) { - return null; - } - - const run = async (hunkIndex: number, action: HunkAction) => { - const hunkPatch = extractHunkPatch(patch, hunkIndex); - if (!hunkPatch) { - setError(t('diffView.hunk.unavailable')); - return; - } - - const key = `${hunkIndex}:${action}`; - setBusyKey(key); - setError(null); - try { - if (action === 'stage') { - if (!git.stageGitHunk) throw new Error(t('diffView.hunk.unsupported')); - await git.stageGitHunk(directory, filePath, hunkPatch); - } else if (action === 'unstage') { - if (!git.unstageGitHunk) throw new Error(t('diffView.hunk.unsupported')); - await git.unstageGitHunk(directory, filePath, hunkPatch); - } else { - if (!git.revertGitHunk) throw new Error(t('diffView.hunk.unsupported')); - await git.revertGitHunk(directory, filePath, hunkPatch); - } - onApplied(action); - } catch (actionError) { - setError(actionError instanceof Error ? actionError.message : String(actionError)); - } finally { - setBusyKey((current) => (current === key ? null : current)); - } - }; - - return ( -
-
- - {t('diffView.hunk.label')} - - {hunks.map((hunk, index) => { - const additions = hunk.additionLines; - const deletions = hunk.deletionLines; - return ( -
- - {String(index + 1).padStart(2, '0')} - - {additions > 0 ? ( - - +{additions} - - ) : null} - {deletions > 0 ? ( - - −{deletions} - - ) : null} - {staged ? ( - - ) : ( - <> - - - - )} -
- ); - })} -
- {error ? ( -
- - {error} -
- ) : null} -
- ); -}); diff --git a/packages/ui/src/components/views/DiffView.tsx b/packages/ui/src/components/views/DiffView.tsx index 9ba6e5b7..8ffcfbe6 100644 --- a/packages/ui/src/components/views/DiffView.tsx +++ b/packages/ui/src/components/views/DiffView.tsx @@ -26,7 +26,6 @@ import { useRuntimeAPIs } from '@/hooks/useRuntimeAPIs'; import { DiffViewToggle } from '@/components/chat/message/DiffViewToggle'; import type { DiffViewMode } from '@/components/chat/message/types'; import { PierreDiffViewer } from './PierreDiffViewer'; -import { DiffHunkActions } from './DiffHunkActions'; import { useDeviceInfo } from '@/lib/device'; import { FileTypeIcon } from '@/components/icons/FileTypeIcon'; import { Icon } from "@/components/icon/Icon"; @@ -394,9 +393,6 @@ interface InlineDiffViewerProps { diff: DiffData; renderSideBySide: boolean; wrapLines: boolean; - directory: string; - staged: boolean; - onHunkApplied: (action: 'stage' | 'unstage' | 'discard') => void; } const InlineDiffViewer = React.memo(({ @@ -404,9 +400,6 @@ const InlineDiffViewer = React.memo(({ diff, renderSideBySide, wrapLines, - directory, - staged, - onHunkApplied, }) => { const language = React.useMemo( () => getLanguageFromExtension(filePath) || 'text', @@ -429,16 +422,6 @@ const InlineDiffViewer = React.memo(({ return (
- {diff.patch && diff.fileDiff ? ( - - ) : null} (({ ); }); +type FileDiffAction = 'stage' | 'unstage' | 'discard'; + +interface FileDiffActionsProps { + filePath: string; + staged: boolean; + busyAction: FileDiffAction | null; + disabled: boolean; + onAction: (action: FileDiffAction) => void; +} + +const FileDiffActions = React.memo(({ + filePath, + staged, + busyAction, + disabled, + onAction, +}) => { + const { t } = useI18n(); + return ( +
+ {staged ? ( + onAction('unstage')} + /> + ) : ( + <> + onAction('discard')} + /> + onAction('stage')} + /> + + )} +
+ ); +}); + +interface FileDiffActionButtonProps { + label: string; + icon: 'add' | 'arrow-go-back'; + loading: boolean; + disabled: boolean; + tone?: 'failure' | 'success'; + onClick: () => void; +} + +const FileDiffActionButton: React.FC = ({ + label, + icon, + loading, + disabled, + tone, + onClick, +}) => ( + +); + interface MultiFileDiffEntryProps { directory: string; file: FileEntry; @@ -468,7 +544,6 @@ interface MultiFileDiffEntryProps { isOpeningInEditor?: boolean; onOpenInEditor?: (filePath: string, diffData: DiffData | null) => void; staged?: boolean; - stagedRevision?: number; loadFullFiles?: boolean; } @@ -487,7 +562,6 @@ const MultiFileDiffEntry = React.memo(({ isOpeningInEditor = false, onOpenInEditor, staged = false, - stagedRevision = 0, loadFullFiles = false, }) => { const { t } = useI18n(); @@ -504,6 +578,7 @@ const MultiFileDiffEntry = React.memo(({ const [diffRetryNonce, setDiffRetryNonce] = React.useState(0); const [diffLoadError, setDiffLoadError] = React.useState(null); const [isLoading, setIsLoading] = React.useState(false); + const [fileAction, setFileAction] = React.useState(null); const [forceRenderLarge, setForceRenderLarge] = React.useState(false); const [localDiffData, setLocalDiffData] = React.useState(null); const [stagedDiffData, setStagedDiffData] = React.useState(null); @@ -513,6 +588,7 @@ const MultiFileDiffEntry = React.memo(({ const descriptor = React.useMemo(() => describeChange(file), [file]); const renderSideBySide = layout === 'side-by-side'; const desiredContextMode: DiffContextMode = loadFullFiles ? 'full' : 'patch'; + const fileStatusKey = `${file.index}:${file.working_dir}:${file.insertions}:${file.deletions}`; const diffData = React.useMemo(() => { if (staged) return stagedDiffData; @@ -545,7 +621,7 @@ const MultiFileDiffEntry = React.memo(({ setDiffLoadError(null); lastDiffRequestRef.current = null; - }, [staged, stagedRevision]); + }, [fileStatusKey, staged]); React.useEffect(() => { if (!isExpanded || !isMounted) return; @@ -555,7 +631,7 @@ const MultiFileDiffEntry = React.memo(({ return; } - const requestKey = `${directory}::${file.path}::${staged ? `staged:${stagedRevision}` : 'unstaged'}::${desiredContextMode}::${diffRetryNonce}`; + const requestKey = `${directory}::${file.path}::${staged ? 'staged' : 'unstaged'}::${fileStatusKey}::${desiredContextMode}::${diffRetryNonce}`; if (lastDiffRequestRef.current === requestKey) { return; } @@ -612,28 +688,58 @@ const MultiFileDiffEntry = React.memo(({ lastDiffRequestRef.current = null; } }; - }, [desiredContextMode, diffData, diffDataMatchesContextMode, diffRetryNonce, directory, file.path, git, isExpanded, isMounted, loadFullFiles, setDiff, staged, stagedRevision]); + }, [desiredContextMode, diffData, diffDataMatchesContextMode, diffRetryNonce, directory, file.path, fileStatusKey, git, isExpanded, isMounted, loadFullFiles, setDiff, staged]); const handleToggle = React.useCallback(() => { handleOpenChange(!isExpanded); handleSelect(); }, [handleOpenChange, handleSelect, isExpanded]); - const handleHunkApplied = React.useCallback(() => { - setDiffRetryNonce((nonce) => nonce + 1); - if (directory) { - void fetchStatus(directory, git); + const handleFileAction = React.useCallback(async (action: FileDiffAction) => { + if (!directory || fileAction !== null) { + return; } - }, [directory, fetchStatus, git]); + + setFileAction(action); + try { + if (action === 'stage') { + await git.stageGitFile(directory, file.path); + } else if (action === 'unstage') { + await git.unstageGitFile(directory, file.path); + } else { + await git.revertGitFile(directory, file.path, { scope: 'working' }); + } + setDiffRetryNonce((nonce) => nonce + 1); + await fetchStatus(directory, git); + } catch (error) { + const fallbackKey = action === 'unstage' + ? 'gitView.toast.unstageFileFailed' + : action === 'stage' + ? 'gitView.toast.stageFileFailed' + : 'gitView.toast.revertFailed'; + toast.error(error instanceof Error ? error.message : t(fallbackKey)); + } finally { + setFileAction((current) => (current === action ? null : current)); + } + }, [directory, fetchStatus, file.path, fileAction, git, t]); return (
-
-
- +
{isExpanded && (
@@ -775,15 +881,25 @@ const MultiFileDiffEntry = React.memo(({
) : null} {isMounted && diffData && (forceRenderLarge || (file.insertions + file.deletions) <= LARGE_DIFF_CHANGED_LINES) ? ( - + <> + +
+
+ +
+
+ ) : null} )} @@ -825,11 +941,6 @@ export const DiffView: React.FC = ({ const ensureStatus = useGitStore((state) => state.ensureStatus); const fetchStatus = useGitStore((state) => state.fetchStatus); const setDiff = useGitStore((state) => state.setDiff); - const indexRevision = useGitStore(React.useCallback((state) => { - if (!effectiveDirectory) return 0; - return state.directories.get(effectiveDirectory)?.indexRevision ?? 0; - }, [effectiveDirectory])); - const [displayFile, setDisplayFile] = React.useState(null); const [displayFileStaged, setDisplayFileStaged] = React.useState(false); const [pinnedStackedTarget, setPinnedStackedTarget] = React.useState(null); @@ -859,6 +970,7 @@ export const DiffView: React.FC = ({ const pendingScrollFrameRef = React.useRef(null); const shouldPinAfterAlignRef = React.useRef(false); const visibleSyncFrameRef = React.useRef(null); + const stackedStateScopeRef = React.useRef(null); const cancelPendingScrollAlignment = React.useCallback(() => { pendingScrollTargetRef.current = null; @@ -918,13 +1030,48 @@ export const DiffView: React.FC = ({ React.useEffect(() => { const paths = changedFilePathsKey ? changedFilePathsKey.split('\0') : []; - const defaultExpandedCount = stackedDefaultCollapsedAll - ? 0 - : getStackedViewDefaultExpandedCount(paths.length); - const defaultExpanded = new Set(paths.slice(0, defaultExpandedCount)); - setExpandedFiles(defaultExpanded); - setMountedStackedFiles(new Set()); - }, [changedFilePathsKey, stackedDefaultCollapsedAll]); + const pathSet = new Set(paths); + const scopeKey = `${effectiveDirectory ?? ''}:${diffScope}:${stackedDefaultCollapsedAll ? 'collapsed' : 'default'}`; + const shouldInitialize = stackedStateScopeRef.current !== scopeKey; + stackedStateScopeRef.current = scopeKey; + + setExpandedFiles((previous) => { + if (shouldInitialize) { + const defaultExpandedCount = stackedDefaultCollapsedAll + ? 0 + : getStackedViewDefaultExpandedCount(paths.length); + return new Set(paths.slice(0, defaultExpandedCount)); + } + + let changed = false; + const next = new Set(); + for (const path of previous) { + if (!pathSet.has(path)) { + changed = true; + continue; + } + next.add(path); + } + return changed ? next : previous; + }); + + setMountedStackedFiles((previous) => { + if (shouldInitialize) { + return new Set(); + } + + let changed = false; + const next = new Set(); + for (const path of previous) { + if (!pathSet.has(path)) { + changed = true; + continue; + } + next.add(path); + } + return changed ? next : previous; + }); + }, [changedFilePathsKey, diffScope, effectiveDirectory, stackedDefaultCollapsedAll]); const syncVisibleStackedFiles = React.useCallback(() => { visibleSyncFrameRef.current = null; @@ -1314,41 +1461,42 @@ export const DiffView: React.FC = ({ /> )} - -
- {changedFiles.map((file) => ( - { - void openFileInEditorAtChange(filePath, diffData); - }} - staged={getFileStaged(file.path)} - stagedRevision={indexRevision} - loadFullFiles={loadFullFiles} - /> - ))} -
- +
+ +
+ {changedFiles.map((file) => ( + { + void openFileInEditorAtChange(filePath, diffData); + }} + staged={getFileStaged(file.path)} + loadFullFiles={loadFullFiles} + /> + ))} +
+ +
); }; diff --git a/packages/ui/src/components/views/PierreDiffViewer.tsx b/packages/ui/src/components/views/PierreDiffViewer.tsx index 0e37ad5d..ba8d0f01 100644 --- a/packages/ui/src/components/views/PierreDiffViewer.tsx +++ b/packages/ui/src/components/views/PierreDiffViewer.tsx @@ -1099,9 +1099,9 @@ export const PierreDiffViewer: React.FC = ({ return (
-
-
- {commentOverlays} +
+ {commentOverlays} +
); }; diff --git a/packages/vscode/src/gitService.ts b/packages/vscode/src/gitService.ts index 8ef2c0c2..a232ef7c 100644 --- a/packages/vscode/src/gitService.ts +++ b/packages/vscode/src/gitService.ts @@ -2327,6 +2327,67 @@ const HUNK_ACTION_ARGS: Record<'stage' | 'unstage' | 'discard', string[]> = { discard: ['--reverse'], }; +const parsePatchPathToken = (line: string): string | null => { + const value = String(line || '').replace(/^(?:-{3}|\+{3})\s+/, ''); + if (!value || value === '/dev/null') { + return null; + } + + if (value.startsWith('"')) { + let token = '"'; + let escaped = false; + for (let index = 1; index < value.length; index += 1) { + const char = value[index]; + token += char; + if (escaped) { + escaped = false; + } else if (char === '\\') { + escaped = true; + } else if (char === '"') { + break; + } + } + + try { + return JSON.parse(token) as string; + } catch { + return token.slice(1, token.endsWith('"') ? -1 : undefined); + } + } + + return value.split('\t', 1)[0] || null; +}; + +const normalizePatchTargetPath = (value: string | null): string | null => { + if (!value || value === '/dev/null') { + return null; + } + return value.replace(/^[ab]\//, '').replace(/\\/g, '/'); +}; + +const extractPatchTargetPath = (patch: string): string | null => { + const matches = [...patch.matchAll(/^(?:-{3}|\+{3})\s+.+$/gm)]; + const realTargets = matches + .map((match) => normalizePatchTargetPath(parsePatchPathToken(match[0] ?? ''))) + .filter((value): value is string => Boolean(value)); + return realTargets[0] || null; +}; + +const getRepoRelativePath = async (directory: string, filePath: string): Promise => { + const normalizedFilePath = normalizePath(filePath).replace(/\\/g, '/'); + if (!path.isAbsolute(normalizedFilePath)) { + return normalizedFilePath.replace(/^\.?\//, ''); + } + + const rootResult = await execGit(['rev-parse', '--show-toplevel'], directory); + if (rootResult.exitCode !== 0) { + throw new Error(rootResult.stderr || 'Failed to resolve repository root'); + } + + const repoRoot = normalizePath(rootResult.stdout.trim()); + return path.relative(repoRoot, normalizedFilePath).replace(/\\/g, '/'); +}; + /** * Apply a single-hunk patch to stage, unstage, or discard it. * The patch is written to a temp file and applied with `git apply`. @@ -2337,6 +2398,9 @@ export async function applyGitHunk( patch: string, action: 'stage' | 'unstage' | 'discard', ): Promise { + if (!HUNK_ACTION_ARGS[action]) { + throw new Error('Invalid hunk action'); + } if (!filePath) { throw new Error('path is required'); } @@ -2347,6 +2411,12 @@ export async function applyGitHunk( throw new Error('patch does not contain a hunk header'); } + const repoRelativePath = await getRepoRelativePath(directory, filePath); + const targetPath = extractPatchTargetPath(patch); + if (targetPath && targetPath !== repoRelativePath && targetPath !== filePath.replace(/\\/g, '/')) { + throw new Error('patch target path does not match the requested file'); + } + const flags = HUNK_ACTION_ARGS[action]; const tmpDir = os.tmpdir(); const tmpPath = path.join(tmpDir, `openchamber-hunk-${Date.now()}-${Math.random().toString(36).slice(2)}.patch`); diff --git a/packages/web/server/lib/git/service.js b/packages/web/server/lib/git/service.js index fe2899a4..ed86fd39 100644 --- a/packages/web/server/lib/git/service.js +++ b/packages/web/server/lib/git/service.js @@ -2561,11 +2561,49 @@ const HUNK_ACTION_FLAGS = { discard: ['--reverse'], }; +const parsePatchPathToken = (line) => { + const value = String(line || '').replace(/^(?:-{3}|\+{3})\s+/, ''); + if (!value || value === '/dev/null') { + return null; + } + + if (value.startsWith('"')) { + let token = '"'; + let escaped = false; + for (let index = 1; index < value.length; index += 1) { + const char = value[index]; + token += char; + if (escaped) { + escaped = false; + } else if (char === '\\') { + escaped = true; + } else if (char === '"') { + break; + } + } + + try { + return JSON.parse(token); + } catch { + return token.slice(1, token.endsWith('"') ? -1 : undefined); + } + } + + return value.split('\t', 1)[0] || null; +}; + +const normalizePatchTargetPath = (value) => { + if (!value || value === '/dev/null') { + return null; + } + return value.replace(/^[ab]\//, ''); +}; + const extractPatchTargetPath = (patch) => { - const matches = [...patch.matchAll(/^(?:-{3}|\+{3})\s+(?:[ab]\/)?([^\s\t]+)/gm)]; + const matches = [...patch.matchAll(/^(?:-{3}|\+{3})\s+.+$/gm)]; const realTargets = matches - .map((match) => match[1]) - .filter((value) => value && value !== '/dev/null'); + .map((match) => normalizePatchTargetPath(parsePatchPathToken(match[0]))) + .filter(Boolean); return realTargets[0] || null; }; diff --git a/packages/web/server/lib/git/service.test.js b/packages/web/server/lib/git/service.test.js index e38e100b..fb67b276 100644 --- a/packages/web/server/lib/git/service.test.js +++ b/packages/web/server/lib/git/service.test.js @@ -240,6 +240,25 @@ describe('applyHunk', () => { 'patch target path does not match' ); }); + + it('accepts hunk patches for files with spaces in their path', async () => { + if (!canRunGit()) return; + const { tmpDir, git } = await createTempRepo(); + const filePath = 'file name.txt'; + await writeFile(tmpDir, filePath, ORIGINAL_FILE); + await git.add(filePath); + await git.commit('Initial'); + + await writeFile(tmpDir, filePath, EDITED_FILE); + const diff = await getDiff(tmpDir, { path: filePath }); + const hunks = splitHunks(diff); + expect(hunks.length).toBe(2); + + await applyHunk(tmpDir, filePath, { patch: hunks[0], action: 'stage' }); + + const staged = (await git.raw(['show', `:${filePath}`])).replace(/\r\n/g, '\n'); + expect(staged).toBe(makeFile('TOP', 'line20')); + }); }); // ---------------------------------------------------------------------------