From 833a707182014d54c24ebb2c7685ffe19f8b97bf Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 18 May 2026 23:41:02 -0700 Subject: [PATCH] fix: address review findings (#2311) --- .../editor/ConflictComponents.test.tsx | 130 +++++++++- .../components/editor/ConflictComponents.tsx | 244 ++++++++++++++---- .../src/components/editor/EditorContent.tsx | 199 +++++++++++--- .../editor/useEditorPanelContentState.ts | 15 +- 4 files changed, 487 insertions(+), 101 deletions(-) diff --git a/src/renderer/src/components/editor/ConflictComponents.test.tsx b/src/renderer/src/components/editor/ConflictComponents.test.tsx index 0bca618c0..e4d0d23f9 100644 --- a/src/renderer/src/components/editor/ConflictComponents.test.tsx +++ b/src/renderer/src/components/editor/ConflictComponents.test.tsx @@ -2,7 +2,12 @@ import { renderToStaticMarkup } from 'react-dom/server' import { describe, expect, it, vi } from 'vitest' import type { OpenFile } from '@/store/slices/editor' import type { GitStatusEntry } from '../../../../shared/types' -import { ConflictReviewPanel } from './ConflictComponents' +import { TooltipProvider } from '@/components/ui/tooltip' +import { + ConflictBanner, + ConflictReviewPanel, + getNextConflictNavigationIndex +} from './ConflictComponents' function createConflictReviewFile(overrides: Partial = {}): OpenFile { return { @@ -46,7 +51,35 @@ function createLiveEntry( } describe('ConflictReviewPanel', () => { - it('renders unresolved conflicts as a left file tree', () => { + it('renders unresolved conflicts as a left tree with supplied default content', () => { + const file = createConflictReviewFile() + const html = renderToStaticMarkup( + file contents} + onDismiss={vi.fn()} + onRefreshSnapshot={vi.fn()} + onReturnToSourceControl={vi.fn()} + /> + ) + + expect(html).toContain('Files') + expect(html).toContain('Collapse file tree') + expect(html).toContain('src/renderer/src/store/slices') + expect(html).toContain('linear.test.ts') + expect(html).toContain('linear.ts') + expect(html).toContain('data-conflict-review-default-content="true"') + expect(html).toContain('file contents') + expect(html).not.toContain('Select a conflict from the file tree') + }) + + it('renders a lightweight overview when no conflict file is selected', () => { const file = createConflictReviewFile() const html = renderToStaticMarkup( { /> ) - expect(html).toContain('Files') - expect(html).toContain('Collapse file tree') - expect(html).toContain('src/renderer/src/store/slices') - expect(html).toContain('linear.test.ts') - expect(html).toContain('linear.ts') - expect(html).toContain('Select a conflict from the file tree') - expect(html).not.toContain('Choose which version to keep, or combine them') + expect(html).toContain('Conflicts') + expect(html).toContain('Open a file to resolve markers in the editor.') + expect(html).toContain('Choose which version to keep, or combine them') + expect(html).toContain('Resolve the conflict markers') + expect(html).not.toContain('Loading conflict contents') + }) + + it('keeps selected conflict content in a flex-height pane', () => { + const file = createConflictReviewFile() + const html = renderToStaticMarkup( + } + onDismiss={vi.fn()} + onRefreshSnapshot={vi.fn()} + onReturnToSourceControl={vi.fn()} + /> + ) + + expect(html).toContain( + 'class="flex min-h-0 flex-1 flex-col">
{ + it('renders conflict navigation controls when conflict markers are available', () => { + const file = createConflictReviewFile({ + mode: 'edit', + conflict: { + kind: 'conflict-editable', + conflictKind: 'both_modified', + conflictStatus: 'unresolved', + conflictStatusSource: 'git' + } + }) + const html = renderToStaticMarkup( + + + + ) + + expect(html).toContain('Unresolved conflict') + expect(html).toContain('2 / 3') + expect(html).toContain('Previous conflict') + expect(html).toContain('Next conflict') + }) +}) + +describe('getNextConflictNavigationIndex', () => { + it('cycles through conflicts in both directions', () => { + expect( + getNextConflictNavigationIndex({ currentIndex: null, direction: 'next', total: 3 }) + ).toBe(0) + expect(getNextConflictNavigationIndex({ currentIndex: 2, direction: 'next', total: 3 })).toBe(0) + expect( + getNextConflictNavigationIndex({ currentIndex: 0, direction: 'previous', total: 3 }) + ).toBe(2) + expect( + getNextConflictNavigationIndex({ currentIndex: null, direction: 'previous', total: 3 }) + ).toBe(2) + expect(getNextConflictNavigationIndex({ currentIndex: 0, direction: 'next', total: 0 })).toBe( + null + ) }) }) diff --git a/src/renderer/src/components/editor/ConflictComponents.tsx b/src/renderer/src/components/editor/ConflictComponents.tsx index b2d00ad65..5960fe392 100644 --- a/src/renderer/src/components/editor/ConflictComponents.tsx +++ b/src/renderer/src/components/editor/ConflictComponents.tsx @@ -1,7 +1,17 @@ import React from 'react' -import { CircleCheck, GitMerge, PanelLeftOpen, RefreshCw, TriangleAlert, X } from 'lucide-react' +import { + ChevronDown, + ChevronUp, + CircleCheck, + GitMerge, + PanelLeftOpen, + RefreshCw, + TriangleAlert, + X +} from 'lucide-react' import { Button } from '@/components/ui/button' import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip' +import { getFileTypeIcon } from '@/lib/file-type-icons' import { cn } from '@/lib/utils' import type { ConflictReviewEntry, OpenFile } from '@/store/slices/editor' import type { GitConflictKind, GitStatusEntry } from '../../../../shared/types' @@ -29,13 +39,41 @@ export const CONFLICT_HINT_MAP: Record = { const EMPTY_CONFLICT_REVIEW_ENTRIES: readonly ConflictReviewEntry[] = [] let conflictReviewFileTreeCollapsedPreference = false +type ConflictNavigationDirection = 'previous' | 'next' +type ConflictReviewPanelEntry = ConflictReviewEntry & { + liveEntry?: GitStatusEntry +} + +export function getNextConflictNavigationIndex({ + currentIndex, + direction, + total +}: { + currentIndex: number | null + direction: ConflictNavigationDirection + total: number +}): number | null { + if (total <= 0) { + return null + } + if (currentIndex === null || currentIndex < 0 || currentIndex >= total) { + return direction === 'previous' ? total - 1 : 0 + } + return direction === 'previous' ? (currentIndex + total - 1) % total : (currentIndex + 1) % total +} export function ConflictBanner({ file, - entry + entry, + conflictNavigation }: { file: OpenFile entry: GitStatusEntry | null + conflictNavigation?: { + currentIndex: number | null + total: number + onJump: (direction: ConflictNavigationDirection) => void + } }): React.JSX.Element | null { const conflict = file.conflict if (!conflict) { @@ -54,15 +92,58 @@ export function ConflictBanner({ : 'border-emerald-500/20 bg-emerald-500/5' )} > -
- {isUnresolved ? ( - - ) : ( - +
+
+ {isUnresolved ? ( + + ) : ( + + )} + + {label} conflict · {CONFLICT_KIND_LABELS[conflict.conflictKind]} + + {conflictNavigation && conflictNavigation.total > 0 && ( + + {(conflictNavigation.currentIndex ?? 0) + 1} / {conflictNavigation.total} + + )} +
+ {conflictNavigation && conflictNavigation.total > 0 && ( +
+ + + + + + Previous conflict + + + + + + + + Next conflict + + +
)} - - {label} conflict · {CONFLICT_KIND_LABELS[conflict.conflictKind]} -
{/* Why: the hint is omitted here because the file is already open in the editor below. Showing "Open and edit…" or similar would be confusing @@ -104,6 +185,105 @@ export function ConflictPlaceholderView({ file }: { file: OpenFile }): React.JSX ) } +function ConflictReviewOverview({ + entries, + resolvedCount, + onOpenEntry, + onReturnToSourceControl, + onDismiss +}: { + entries: readonly ConflictReviewPanelEntry[] + resolvedCount: number + onOpenEntry: (entry: GitStatusEntry) => void + onReturnToSourceControl: () => void + onDismiss: () => void +}): React.JSX.Element { + return ( +
+
+
+
+
Conflicts
+
+ Open a file to resolve markers in the editor. +
+
+
+ + +
+
+ {resolvedCount > 0 && ( +
+ {resolvedCount} conflict{resolvedCount === 1 ? '' : 's'} no longer live in Git. +
+ )} +
+ {entries.map((entry) => { + const FileIcon = getFileTypeIcon(entry.path) + const liveEntry = entry.liveEntry + const isUnresolved = liveEntry?.conflictStatus === 'unresolved' + const statusLabel = isUnresolved ? 'Unresolved' : liveEntry ? 'Resolved' : 'Gone' + + return ( + + ) + })} +
+
+
+ ) +} + export function ConflictReviewPanel({ file, liveEntries, @@ -131,7 +311,7 @@ export function ConflictReviewPanel({ () => new Map(liveEntries.map((entry) => [entry.path, entry])), [liveEntries] ) - const treeEntries = React.useMemo( + const treeEntries = React.useMemo( () => snapshotEntries.map((entry) => ({ ...entry, @@ -219,41 +399,15 @@ export function ConflictReviewPanel({ Refresh
-
+
{selectedContent ?? ( -
-
-
- -
-
- Select a conflict from the file tree -
-
- Open each unresolved file, edit the conflict markers, then refresh this snapshot. -
- {resolvedCount > 0 && ( -
- {resolvedCount} conflict{resolvedCount === 1 ? '' : 's'} no longer live in Git. -
- )} -
- - -
-
-
+ )}
diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index 3107d9ecb..b83575666 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -10,7 +10,12 @@ import { detectLanguage } from '@/lib/language-detect' import { useAppStore } from '@/store' import { Button } from '@/components/ui/button' import { ChangesModeView } from './ChangesModeView' -import { ConflictBanner, ConflictPlaceholderView, ConflictReviewPanel } from './ConflictComponents' +import { + ConflictBanner, + ConflictPlaceholderView, + ConflictReviewPanel, + getNextConflictNavigationIndex +} from './ConflictComponents' import type { MarkdownViewMode, OpenFile } from '@/store/slices/editor' import type { GitStatusEntry, GitDiffResult } from '../../../../shared/types' import { RICH_MARKDOWN_MAX_SIZE_BYTES } from '../../../../shared/constants' @@ -19,6 +24,7 @@ import { getMarkdownRichModeUnsupportedMessage } from './markdown-rich-mode' import { extractFrontMatter, prependFrontMatter } from './markdown-frontmatter' import { RichMarkdownErrorBoundary } from './RichMarkdownErrorBoundary' import { useMarkdownDocuments } from './useMarkdownDocuments' +import { findGitConflictBlocks } from './monaco-conflict-decorations' const MonacoEditor = lazy(() => import('./MonacoEditor')) const DiffViewer = lazy(() => import('./DiffViewer')) @@ -146,6 +152,10 @@ export function EditorContent({ const openConflictReview = useAppStore((s) => s.openConflictReview) const closeFile = useAppStore((s) => s.closeFile) const setRightSidebarTab = useAppStore((s) => s.setRightSidebarTab) + const setPendingEditorReveal = useAppStore((s) => s.setPendingEditorReveal) + const [conflictNavigationIndexByFile, setConflictNavigationIndexByFile] = React.useState< + Record + >({}) const md = useMarkdownDocuments(activeFile, isMarkdown, mdViewMode, handleSave) const activeConflictEntry = worktreeEntries.find((entry) => entry.path === activeFile.relativePath) ?? null @@ -160,6 +170,68 @@ export function EditorContent({ activeFile.diffSource === 'combined-branch' || activeFile.diffSource === 'combined-commit') + const getConflictNavigation = React.useCallback( + (file: OpenFile, content: string) => { + const blocks = findGitConflictBlocks(content) + if (blocks.length === 0) { + return undefined + } + + const currentIndex = conflictNavigationIndexByFile[file.id] ?? null + return { + currentIndex, + total: blocks.length, + onJump: (direction: 'previous' | 'next') => { + const nextIndex = getNextConflictNavigationIndex({ + currentIndex, + direction, + total: blocks.length + }) + if (nextIndex === null) { + return + } + const line = blocks[nextIndex].startLine + const markerLine = content.split(/\r?\n/)[line - 1] ?? '' + setConflictNavigationIndexByFile((prev) => ({ ...prev, [file.id]: nextIndex })) + // Why: a same-location reveal can be requested twice before Monaco + // consumes the first one. Clearing first guarantees the prop changes + // and the mounted editor runs its reveal effect again. + setPendingEditorReveal(null) + queueMicrotask(() => { + setPendingEditorReveal({ + filePath: file.filePath, + line, + column: 1, + matchLength: markerLine.length + }) + }) + } + } + }, + [conflictNavigationIndexByFile, setPendingEditorReveal] + ) + const openConflictEntry = React.useCallback( + (entry: GitStatusEntry) => { + if (activeFile.mode !== 'conflict-review') { + return + } + openConflictReviewFile( + activeFile.id, + activeFile.worktreeId, + activeFile.filePath, + entry, + detectLanguage(entry.path) + ) + }, + [ + activeFile.filePath, + activeFile.id, + activeFile.mode, + activeFile.worktreeId, + openConflictReviewFile + ] + ) + const renderMonacoEditor = (fc: FileContent): React.JSX.Element => ( // Why: Without a key, React reuses the same MonacoEditor instance when // switching tabs or split panes, just updating props. That means @@ -324,77 +396,105 @@ export function EditorContent({ return
{renderMonacoEditor(fc)}
} - const renderConflictReviewSelectedContent = (selectedFile: OpenFile): React.JSX.Element => { - if (selectedFile.conflict?.kind === 'conflict-placeholder') { - return + const renderConflictReviewEditorContent = ({ + contentFile, + entry, + className, + viewStateKeySuffix + }: { + contentFile: OpenFile + entry: GitStatusEntry | null + className: string + viewStateKeySuffix: string + }): React.JSX.Element => { + if (contentFile.conflict?.kind === 'conflict-placeholder') { + return ( +
+ +
+ ) } - const fc = fileContents[selectedFile.id] + const fc = fileContents[contentFile.id] if (!fc) { return ( -
- Loading... +
+
+ Loading... +
) } if (fc.loadError) { return ( - reloadFileContent(selectedFile)} /> +
+ reloadFileContent(contentFile)} + /> +
) } if (fc.isBinary) { if (fc.isImage) { return ( - +
+ +
) } return ( -
- Binary file — cannot display +
+
+ Binary file — cannot display +
) } - const selectedConflictEntry = - worktreeEntries.find((entry) => entry.path === selectedFile.relativePath) ?? null - const selectedLanguage = detectLanguage(selectedFile.relativePath) + const selectedLanguage = detectLanguage(contentFile.relativePath) const monacoSelectedLanguage = selectedLanguage === 'notebook' ? 'json' : selectedLanguage - const selectedViewStateKey = `${selectedFile.filePath}::${viewStateScopeId}` + const selectedViewStateKey = `${contentFile.filePath}::${viewStateScopeId}:${viewStateKeySuffix}` + const selectedContent = editBuffers[contentFile.id] ?? fc.content return ( -
- {selectedFile.conflict && ( - +
+ {contentFile.conflict && ( + )}
handleContentChangeForFile(selectedFile, content)} - onSave={(content) => handleSaveForFile(selectedFile, content)} - worktreeId={selectedFile.worktreeId} + onContentChange={(content) => handleContentChangeForFile(contentFile, content)} + onSave={(content) => handleSaveForFile(contentFile, content)} + worktreeId={contentFile.worktreeId} markdownAnnotationsEnabled={false} - conflictDecorationsEnabled={selectedFile.conflict?.conflictStatus === 'unresolved'} + conflictDecorationsEnabled={contentFile.conflict?.conflictStatus === 'unresolved'} revealLine={ - pendingEditorReveal?.filePath === selectedFile.filePath + pendingEditorReveal?.filePath === contentFile.filePath ? pendingEditorReveal.line : undefined } revealColumn={ - pendingEditorReveal?.filePath === selectedFile.filePath + pendingEditorReveal?.filePath === contentFile.filePath ? pendingEditorReveal.column : undefined } revealMatchLength={ - pendingEditorReveal?.filePath === selectedFile.filePath + pendingEditorReveal?.filePath === contentFile.filePath ? pendingEditorReveal.matchLength : undefined } @@ -404,20 +504,24 @@ export function EditorContent({ ) } + const renderConflictReviewSelectedContent = (selectedFile: OpenFile): React.JSX.Element => { + const selectedConflictEntry = + worktreeEntries.find((entry) => entry.path === selectedFile.relativePath) ?? null + + return renderConflictReviewEditorContent({ + contentFile: selectedFile, + entry: selectedConflictEntry, + className: 'flex min-h-0 flex-1 flex-col', + viewStateKeySuffix: 'selected' + }) + } + if (activeFile.mode === 'conflict-review') { return ( - openConflictReviewFile( - activeFile.id, - activeFile.worktreeId, - activeFile.filePath, - entry, - detectLanguage(entry.path) - ) - } + onOpenEntry={openConflictEntry} selectedFile={selectedConflictReviewFile} selectedContent={ selectedConflictReviewFile @@ -540,7 +644,16 @@ export function EditorContent({ } return (
- {activeFile.conflict && } + {activeFile.conflict && ( + + )}
{isMarkdown ? ( renderMarkdownContent(fc) diff --git a/src/renderer/src/components/editor/useEditorPanelContentState.ts b/src/renderer/src/components/editor/useEditorPanelContentState.ts index 9ef819ec4..acd5d8a64 100644 --- a/src/renderer/src/components/editor/useEditorPanelContentState.ts +++ b/src/renderer/src/components/editor/useEditorPanelContentState.ts @@ -81,7 +81,12 @@ export function useEditorPanelContentState({ : null const loadFileContent = useCallback( - async (filePath: string, id: string, worktreeId?: string): Promise => { + async ( + filePath: string, + id: string, + worktreeId?: string, + relativePath?: string + ): Promise => { try { const connectionId = getConnectionId(worktreeId ?? null) ?? undefined const restoredOpenFile = openFilesRef.current.find((file) => file.id === id) @@ -108,7 +113,7 @@ export function useEditorPanelContentState({ pending = readRuntimeFileContent({ settings: readSettings, filePath, - relativePath: restoredOpenFile?.relativePath, + relativePath: restoredOpenFile?.relativePath ?? relativePath, worktreeId, connectionId }) as Promise @@ -246,7 +251,7 @@ export function useEditorPanelContentState({ delete next[file.id] return next }) - void loadFileContent(file.filePath, file.id, file.worktreeId) + void loadFileContent(file.filePath, file.id, file.worktreeId, file.relativePath) }, [loadFileContent] ) @@ -281,8 +286,10 @@ export function useEditorPanelContentState({ activeFile?.id, activeFile?.mode, activeFile?.conflictReview?.selectedFileId, + activeFile?.conflictReview?.snapshotTimestamp, selectedConflictReviewFile?.id, - isChangesMode + isChangesMode, + gitStatusByWorktree ]) useEditorPanelFileLoadRetry({