diff --git a/desktop/src/components/chat/AttachmentGallery.test.tsx b/desktop/src/components/chat/AttachmentGallery.test.tsx index 9e26a81d..d435074c 100644 --- a/desktop/src/components/chat/AttachmentGallery.test.tsx +++ b/desktop/src/components/chat/AttachmentGallery.test.tsx @@ -2,10 +2,43 @@ import '@testing-library/jest-dom' import { fireEvent, render } from '@testing-library/react' -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { useSettingsStore } from '../../stores/settingsStore' import { AttachmentGallery } from './AttachmentGallery' describe('AttachmentGallery', () => { + beforeEach(() => { + useSettingsStore.setState({ locale: 'en' }) + }) + + it('renders diff comments as note-first composer cards with side-aware locations', () => { + const view = render( + , + ) + + const card = view.getByTestId('diff-comment-card') + expect(card.textContent).toContain('src/a.ts · new L11-L12') + expect(card.textContent).toContain('Use a shared helper') + expect(card.textContent).toContain('const result = buildResult() return result') + expect(card.textContent?.indexOf('Use a shared helper')).toBeLessThan( + card.textContent?.indexOf('const result = buildResult()') ?? -1, + ) + }) + it('renders a compact quote preview for selected workspace text', () => { render( { expect(tooltip).toHaveTextContent('这个标题更轻一点') expect(tooltip.className).toContain('group-hover/selection:visible') }) + + it('localizes diff sides and remove actions in Chinese', () => { + useSettingsStore.setState({ locale: 'zh' }) + const view = render( + , + ) + + expect(view.getByTestId('diff-comment-card')).toHaveTextContent('src/a.ts · 新 L11') + expect(view.getByRole('button', { name: '移除 a.ts' })).toBeInTheDocument() + }) }) diff --git a/desktop/src/components/chat/AttachmentGallery.tsx b/desktop/src/components/chat/AttachmentGallery.tsx index 9f597eea..721ca2bf 100644 --- a/desktop/src/components/chat/AttachmentGallery.tsx +++ b/desktop/src/components/chat/AttachmentGallery.tsx @@ -1,4 +1,6 @@ import { useMemo, useState } from 'react' +import { MessageSquare, X } from 'lucide-react' +import { useTranslation } from '../../i18n' import { ImageGalleryModal } from './ImageGalleryModal' export type AttachmentPreview = { @@ -11,6 +13,8 @@ export type AttachmentPreview = { isDirectory?: boolean lineStart?: number lineEnd?: number + diffSide?: 'old' | 'new' + hunkId?: string note?: string quote?: string } @@ -22,6 +26,7 @@ type Props = { } export function AttachmentGallery({ attachments, variant = 'message', onRemove }: Props) { + const t = useTranslation() const [activeImageIndex, setActiveImageIndex] = useState(null) const images = useMemo( @@ -120,7 +125,7 @@ export function AttachmentGallery({ attachments, variant = 'message', onRemove } type="button" onClick={() => onRemove(attachment.id!)} className="absolute -right-1 -top-1 flex h-5 w-5 items-center justify-center rounded-full bg-[var(--color-error)] text-[10px] text-white opacity-0 transition-opacity group-hover:opacity-100" - aria-label={`Remove ${attachment.name}`} + aria-label={t('attachments.remove', { name: attachment.name })} > × @@ -129,6 +134,57 @@ export function AttachmentGallery({ attachments, variant = 'message', onRemove } ) } + if (attachment.diffSide) { + const lineRange = attachment.lineStart + ? `L${attachment.lineStart}${attachment.lineEnd && attachment.lineEnd !== attachment.lineStart ? `-L${attachment.lineEnd}` : ''}` + : '' + const location = [ + attachment.path || attachment.name, + '·', + t(`workspace.diffReview.side.${attachment.diffSide}`), + lineRange, + ] + .filter(Boolean) + .join(' ') + const note = attachment.note?.trim() + const quotePreview = attachment.quote?.trim().replace(/\s+/g, ' ') + + return ( +
+
+ ) + } + const lineLabel = attachment.lineStart ? `:L${attachment.lineStart}${attachment.lineEnd && attachment.lineEnd !== attachment.lineStart ? `-L${attachment.lineEnd}` : ''}` : '' @@ -164,7 +220,7 @@ export function AttachmentGallery({ attachments, variant = 'message', onRemove } type="button" onClick={() => onRemove(attachment.id!)} className={`${hasQuotePreview ? 'mt-0.5' : 'ml-0.5'} flex h-5 w-5 shrink-0 items-center justify-center rounded-full text-[var(--color-text-tertiary)] transition-colors hover:text-[var(--color-text-primary)]`} - aria-label={`Remove ${attachment.name}`} + aria-label={t('attachments.remove', { name: attachment.name })} > close diff --git a/desktop/src/components/chat/ChatInput.test.tsx b/desktop/src/components/chat/ChatInput.test.tsx index e7ba978d..992224fd 100644 --- a/desktop/src/components/chat/ChatInput.test.tsx +++ b/desktop/src/components/chat/ChatInput.test.tsx @@ -224,6 +224,35 @@ describe('ChatInput file mentions', () => { vi.unstubAllGlobals() }) + it('passes diff metadata to the composer card and clears the reference after send', async () => { + act(() => { + useWorkspaceChatContextStore.getState().addReference(sessionId, { + kind: 'code-comment', + path: 'src/a.ts', + absolutePath: '/repo/src/a.ts', + name: 'a.ts', + lineStart: 11, + lineEnd: 12, + diffSide: 'new', + hunkId: 'hunk-1', + note: 'Use a shared helper', + quote: 'const result = buildResult()\nreturn result', + }) + }) + + render() + + expect(screen.getByTestId('diff-comment-card')).toHaveTextContent('src/a.ts · new L11-L12') + expect(screen.getByTestId('diff-comment-card')).toHaveTextContent('Use a shared helper') + + fireEvent.keyDown(screen.getByRole('textbox'), { key: 'Enter' }) + + await waitFor(() => { + expect(useWorkspaceChatContextStore.getState().referencesBySession[sessionId]).toEqual([]) + }) + expect(screen.queryByTestId('diff-comment-card')).not.toBeInTheDocument() + }) + it('keeps unsent composer drafts isolated when switching between session tabs', async () => { const historySessionId = 'history-session' useTabStore.setState({ diff --git a/desktop/src/components/chat/ChatInput.tsx b/desktop/src/components/chat/ChatInput.tsx index e88394b6..fd5f9bdc 100644 --- a/desktop/src/components/chat/ChatInput.tsx +++ b/desktop/src/components/chat/ChatInput.tsx @@ -65,6 +65,8 @@ function workspaceReferenceToAttachment(reference: WorkspaceChatReference): Atta isDirectory: reference.isDirectory, lineStart: reference.lineStart, lineEnd: reference.lineEnd, + diffSide: reference.diffSide, + hunkId: reference.hunkId, note: reference.note, quote: reference.quote, } @@ -974,6 +976,7 @@ export function ChatInput({ variant = 'default', compact = false }: ChatInputPro return (
{ +const { openPreviewSpy, browserOpenSpy, openTargetSpy, ensureTargetsMock, panelState } = vi.hoisted(() => { const openPreviewSpy = vi.fn().mockResolvedValue(undefined) const browserOpenSpy = vi.fn() const openTargetSpy = vi.fn().mockResolvedValue(undefined) const ensureTargetsMock = vi.fn().mockResolvedValue(undefined) - return { openPreviewSpy, browserOpenSpy, openTargetSpy, ensureTargetsMock } + const panelState = { isOpen: false } + return { openPreviewSpy, browserOpenSpy, openTargetSpy, ensureTargetsMock, panelState } }) // Mock openTargetStore @@ -49,10 +50,10 @@ vi.mock('../../stores/browserPanelStore', () => ({ // Mock workspacePanelStore vi.mock('../../stores/workspacePanelStore', () => ({ useWorkspacePanelStore: Object.assign( - (selector: (s: { openPreview: () => Promise }) => unknown) => - selector({ openPreview: openPreviewSpy }), + (selector: (s: { openPreview: () => Promise; isPanelOpen: () => boolean }) => unknown) => + selector({ openPreview: openPreviewSpy, isPanelOpen: () => panelState.isOpen }), { - getState: vi.fn(() => ({ openPreview: openPreviewSpy })), + getState: vi.fn(() => ({ openPreview: openPreviewSpy, isPanelOpen: () => panelState.isOpen })), }, ), })) @@ -109,7 +110,7 @@ function makeCheckpoint(filesChanged: string[]): SessionTurnCheckpoint { } } -function renderCard(filesChanged: string[]) { +function renderCard(filesChanged: string[], isLatest = true) { const checkpoint = makeCheckpoint(filesChanged) return render( , ) @@ -136,6 +137,7 @@ describe('CurrentTurnChangeCard – rich file row (icon / name / type)', () => { vi.clearAllMocks() ensureTargetsMock.mockResolvedValue(undefined) openPreviewSpy.mockResolvedValue(undefined) + panelState.isOpen = false }) it('renders the filename (not just full path) for each file', () => { @@ -192,14 +194,14 @@ describe('CurrentTurnChangeCard – row opens the workspace diff', () => { const row = screen.getByRole('button', { name: /turnChangesOpenInWorkspaceAria/ }) fireEvent.click(row) // displayPath is the workDir-relative path (matches the workspace file tree) - expect(openPreviewSpy).toHaveBeenCalledWith('s1', 'src/main.ts', 'diff') + expect(openPreviewSpy).toHaveBeenCalledWith('s1', 'src/main.ts', 'diff', expect.objectContaining({ sourceTurnKey: 'msg-1' })) }) it('passes the workDir-relative displayPath (not the absolute path) to openPreview', () => { renderCard(['/w/proj/README.md']) const row = screen.getByRole('button', { name: /turnChangesOpenInWorkspaceAria/ }) fireEvent.click(row) - expect(openPreviewSpy).toHaveBeenCalledWith('s1', 'README.md', 'diff') + expect(openPreviewSpy).toHaveBeenCalledWith('s1', 'README.md', 'diff', expect.objectContaining({ sourceTurnKey: 'msg-1' })) }) it('clicking an outside-workspace html changed file opens the in-app browser via local-file', () => { @@ -216,7 +218,7 @@ describe('CurrentTurnChangeCard – row opens the workspace diff', () => { renderCard(['/other/place/notes.txt']) const row = screen.getByRole('button', { name: /turnChangesOpenInWorkspaceAria/ }) fireEvent.click(row) - expect(openPreviewSpy).toHaveBeenCalledWith('s1', '/other/place/notes.txt', 'file') + expect(openPreviewSpy).toHaveBeenCalledWith('s1', '/other/place/notes.txt', 'file', expect.objectContaining({ sourceTurnKey: 'msg-1' })) expect(browserOpenSpy).not.toHaveBeenCalled() }) @@ -264,11 +266,18 @@ describe('CurrentTurnChangeCard – open-with buttons', () => { expect(screen.getAllByRole('button', { name: 'openWith.title' })).toHaveLength(2) }) - it('hides the workspace chevron on rows that already show an open-with button', () => { + it('keeps open-with secondary while every row retains its workspace chevron', () => { renderCard(['/w/proj/README.md', '/w/proj/index.html', '/w/proj/src/main.ts']) expect(screen.getAllByRole('button', { name: 'openWith.title' })).toHaveLength(2) - expect(screen.getAllByText('chevron_right')).toHaveLength(1) + const rows = screen.getAllByRole('button', { name: /turnChangesOpenInWorkspaceAria/ }) + expect(rows.every((row) => row.querySelector('.lucide-chevron-right'))).toBe(true) + }) + + it('shows the same destination chevron on every changed-file row', () => { + const { container } = renderCard(['/w/proj/README.md', '/w/proj/src/main.ts']) + + expect(container.querySelectorAll('.lucide-chevron-right')).toHaveLength(2) }) it('clicking README.md open-with opens menu with workspace preview item', async () => { @@ -363,6 +372,36 @@ describe('CurrentTurnChangeCard – open-with buttons', () => { }) }) +describe('CurrentTurnChangeCard – conversation continuity', () => { + beforeEach(() => { + vi.clearAllMocks() + panelState.isOpen = false + openPreviewSpy.mockImplementation(async () => { + panelState.isOpen = true + }) + }) + + it('truthfully labels a historical row as opening the current workspace diff', () => { + renderCard(['/w/proj/src/main.ts'], false) + + expect(screen.getByText('chat.turnChangesCurrentWorkspaceDiff')).toBeInTheDocument() + }) + + it('records a stable opener id and semantic turn key before opening the diff', () => { + renderCard(['/w/proj/src/main.ts']) + const row = screen.getByRole('button', { name: /turnChangesOpenInWorkspaceAria/ }) + + fireEvent.click(row) + + expect(row.id).toContain('msg-1') + expect(row).toHaveAttribute('data-source-turn-key', 'msg-1') + expect(openPreviewSpy).toHaveBeenCalledWith('s1', 'src/main.ts', 'diff', { + sourceTurnKey: 'msg-1', + sourceElementId: row.id, + }) + }) +}) + describe('CurrentTurnChangeCard – collapse long file lists', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/desktop/src/components/chat/CurrentTurnChangeCard.tsx b/desktop/src/components/chat/CurrentTurnChangeCard.tsx index 896d3c7e..12808927 100644 --- a/desktop/src/components/chat/CurrentTurnChangeCard.tsx +++ b/desktop/src/components/chat/CurrentTurnChangeCard.tsx @@ -1,6 +1,6 @@ import { useCallback, useMemo, useState } from 'react' import type { MouseEvent as ReactMouseEvent } from 'react' -import { ChevronDown, ChevronUp } from 'lucide-react' +import { ChevronDown, ChevronRight, ChevronUp } from 'lucide-react' import type { SessionTurnCheckpoint } from '../../api/sessions' import { useTranslation, type TranslationKey } from '../../i18n' import { OpenWithMenu } from '../common/OpenWithMenu' @@ -59,7 +59,12 @@ export function CurrentTurnChangeCard({ ? files.slice(0, COLLAPSED_COUNT) : files - const openChangedFile = useCallback((fileEntry: ChangedFileEntry) => { + const openChangedFile = useCallback((event: ReactMouseEvent, fileEntry: ChangedFileEntry) => { + const renderItem = event.currentTarget.closest('[data-chat-render-item-key]') + const origin = { + sourceTurnKey: renderItem?.dataset.chatRenderItemKey ?? checkpoint.target.targetUserMessageId, + sourceElementId: event.currentTarget.id, + } // A changed file outside the workdir (absolute displayPath — e.g. another // drive) has no checkpoint baseline, so a diff is meaningless. Render html in // the in-app browser and everything else as a file preview (served by its @@ -69,14 +74,14 @@ export function CurrentTurnChangeCard({ useBrowserPanelStore.getState().open(sessionId, localFileUrl(getServerBaseUrl(), fileEntry.apiPath)) return } - void useWorkspacePanelStore.getState().openPreview(sessionId, fileEntry.displayPath, 'file') + void useWorkspacePanelStore.getState().openPreview(sessionId, fileEntry.displayPath, 'file', origin) return } // Jump to the right-side workspace and open a diff tab. We pass the workDir-relative // path (same format the workspace file tree passes to openPreview), so the diff tab // is keyed/fetched identically to the tree-driven one. - void useWorkspacePanelStore.getState().openPreview(sessionId, fileEntry.displayPath, 'diff') - }, [sessionId, files]) + void useWorkspacePanelStore.getState().openPreview(sessionId, fileEntry.displayPath, 'diff', origin) + }, [checkpoint.target.targetUserMessageId, sessionId, files]) const handleOpenWith = useCallback((event: ReactMouseEvent, fileEntry: ChangedFileEntry) => { event.stopPropagation() @@ -113,7 +118,7 @@ export function CurrentTurnChangeCard({ : t('chat.turnChangesHistoricalCardLabel') const subtitle = isLatest ? t('chat.turnChangesLatestSubtitle') - : t('chat.turnChangesHistoricalSubtitle') + : t('chat.turnChangesCurrentWorkspaceDiff') const undoLabel = isLatest ? t('chat.turnChangesLatestUndo') : t('chat.turnChangesHistoricalUndo') @@ -165,7 +170,9 @@ export function CurrentTurnChangeCard({
{previewable && ( + )}
= { - text: 'text', - typescript: 'typescript', - ts: 'typescript', - tsx: 'tsx', - javascript: 'javascript', - js: 'javascript', - jsx: 'jsx', - markdown: 'markdown', - md: 'markdown', - html: 'markup', - xml: 'markup', - shell: 'bash', - sh: 'bash', - zsh: 'bash', - diff: 'diff', - } - return map[lower] ?? lower -} - -export function getLanguageFromPath(path: string) { - return normalizePrismLanguage(getFileExtension(path) || 'text') -} - -export function InlineHighlightedCode({ - value, - language, -}: { - value: string - language: string -}) { - return ( - - {({ tokens, getTokenProps }) => ( - <> - {(tokens[0] ?? []).map((token, tokenIndex) => { - const { key: tokenKey, ...tokenProps } = getTokenProps({ token, key: tokenIndex }) - return - })} - - )} - - ) -} - -export function WorkspaceDiffSurface({ - value, - path, - className = 'min-h-0 flex-1 overflow-auto bg-[var(--color-code-bg)]', - lineLimit = WORKSPACE_PREVIEW_LINE_LIMIT, -}: { - value: string - path: string - className?: string - lineLimit?: number -}) { - const t = useTranslation() - const [showAllLines, setShowAllLines] = useState(false) - const lines = value.split('\n') - const visibleLines = showAllLines ? lines : lines.slice(0, lineLimit) - const language = getLanguageFromPath(path) - const usePlainLargePreview = showAllLines && lines.length > WORKSPACE_PLAIN_TEXT_LINE_THRESHOLD - - useEffect(() => { - setShowAllLines(false) - }, [path, value]) - - return ( -
-
-
-          {visibleLines.map((line, index) => {
-            const isFileHeader = line.startsWith('diff --') || line.startsWith('--- ') || line.startsWith('+++ ')
-            const isHunk = line.startsWith('@@')
-            const isAdded = line.startsWith('+') && !line.startsWith('+++')
-            const isRemoved = line.startsWith('-') && !line.startsWith('---')
-            const isCodeLine = isAdded || isRemoved || line.startsWith(' ')
-            const code = isCodeLine ? line.slice(1) : line
-            const prefix = isCodeLine ? line[0] : ' '
-
-            return (
-              
- - {index + 1} - - - {prefix} - - - {isCodeLine && !usePlainLargePreview ? ( - code ? : ' ' - ) : ( - code || ' ' - )} - -
- ) - })} -
- {lines.length > lineLimit && ( -
- - {showAllLines - ? t('workspace.previewAllLines', { total: lines.length }) - : t('workspace.previewLineLimit', { count: visibleLines.length, total: lines.length })} - - -
- )} -
-
- ) -} +export { + getFileExtension, + getLanguageFromPath, + InlineHighlightedCode, + normalizePrismLanguage, + WORKSPACE_PLAIN_TEXT_LINE_THRESHOLD, + WORKSPACE_PREVIEW_LINE_LIMIT, + WorkspaceDiffSurface, + workspacePrismTheme, +} from './WorkspaceDiffSurface' +export type { + WorkspaceDiffCommentSelection, + WorkspaceDiffSurfaceProps, +} from './WorkspaceDiffSurface' diff --git a/desktop/src/components/workspace/WorkspaceDiffSurface.test.tsx b/desktop/src/components/workspace/WorkspaceDiffSurface.test.tsx new file mode 100644 index 00000000..8a12c313 --- /dev/null +++ b/desktop/src/components/workspace/WorkspaceDiffSurface.test.tsx @@ -0,0 +1,353 @@ +import '@testing-library/jest-dom/vitest' +import { act, fireEvent, render, screen } from '@testing-library/react' +import type { ComponentProps } from 'react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { useSettingsStore } from '../../stores/settingsStore' +import { WorkspaceDiffSurface } from './WorkspaceDiffSurface' +import { + WORKSPACE_PLAIN_TEXT_LINE_THRESHOLD, + WORKSPACE_PREVIEW_LINE_LIMIT, + WorkspaceDiffSurface as ExportedWorkspaceDiffSurface, +} from './WorkspaceCodeSurface' + +const highlightRenderSpy = vi.hoisted(() => vi.fn()) + +vi.mock('prism-react-renderer', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + Highlight: (props: ComponentProps) => { + highlightRenderSpy() + return + }, + } +}) + +const diff = [ + 'diff --git a/src/a.ts b/src/a.ts', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -10,2 +10,3 @@', + ' const a = 1', + '-const b = 2', + '+const b = 3', + '+const c = 4', + '@@ -20 +21 @@', + '-old tail', + '+new tail', +].join('\n') + +function getCodeRow(text: string) { + const row = document.querySelector(`[data-row-text="${text}"]`) + expect(row).not.toBeNull() + return row! +} + +describe('WorkspaceDiffSurface', () => { + beforeEach(() => { + useSettingsStore.setState({ locale: 'en' }) + highlightRenderSpy.mockClear() + }) + + it('submits a forward range with its source coordinates and quote', () => { + const onAddComment = vi.fn() + render() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + expect(screen.getByRole('textbox', { name: 'Review comment' })).toHaveFocus() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 12' }), { shiftKey: true }) + expect(screen.getByText('new L11-L12')).toBeInTheDocument() + const rangeEndRow = getCodeRow('const c = 4').closest('[data-diff-row-id]') + const editorContainer = screen.getByRole('textbox', { name: 'Review comment' }).closest('[data-diff-editor]') + expect(rangeEndRow?.nextElementSibling).toBe(editorContainer) + expect(getCodeRow('const b = 3')).toHaveAttribute('data-selected', 'true') + expect(getCodeRow('const c = 4')).toHaveAttribute('data-selected', 'true') + + const editor = screen.getByRole('textbox', { name: 'Review comment' }) + fireEvent.change(editor, { target: { value: 'Use a shared helper' } }) + fireEvent.keyDown(editor, { key: 'Enter', metaKey: true }) + + expect(onAddComment).toHaveBeenCalledWith(expect.objectContaining({ + side: 'new', + lineStart: 11, + lineEnd: 12, + quote: 'const b = 3\nconst c = 4', + hunkId: 'file-0-hunk-0', + }), 'Use a shared helper') + }) + + it('normalizes reverse Shift selection', () => { + render() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 12' })) + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' }), { shiftKey: true }) + + expect(screen.getByText('new L11-L12')).toBeInTheDocument() + expect(getCodeRow('const b = 3')).toHaveAttribute('data-selected', 'true') + expect(getCodeRow('const c = 4')).toHaveAttribute('data-selected', 'true') + }) + + it('does not submit an empty review comment', () => { + const onAddComment = vi.fn() + render() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + fireEvent.keyDown(screen.getByRole('textbox', { name: 'Review comment' }), { key: 'Enter', ctrlKey: true }) + + expect(onAddComment).not.toHaveBeenCalled() + expect(screen.getByRole('textbox', { name: 'Review comment' })).toBeInTheDocument() + }) + + it('closes on Escape and restores focus to the anchor gutter button', () => { + render() + const anchor = screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' }) + + fireEvent.click(anchor) + fireEvent.keyDown(screen.getByRole('textbox', { name: 'Review comment' }), { key: 'Escape' }) + + expect(screen.queryByRole('textbox', { name: 'Review comment' })).not.toBeInTheDocument() + expect(anchor).toHaveFocus() + }) + + it('resets an incompatible Shift range and announces why', () => { + render() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts old line 20' }), { shiftKey: true }) + + expect(screen.getByText('Selection reset: choose lines from the same side and hunk.')).toBeInTheDocument() + expect(screen.getByText('old L20')).toBeInTheDocument() + }) + + it('uses one roving tab stop and supports Arrow, Home, End, and activation keys', () => { + render() + const buttons = screen.getAllByRole('button', { name: /Comment on src\/a\.ts/ }) + const firstButton = buttons[0]! + const secondButton = buttons[1]! + + expect(buttons.filter((button) => button.tabIndex === 0)).toHaveLength(1) + act(() => firstButton.focus()) + fireEvent.keyDown(firstButton, { key: 'ArrowDown' }) + expect(secondButton).toHaveFocus() + expect(secondButton).toHaveAttribute('tabindex', '0') + + fireEvent.keyDown(secondButton, { key: 'End' }) + expect(buttons.at(-1)).toHaveFocus() + fireEvent.keyDown(buttons.at(-1)!, { key: 'Home' }) + expect(firstButton).toHaveFocus() + fireEvent.keyDown(firstButton, { key: ' ' }) + expect(screen.getByRole('textbox', { name: 'Review comment' })).toHaveFocus() + }) + + it('keeps Shift+Home selection inside the current side and hunk', () => { + render() + const line10 = screen.getByRole('button', { name: 'Comment on src/a.ts new line 10' }) + const line11 = screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' }) + + act(() => line11.focus()) + fireEvent.keyDown(line11, { key: 'Home', shiftKey: true }) + + expect(line10).toHaveFocus() + expect(screen.getByText('new L10-L11')).toBeInTheDocument() + expect(getCodeRow('const a = 1')).toHaveAttribute('data-selected', 'true') + expect(getCodeRow('const b = 3')).toHaveAttribute('data-selected', 'true') + }) + + it('extends the range with Shift+Arrow and returns focus after submit', () => { + const onAddComment = vi.fn() + render() + const anchor = screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' }) + + act(() => anchor.focus()) + fireEvent.keyDown(anchor, { key: 'ArrowDown', shiftKey: true }) + expect(screen.getByText('new L11-L12')).toBeInTheDocument() + + const editor = screen.getByRole('textbox', { name: 'Review comment' }) + fireEvent.change(editor, { target: { value: 'Keep this focused' } }) + fireEvent.keyDown(editor, { key: 'Enter', ctrlKey: true }) + expect(onAddComment).toHaveBeenCalledOnce() + expect(anchor).toHaveFocus() + }) + + it('skips incompatible rows when extending with Shift+Arrow', () => { + render() + const anchor = screen.getByRole('button', { name: 'Comment on src/a.ts new line 10' }) + + act(() => anchor.focus()) + fireEvent.keyDown(anchor, { key: 'ArrowDown', shiftKey: true }) + + expect(screen.getByText('new L10-L11')).toBeInTheDocument() + expect(getCodeRow('const a = 1')).toHaveAttribute('data-selected', 'true') + expect(getCodeRow('const b = 3')).toHaveAttribute('data-selected', 'true') + }) + + it('keeps gutter focus for repeatable Shift+Arrow extension and shrinking', () => { + render() + const line10 = screen.getByRole('button', { name: 'Comment on src/a.ts new line 10' }) + const line11 = screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' }) + const line12 = screen.getByRole('button', { name: 'Comment on src/a.ts new line 12' }) + + act(() => line10.focus()) + fireEvent.keyDown(line10, { key: 'ArrowDown', shiftKey: true }) + expect(line11).toHaveFocus() + fireEvent.keyDown(line11, { key: 'ArrowDown', shiftKey: true }) + expect(line12).toHaveFocus() + expect(screen.getByText('new L10-L12')).toBeInTheDocument() + + fireEvent.keyDown(line12, { key: 'ArrowUp', shiftKey: true }) + expect(line11).toHaveFocus() + expect(screen.getByText('new L10-L11')).toBeInTheDocument() + }) + + it('keeps roving navigation on mounted rows when the preview is truncated', () => { + render() + const visibleButtons = screen.getAllByRole('button', { name: /Comment on src\/a\.ts/ }) + const lastVisibleButton = visibleButtons.at(-1)! + + act(() => lastVisibleButton.focus()) + fireEvent.keyDown(lastVisibleButton, { key: 'ArrowDown' }) + + expect(lastVisibleButton).toHaveFocus() + expect(visibleButtons.filter((button) => button.tabIndex === 0)).toHaveLength(1) + expect(screen.getByText('Showing first 5 of 11 loaded lines.')).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Show all loaded lines' })).toBeInTheDocument() + }) + + it('invalidates a hidden selection on collapse while preserving its draft and visible roving target', () => { + render() + fireEvent.click(screen.getByRole('button', { name: 'Show all loaded lines' })) + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + fireEvent.change(screen.getByRole('textbox', { name: 'Review comment' }), { + target: { value: 'Keep this collapsed draft' }, + }) + + fireEvent.click(screen.getByRole('button', { name: 'Collapse preview' })) + + expect(screen.queryByRole('textbox', { name: 'Review comment' })).not.toBeInTheDocument() + expect(screen.getByRole('status')).toHaveTextContent('Select visible lines again') + const visibleButtons = screen.getAllByRole('button', { name: /Comment on src\/a\.ts/ }) + expect(visibleButtons.filter((button) => button.tabIndex === 0)).toHaveLength(1) + expect(visibleButtons[0]).toHaveFocus() + + fireEvent.click(visibleButtons[0]!) + expect(screen.getByRole('textbox', { name: 'Review comment' })).toHaveValue('Keep this collapsed draft') + }) + + it('uses plain text instead of Prism after expanding a diff beyond the large preview threshold', () => { + const additions = Array.from( + { length: WORKSPACE_PLAIN_TEXT_LINE_THRESHOLD + 1 }, + (_, index) => `+const value${index} = ${index}`, + ) + const largeDiff = [ + 'diff --git a/src/large.ts b/src/large.ts', + '--- a/src/large.ts', + '+++ b/src/large.ts', + `@@ -0,0 +1,${additions.length} @@`, + ...additions, + ].join('\n') + render() + + fireEvent.click(screen.getByRole('button', { name: 'Show all loaded lines' })) + + expect(document.querySelector('.token')).not.toBeInTheDocument() + expect(getCodeRow('const value5000 = 5000')).toHaveTextContent('const value5000 = 5000') + }) + + it('renders parsed file headers and keeps multiple files visually separated', () => { + const multiFileDiff = [ + diff, + 'diff --git a/src/b.ts b/src/b.ts', + '--- a/src/b.ts', + '+++ b/src/b.ts', + '@@ -1 +1 @@', + '-export const before = true', + '+export const after = true', + ].join('\n') + + render() + + const headers = screen.getAllByTestId('workspace-diff-file-header') + expect(headers).toHaveLength(2) + expect(headers[0]).toHaveTextContent('diff --git a/src/a.ts b/src/a.ts') + expect(headers[1]).toHaveTextContent('diff --git a/src/b.ts b/src/b.ts') + }) + + it('renders TypeScript Prism tokens through the compatibility export without a circular runtime failure', () => { + render() + + const keyword = screen.getAllByText('const').find((element) => element.classList.contains('keyword')) + expect(keyword).toHaveClass('token', 'keyword') + expect(document.querySelectorAll('[data-row-text="const b = 3"]')).toHaveLength(1) + }) + + it('renders the complete review flow in Chinese', () => { + useSettingsStore.setState({ locale: 'zh' }) + render() + + const gutter = screen.getByRole('button', { name: '评论 src/a.ts 的新侧第 11 行' }) + fireEvent.click(gutter) + + expect(screen.getByRole('textbox', { name: '评审评论' })).toHaveFocus() + expect(screen.getByText('新 L11')).toBeInTheDocument() + expect(screen.getByRole('button', { name: '提交评审评论' })).toBeInTheDocument() + + fireEvent.click(screen.getByRole('button', { name: '评论 src/a.ts 的旧侧第 20 行' }), { shiftKey: true }) + expect(screen.getByRole('status')).toHaveTextContent('只能选择同一侧、同一变更块中的行') + }) + + it('does not rerun Prism highlighting for each controlled draft change', () => { + const additions = Array.from( + { length: WORKSPACE_PREVIEW_LINE_LIMIT - 4 }, + (_, index) => `+const value${index + 1} = ${index + 1}`, + ) + const nearLimitDiff = [ + 'diff --git a/src/near-limit.ts b/src/near-limit.ts', + '--- a/src/near-limit.ts', + '+++ b/src/near-limit.ts', + `@@ -0,0 +1,${additions.length} @@`, + ...additions, + ].join('\n') + render() + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/near-limit.ts new line 1' })) + const highlightCountBeforeTyping = highlightRenderSpy.mock.calls.length + const editor = screen.getByRole('textbox', { name: 'Review comment' }) + + fireEvent.change(editor, { target: { value: 'a' } }) + fireEvent.change(editor, { target: { value: 'ab' } }) + fireEvent.change(editor, { target: { value: 'abc' } }) + + expect(highlightRenderSpy).toHaveBeenCalledTimes(highlightCountBeforeTyping) + expect(editor).toHaveValue('abc') + }) + + it('preserves draft text but invalidates its selection when the diff changes', () => { + const { rerender } = render() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + fireEvent.change(screen.getByRole('textbox', { name: 'Review comment' }), { + target: { value: 'Draft survives refresh' }, + }) + rerender() + + expect(screen.queryByRole('textbox', { name: 'Review comment' })).not.toBeInTheDocument() + expect(screen.getByText('Diff changed. Select lines again to submit this comment.')).toBeInTheDocument() + + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + expect(screen.getByRole('textbox', { name: 'Review comment' })).toHaveValue('Draft survives refresh') + }) + + it('resets the editor and draft when the path changes', () => { + const { rerender } = render() + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/a.ts new line 11' })) + fireEvent.change(screen.getByRole('textbox', { name: 'Review comment' }), { + target: { value: 'Discard on another file' }, + }) + + rerender() + + expect(screen.queryByRole('textbox', { name: 'Review comment' })).not.toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Comment on src/b.ts new line 11' })) + expect(screen.getByRole('textbox', { name: 'Review comment' })).toHaveValue('') + }) +}) diff --git a/desktop/src/components/workspace/WorkspaceDiffSurface.tsx b/desktop/src/components/workspace/WorkspaceDiffSurface.tsx new file mode 100644 index 00000000..808e0d66 --- /dev/null +++ b/desktop/src/components/workspace/WorkspaceDiffSurface.tsx @@ -0,0 +1,556 @@ +import { + Fragment, + memo, + useEffect, + useMemo, + useRef, + useState, + type KeyboardEvent, + type MouseEvent, +} from 'react' +import { CornerDownLeft, MessageSquare, Plus } from 'lucide-react' +import { Highlight, type PrismTheme } from 'prism-react-renderer' +import { useTranslation } from '../../i18n' +import { + getCompatibleDiffRange, + parseWorkspaceDiff, + type WorkspaceDiffRow, + type WorkspaceDiffSelection, +} from './workspaceDiffModel' + +export const WORKSPACE_PREVIEW_LINE_LIMIT = 2000 +export const WORKSPACE_PLAIN_TEXT_LINE_THRESHOLD = 5000 + +export const workspacePrismTheme: PrismTheme = { + plain: { + color: 'var(--color-code-fg)', + backgroundColor: 'transparent', + }, + styles: [ + { types: ['comment', 'prolog', 'doctype', 'cdata'], style: { color: 'var(--color-code-comment)', fontStyle: 'italic' } }, + { types: ['string', 'attr-value', 'template-string'], style: { color: 'var(--color-code-string)' } }, + { types: ['keyword', 'selector', 'important', 'atrule'], style: { color: 'var(--color-code-keyword)' } }, + { types: ['function'], style: { color: 'var(--color-code-function)' } }, + { types: ['tag'], style: { color: 'var(--color-code-keyword)' } }, + { types: ['number', 'boolean'], style: { color: 'var(--color-code-number)' } }, + { types: ['operator'], style: { color: 'var(--color-code-fg)' } }, + { types: ['punctuation'], style: { color: 'var(--color-code-punctuation)' } }, + { types: ['variable', 'parameter'], style: { color: 'var(--color-code-fg)' } }, + { types: ['property', 'attr-name'], style: { color: 'var(--color-code-property)' } }, + { types: ['builtin', 'class-name', 'constant', 'symbol'], style: { color: 'var(--color-code-type)' } }, + { types: ['inserted'], style: { color: 'var(--color-code-inserted)' } }, + { types: ['deleted'], style: { color: 'var(--color-code-deleted)' } }, + ], +} + +export function getFileExtension(name: string) { + const cleanName = name.split('/').pop() ?? name + const lastDot = cleanName.lastIndexOf('.') + if (lastDot <= 0 || lastDot === cleanName.length - 1) return '' + return cleanName.slice(lastDot + 1).toLowerCase() +} + +export function normalizePrismLanguage(language: string) { + const lower = language.toLowerCase() + const map: Record = { + text: 'text', + typescript: 'typescript', + ts: 'typescript', + tsx: 'tsx', + javascript: 'javascript', + js: 'javascript', + jsx: 'jsx', + markdown: 'markdown', + md: 'markdown', + html: 'markup', + xml: 'markup', + shell: 'bash', + sh: 'bash', + zsh: 'bash', + diff: 'diff', + } + return map[lower] ?? lower +} + +export function getLanguageFromPath(path: string) { + return normalizePrismLanguage(getFileExtension(path) || 'text') +} + +export const InlineHighlightedCode = memo(function InlineHighlightedCode({ + value, + language, +}: { + value: string + language: string +}) { + return ( + + {({ tokens, getTokenProps }) => ( + <> + {(tokens[0] ?? []).map((token, tokenIndex) => { + const { key: tokenKey, ...tokenProps } = getTokenProps({ token, key: tokenIndex }) + return + })} + + )} + + ) +}) + +export interface WorkspaceDiffCommentSelection { + side: 'old' | 'new' + lineStart: number + lineEnd: number + quote: string + hunkId: string +} + +export interface WorkspaceDiffSurfaceProps { + value: string + path: string + className?: string + lineLimit?: number + onAddComment?: (selection: WorkspaceDiffCommentSelection, note: string) => void +} + +interface ReviewState { + anchorId: string | null + focusId: string | null + selection: WorkspaceDiffSelection | null + draft: string +} + +type ReviewStatus = 'selectionReset' | 'diffChanged' | 'collapsedSelection' | null + +const emptyReviewState: ReviewState = { + anchorId: null, + focusId: null, + selection: null, + draft: '', +} + +function rowTone(row: WorkspaceDiffRow) { + if (row.kind === 'addition') return 'bg-[var(--color-diff-added-bg)]' + if (row.kind === 'deletion') return 'bg-[var(--color-diff-removed-bg)]' + if (row.kind === 'hunk') return 'bg-[var(--color-diff-highlight-bg)]' + return 'hover:bg-[var(--color-surface-hover)]' +} + +function prefixTone(row: WorkspaceDiffRow) { + if (row.kind === 'addition') return 'text-[var(--color-diff-added-text)]' + if (row.kind === 'deletion') return 'text-[var(--color-diff-removed-text)]' + return 'text-[var(--color-text-tertiary)]' +} + +function codeTone(row: WorkspaceDiffRow) { + if (row.kind === 'metadata') return 'font-semibold text-[var(--color-text-secondary)]' + if (row.kind === 'hunk') return 'font-semibold text-[var(--color-warning)]' + return '' +} + +export function WorkspaceDiffSurface({ + value, + path, + className = 'min-h-0 flex-1 overflow-auto bg-[var(--color-code-bg)]', + lineLimit = WORKSPACE_PREVIEW_LINE_LIMIT, + onAddComment, +}: WorkspaceDiffSurfaceProps) { + const t = useTranslation() + const files = useMemo(() => parseWorkspaceDiff(value), [value]) + const rows = useMemo(() => files.flatMap((file) => file.rows), [files]) + const displayItemIds = useMemo( + () => files.flatMap((file) => [`${file.id}-header`, ...file.rows.map((row) => row.id)]), + [files], + ) + const [review, setReview] = useState(emptyReviewState) + const [status, setStatus] = useState(null) + const [showAllRows, setShowAllRows] = useState(false) + const visibleItemIds = useMemo( + () => new Set(showAllRows ? displayItemIds : displayItemIds.slice(0, lineLimit)), + [displayItemIds, lineLimit, showAllRows], + ) + const visibleRows = useMemo(() => rows.filter((row) => visibleItemIds.has(row.id)), [rows, visibleItemIds]) + const selectableRows = useMemo(() => visibleRows.filter((row) => row.selectable), [visibleRows]) + const usePlainLargePreview = showAllRows && rows.length > WORKSPACE_PLAIN_TEXT_LINE_THRESHOLD + const [rovingId, setRovingId] = useState(() => rows.find((row) => row.selectable)?.id ?? null) + const buttonRefs = useRef(new Map()) + const editorRef = useRef(null) + const shouldFocusEditor = useRef(false) + const pendingRovingFocus = useRef(null) + const previousPath = useRef(path) + const previousValue = useRef(value) + const selectedIds = new Set(review.selection?.rowIds ?? []) + const sideLabel = (side: 'old' | 'new') => t(`workspace.diffReview.side.${side}`) + + useEffect(() => { + const pathChanged = previousPath.current !== path + const valueChanged = previousValue.current !== value + previousPath.current = path + previousValue.current = value + + if (pathChanged) { + setReview(emptyReviewState) + setStatus(null) + setShowAllRows(false) + setRovingId(selectableRows[0]?.id ?? null) + return + } + + if (valueChanged) { + setReview((current) => ({ + ...current, + anchorId: null, + focusId: null, + selection: null, + })) + setStatus(review.draft ? 'diffChanged' : null) + setRovingId(selectableRows[0]?.id ?? null) + } + }, [path, review.draft, selectableRows, value]) + + useEffect(() => { + if (!rovingId || !selectableRows.some((row) => row.id === rovingId)) { + setRovingId(selectableRows[0]?.id ?? null) + } + const pendingId = pendingRovingFocus.current + if (pendingId && selectableRows.some((row) => row.id === pendingId)) { + pendingRovingFocus.current = null + setRovingId(pendingId) + buttonRefs.current.get(pendingId)?.focus() + } + }, [rovingId, selectableRows]) + + useEffect(() => { + if (review.selection && shouldFocusEditor.current) { + shouldFocusEditor.current = false + editorRef.current?.focus() + } + }, [review.selection]) + + const focusButton = (id: string | null) => { + if (id) buttonRefs.current.get(id)?.focus() + } + + const selectSingleRow = (row: WorkspaceDiffRow, resetStatus: ReviewStatus = null, focusEditor = false) => { + const selection = getCompatibleDiffRange(rows, row.id, row.id) + if (!selection) return + shouldFocusEditor.current = focusEditor + setReview((current) => ({ + ...current, + anchorId: row.id, + focusId: row.id, + selection, + })) + setStatus(resetStatus) + } + + const extendSelection = (row: WorkspaceDiffRow, focusEditor = false) => { + if (!review.anchorId) { + selectSingleRow(row, null, focusEditor) + return + } + const selection = getCompatibleDiffRange(rows, review.anchorId, row.id) + if (!selection) { + selectSingleRow(row, 'selectionReset', focusEditor) + return + } + shouldFocusEditor.current = focusEditor + setReview((current) => ({ ...current, focusId: row.id, selection })) + setStatus(null) + } + + const activateRow = (row: WorkspaceDiffRow, extend: boolean, focusEditor: boolean) => { + setRovingId(row.id) + if (extend) extendSelection(row, focusEditor) + else selectSingleRow(row, null, focusEditor) + } + + const handleRowClick = (event: MouseEvent, row: WorkspaceDiffRow) => { + activateRow(row, event.shiftKey, true) + } + + const moveRovingFocus = (row: WorkspaceDiffRow, direction: -1 | 1, extend: boolean) => { + const currentIndex = selectableRows.findIndex((candidate) => candidate.id === row.id) + const anchorRow = review.anchorId + ? selectableRows.find((candidate) => candidate.id === review.anchorId) ?? row + : row + let target = selectableRows[currentIndex + direction] + if (extend) { + let targetIndex = currentIndex + direction + while (target && (target.side !== anchorRow.side || target.hunkId !== anchorRow.hunkId)) { + targetIndex += direction + target = selectableRows[targetIndex] + } + } + if (!target) return + setRovingId(target.id) + focusButton(target.id) + if (extend && !review.anchorId) { + const selection = getCompatibleDiffRange(rows, row.id, target.id) + if (selection) { + setReview((current) => ({ + ...current, + anchorId: row.id, + focusId: target.id, + selection, + })) + setStatus(null) + } else { + selectSingleRow(target, 'selectionReset') + } + } else if (extend) { + extendSelection(target) + } + } + + const handleRowKeyDown = (event: KeyboardEvent, row: WorkspaceDiffRow) => { + if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { + event.preventDefault() + moveRovingFocus(row, event.key === 'ArrowDown' ? 1 : -1, event.shiftKey) + return + } + if (event.key === 'Home' || event.key === 'End') { + event.preventDefault() + const navigationRows = event.shiftKey + ? selectableRows.filter((candidate) => ( + candidate.side === row.side && candidate.hunkId === row.hunkId + )) + : selectableRows + const target = event.key === 'Home' ? navigationRows[0] : navigationRows.at(-1) + if (target) { + setRovingId(target.id) + focusButton(target.id) + if (event.shiftKey && !review.anchorId) { + const selection = getCompatibleDiffRange(rows, row.id, target.id) + if (selection) { + setReview((current) => ({ + ...current, + anchorId: row.id, + focusId: target.id, + selection, + })) + setStatus(null) + } + } else if (event.shiftKey) { + extendSelection(target) + } + } + return + } + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault() + activateRow(row, event.shiftKey, true) + } + } + + const closeEditor = () => { + const restoreId = review.anchorId + setReview((current) => ({ ...current, anchorId: null, focusId: null, selection: null })) + setStatus(null) + focusButton(restoreId) + } + + const submitComment = () => { + const note = review.draft.trim() + if (!note || !review.selection) return + const { side, lineStart, lineEnd, quote, hunkId } = review.selection + onAddComment?.({ side, lineStart, lineEnd, quote, hunkId }, note) + const restoreId = review.anchorId + setReview(emptyReviewState) + setStatus(null) + focusButton(restoreId) + } + + const handleEditorKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + event.preventDefault() + closeEditor() + return + } + if (event.key === 'Enter' && (event.metaKey || event.ctrlKey)) { + event.preventDefault() + submitComment() + } + } + + const toggleRows = () => { + if (!showAllRows) { + setShowAllRows(true) + return + } + + const collapsedItemIds = new Set(displayItemIds.slice(0, lineLimit)) + const collapsedSelectableRows = rows.filter((row) => row.selectable && collapsedItemIds.has(row.id)) + const nextRovingId = collapsedSelectableRows[0]?.id ?? null + const selectionWillBeHidden = review.selection?.rowIds.some((id) => !collapsedItemIds.has(id)) ?? false + + if (selectionWillBeHidden) { + setReview((current) => ({ + ...current, + anchorId: null, + focusId: null, + selection: null, + })) + setStatus('collapsedSelection') + } + setRovingId(nextRovingId) + pendingRovingFocus.current = nextRovingId + setShowAllRows(false) + } + + const renderEditor = () => review.selection && ( +
+ {status && ( +
+ {t(`workspace.diffReview.${status}`)} +
+ )} +
+