From d507adc55535d9be159bed64e22ebae048ce47ee Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:47:46 -0700 Subject: [PATCH] Fix combined diff saves before React commits native edits --- .../combined-diff/CombinedDiffViewer.tsx | 8 +-- .../combined-diff-section-load-registry.ts | 4 +- .../use-combined-diff-view-restore.test.tsx | 3 +- .../use-combined-diff-section-save.test.tsx | 20 +++++-- .../use-combined-diff-sections-state.ts | 14 +++++ .../use-pierre-diff-native-view.ts | 5 +- tests/e2e/diff-context-copy.spec.ts | 38 ++++++------ tests/e2e/diff-edit-state-restoration.spec.ts | 8 +-- .../e2e/diff-native-state-restoration.spec.ts | 32 ++-------- tests/e2e/diff-text-selection.ts | 58 +++++++++++++++++++ 10 files changed, 129 insertions(+), 61 deletions(-) create mode 100644 src/renderer/src/components/editor/combined-diff/use-combined-diff-sections-state.ts create mode 100644 tests/e2e/diff-text-selection.ts diff --git a/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx b/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx index dc8f957e12b..c8fbdf44ba1 100644 --- a/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx +++ b/src/renderer/src/components/editor/combined-diff/CombinedDiffViewer.tsx @@ -1,11 +1,11 @@ import React, { useCallback, useRef, useState } from 'react' +import { useCombinedDiffSectionsState } from './use-combined-diff-sections-state' import { useAppStore } from '@/store' import { createProgrammaticScrollMarks } from '@/hooks/programmatic-scroll-marks' import { useWorkspaceFileBrowserActionPredicate } from '@/lib/file-preview' import { selectWorktreeDiffCommentsOrEmpty } from '@/store/worktree-diff-comments-selector' import type { OpenFile } from '@/store/slices/editor' import '@/lib/monaco-setup' -import type { DiffSection } from '../diff-section-types' import { EMPTY_GIT_BRANCH_ENTRIES, EMPTY_GIT_STATUS_ENTRIES, @@ -66,7 +66,7 @@ export default function CombinedDiffViewer({ const activeGroupId = useAppStore((s) => s.activeGroupIdByWorktree[file.worktreeId]) const canOpenWorkspaceFileBrowserForPath = useWorkspaceFileBrowserActionPredicate(file.worktreeId) - const [sections, setSections] = useState([]) + const { sections, sectionsRef, setSections } = useCombinedDiffSectionsState() const [sectionHeights, setSectionHeights] = useState>({}) const [generation, setGeneration] = useState(0) // Why: a browser scroll clamp must re-pin the restore without being recorded as user intent. @@ -74,7 +74,7 @@ export default function CombinedDiffViewer({ const [programmaticScrollMarks] = useState(createProgrammaticScrollMarks) const scrollContainerRef = useRef(null) - const registry = useCombinedDiffSectionLoadRegistry(sections) + const registry = useCombinedDiffSectionLoadRegistry(sectionsRef) const entrySet = useCombinedDiffEntrySet({ file, gitStatusEntries, @@ -170,7 +170,7 @@ export default function CombinedDiffViewer({ registry.loadSchedulerRef.current.request(index) } }, - [registry.loadSchedulerRef, registry.sectionsRef] + [registry.loadSchedulerRef, registry.sectionsRef, setSections] ) const treeNavigation = useCombinedDiffTreeNavigation({ diff --git a/src/renderer/src/components/editor/combined-diff/load-sections/combined-diff-section-load-registry.ts b/src/renderer/src/components/editor/combined-diff/load-sections/combined-diff-section-load-registry.ts index 370dbd6ebf0..3982b1d6a04 100644 --- a/src/renderer/src/components/editor/combined-diff/load-sections/combined-diff-section-load-registry.ts +++ b/src/renderer/src/components/editor/combined-diff/load-sections/combined-diff-section-load-registry.ts @@ -33,12 +33,11 @@ export type CombinedDiffSectionLoadRegistry = { } export function useCombinedDiffSectionLoadRegistry( - sections: DiffSection[] + sectionsRef: React.RefObject ): CombinedDiffSectionLoadRegistry { const loadedIndicesRef = useRef>(new Set()) const loadingIndicesRef = useRef>(new Set()) const deferredLoadRequestsRef = useRef>(new Set()) - const sectionsRef = useRef([]) const generationRef = useRef(0) // Why: per-section reload token, so a sibling's reload can't discard this section's in-flight load. const sectionLoadTokensRef = useRef>(new Map()) @@ -51,7 +50,6 @@ export function useCombinedDiffSectionLoadRegistry( loadSchedulerRef.current ??= createCombinedDiffLoadScheduler({ loadSection: (index) => loadSectionRef.current(index) }) - sectionsRef.current = sections useEffect(() => { // Why: React StrictMode replays effect cleanup in dev; reset revives the scheduler for the replayed mount. diff --git a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx index 010932438a6..7d11b0a3dd6 100644 --- a/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx +++ b/src/renderer/src/components/editor/combined-diff/remember-view/use-combined-diff-view-restore.test.tsx @@ -1,6 +1,7 @@ // @vitest-environment happy-dom import { afterEach, describe, expect, it } from 'vitest' +import { useRef } from 'react' import { cleanup, renderHook } from '@testing-library/react' import type { GitBranchChangeEntry } from '../../../../../../shared/git-diff-compare-types' import type { GitStatusEntry } from '../../../../../../shared/git-status-types' @@ -39,7 +40,7 @@ function buildAllModeEntrySet( function restoreSections(entrySet: CombinedDiffEntrySet, viewStateKey: string): DiffSection[] { let sections: DiffSection[] = [] renderHook(() => { - const registry = useCombinedDiffSectionLoadRegistry([]) + const registry = useCombinedDiffSectionLoadRegistry(useRef([])) return useCombinedDiffViewRestore({ entrySet, gitStatusEntries: [], diff --git a/src/renderer/src/components/editor/combined-diff/review-controls/use-combined-diff-section-save.test.tsx b/src/renderer/src/components/editor/combined-diff/review-controls/use-combined-diff-section-save.test.tsx index 1db8bb99ec3..8a05ee573ac 100644 --- a/src/renderer/src/components/editor/combined-diff/review-controls/use-combined-diff-section-save.test.tsx +++ b/src/renderer/src/components/editor/combined-diff/review-controls/use-combined-diff-section-save.test.tsx @@ -1,7 +1,8 @@ // @vitest-environment happy-dom import { act, renderHook } from '@testing-library/react' import { beforeEach, describe, expect, it, vi } from 'vitest' -import { useRef, useState } from 'react' +import { useState } from 'react' +import { useCombinedDiffSectionsState } from '../use-combined-diff-sections-state' import type { OpenFile } from '@/store/slices/editor' import type { DiffSection } from '../../diff-section-types' import { useCombinedDiffSectionSave } from './use-combined-diff-section-save' @@ -45,10 +46,8 @@ function section(key = 'file.ts'): DiffSection { } function setup(initial = [section()]) { return renderHook(() => { - const [sections, setSections] = useState(initial) + const { sections, setSections, sectionsRef } = useCombinedDiffSectionsState(initial) const [heights, setSectionHeights] = useState>({ 0: 100, 1: 200 }) - const sectionsRef = useRef(sections) - sectionsRef.current = sections const save = useCombinedDiffSectionSave({ file, sectionsRef, setSections, setSectionHeights }) return { sections, setSections, heights, save } }) @@ -68,6 +67,19 @@ beforeEach(() => { }) describe('combined diff section saves', () => { + it('saves native editor edits before React commits a render', async () => { + writeFile.mockResolvedValue(undefined) + const view = setup([{ ...section(), dirty: false, modifiedContent: 'disk' }]) + await act(async () => { + view.result.current.setSections((prev) => [ + { ...prev[0], modifiedContent: 'typed immediately before save', dirty: true } + ]) + await view.result.current.save.current(0) + }) + expect(writeFile.mock.calls[0]?.[2]).toBe('typed immediately before save') + expect(view.result.current.sections[0].dirty).toBe(false) + }) + it('keeps edits made during a delayed remote write and advances only the saved baseline', async () => { const pending = deferred() writeFile.mockReturnValueOnce(pending.promise) diff --git a/src/renderer/src/components/editor/combined-diff/use-combined-diff-sections-state.ts b/src/renderer/src/components/editor/combined-diff/use-combined-diff-sections-state.ts new file mode 100644 index 00000000000..edbac052b34 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff/use-combined-diff-sections-state.ts @@ -0,0 +1,14 @@ +import { useCallback, useRef, useState, type SetStateAction } from 'react' +import type { DiffSection } from '../diff-section-types' + +export function useCombinedDiffSectionsState(initial: DiffSection[] = []) { + const [sections, renderSections] = useState(initial) + const sectionsRef = useRef(sections) + const setSections = useCallback((update: SetStateAction) => { + const next = typeof update === 'function' ? update(sectionsRef.current) : update + // Native editor events can reach Save before React commits their queued render. + sectionsRef.current = next + renderSections(next) + }, []) + return { sections, sectionsRef, setSections } +} diff --git a/src/renderer/src/components/editor/pierre-diff/use-pierre-diff-native-view.ts b/src/renderer/src/components/editor/pierre-diff/use-pierre-diff-native-view.ts index 2040445aea2..ba9a76759b7 100644 --- a/src/renderer/src/components/editor/pierre-diff/use-pierre-diff-native-view.ts +++ b/src/renderer/src/components/editor/pierre-diff/use-pierre-diff-native-view.ts @@ -1,4 +1,4 @@ -import { useCallback, useLayoutEffect, useRef } from 'react' +import { useCallback, useLayoutEffect, useRef, useState } from 'react' import type { FileDiffMetadata, PostRenderPhase } from '@pierre/diffs' import type { Editor } from '@pierre/diffs/edit' import type { PierreDiffInstance } from './PierreDiffSurface' @@ -24,7 +24,8 @@ export function usePierreDiffNativeView( useLayoutEffect(() => { latest.current = { fileDiff, editable, activeGroupId } }, [fileDiff, editable, activeGroupId]) - const pending = useRef(key ? getPierreNativeView(key) : undefined) + const [restoreSeed] = useState(() => (key ? getPierreNativeView(key) : undefined)) + const pending = useRef(restoreSeed) const frame = useRef(null) const attempts = useRef(0) const lastSnapshot = useRef(undefined) diff --git a/tests/e2e/diff-context-copy.spec.ts b/tests/e2e/diff-context-copy.spec.ts index 52e2c3d7be5..ed5bcf5c3a3 100644 --- a/tests/e2e/diff-context-copy.spec.ts +++ b/tests/e2e/diff-context-copy.spec.ts @@ -1,4 +1,5 @@ import { rmSync, writeFileSync } from 'node:fs' +import { diffTextSelectionPoints } from './diff-text-selection' import { test, expect } from './helpers/orca-app' import { waitForSessionReady } from './helpers/store' import { addAndActivateRepo } from './helpers/isolated-repo-activation' @@ -31,24 +32,27 @@ test('copies backwards selections with file and line context from each diff side let copied = '' try { for (const side of ['deletions', 'additions']) { - const coordinates = await orcaPage.locator('diffs-container').evaluate((host, side) => { - const rows = [ - ...host.shadowRoot!.querySelectorAll(`[data-${side}] [data-content] [data-line]`) - ] - const rect = (row: Element) => { - const range = document.createRange() - range.selectNodeContents(row) - const bounds = range.getBoundingClientRect() - return { left: bounds.left, right: bounds.right, y: bounds.top + bounds.height / 2 } - } - return { start: rect(rows[0]), end: rect(rows.at(-1)) } - }, side) - await orcaPage.mouse.move(coordinates.end.right, coordinates.end.y) - await orcaPage.mouse.down() - await orcaPage.mouse.move(coordinates.start.left, coordinates.start.y, { steps: 8 }) - await orcaPage.mouse.up() - await orcaPage.keyboard.press('ControlOrMeta+Alt+c') const contents = side === 'deletions' ? original : modified + const coordinates = await diffTextSelectionPoints( + orcaPage.locator(`diffs-container [data-code][data-${side}]`), + contents.trimEnd() + ) + await orcaPage.mouse.move(coordinates.end.x, coordinates.end.y) + await orcaPage.mouse.down() + await orcaPage.mouse.move(coordinates.start.x, coordinates.start.y, { steps: 8 }) + await orcaPage.mouse.up() + await expect + .poll(() => + orcaPage + .locator('diffs-container') + .evaluate((host) => + (host.shadowRoot as ShadowRoot & { getSelection(): Selection }) + .getSelection() + .toString() + ) + ) + .toBe(contents.trimEnd()) + await orcaPage.keyboard.press('ControlOrMeta+Alt+c') copied = `File: ${fixture.relativePath}\nLines: 1-3\n\n\`\`\`ts\n${contents}\`\`\`` await expect .poll(() => electronApp.evaluate(({ clipboard }) => clipboard.readText())) diff --git a/tests/e2e/diff-edit-state-restoration.spec.ts b/tests/e2e/diff-edit-state-restoration.spec.ts index 17eadb99654..942fdb5a8eb 100644 --- a/tests/e2e/diff-edit-state-restoration.spec.ts +++ b/tests/e2e/diff-edit-state-restoration.spec.ts @@ -75,9 +75,9 @@ for (const surface of ['file', 'combined']) { await entry.click() } else { const header = orcaPage.locator('[data-combined-diff-section-row] .sticky').first() - await header.click() + await header.click({ position: { x: 4, y: 8 } }) await expect(orcaPage.locator('diffs-container')).toHaveCount(0) - await header.click() + await header.click({ position: { x: 4, y: 8 } }) } await expect(line).toHaveText(`${modified.trimEnd()}X`, { timeout: 20_000 }) await expect @@ -103,10 +103,10 @@ for (const surface of ['file', 'combined']) { await entry.click() } else { const header = orcaPage.locator('[data-combined-diff-section-row] .sticky').first() - await header.click() + await header.click({ position: { x: 4, y: 8 } }) await expect(orcaPage.locator('diffs-container')).toHaveCount(0) writeFileSync(fixture.absolutePath, 'external replacement\n') - await header.click() + await header.click({ position: { x: 4, y: 8 } }) } await expect(host.locator('[data-content]').last()).toContainText('external replacement', { timeout: 20_000 diff --git a/tests/e2e/diff-native-state-restoration.spec.ts b/tests/e2e/diff-native-state-restoration.spec.ts index ffec833ce74..58b62bcd7f4 100644 --- a/tests/e2e/diff-native-state-restoration.spec.ts +++ b/tests/e2e/diff-native-state-restoration.spec.ts @@ -1,5 +1,6 @@ import { rmSync, writeFileSync } from 'node:fs' import path from 'node:path' +import { diffTextSelectionPoints } from './diff-text-selection' import { test, expect } from './helpers/orca-app' import { waitForSessionReady } from './helpers/store' import { addAndActivateRepo } from './helpers/isolated-repo-activation' @@ -49,31 +50,10 @@ for (const mode of ['original-file', 'readonly-combined']) { const side = mode === 'original-file' ? 'deletions' : 'additions' const code = host.locator(`[data-code][data-${side}]`) await expect(code).toBeVisible({ timeout: 20_000 }) - await code.evaluate((node) => { - node.scrollLeft = 1000 - }) - const word = await code - .locator('[data-line]') - .first() - .evaluate((row) => { - const walker = document.createTreeWalker(row, NodeFilter.SHOW_TEXT) - let node: Node | null - while ((node = walker.nextNode())) { - const start = node.textContent?.indexOf('SELECT_ME') ?? -1 - if (start < 0) { - continue - } - const range = document.createRange() - range.setStart(node, start) - range.setEnd(node, start + 9) - const rect = range.getBoundingClientRect() - return { x: rect.x, right: rect.right, y: rect.y + rect.height / 2 } - } - throw new Error('Missing target text') - }) - await orcaPage.mouse.move(word.right, word.y) + const points = await diffTextSelectionPoints(code, 'SELECT_ME') + await orcaPage.mouse.move(points.end.x, points.end.y) await orcaPage.mouse.down() - await orcaPage.mouse.move(word.x, word.y, { steps: 6 }) + await orcaPage.mouse.move(points.start.x, points.start.y, { steps: 6 }) await orcaPage.mouse.up() const selectedText = () => host.evaluate((host) => @@ -94,9 +74,9 @@ for (const mode of ['original-file', 'readonly-combined']) { .filter({ hasText: path.basename(fixture.relativePath) }) .locator('.sticky') .first() - await header.click() + await header.click({ position: { x: 4, y: 8 } }) await expect(host).toHaveCount(0) - await header.click() + await header.click({ position: { x: 4, y: 8 } }) } await expect.poll(() => code.evaluate((node) => node.scrollLeft)).toBeCloseTo(scrollLeft, 0) await expect.poll(selectedText).toBe('SELECT_ME') diff --git a/tests/e2e/diff-text-selection.ts b/tests/e2e/diff-text-selection.ts new file mode 100644 index 00000000000..a0d2e5dd3a6 --- /dev/null +++ b/tests/e2e/diff-text-selection.ts @@ -0,0 +1,58 @@ +import type { Locator } from '@playwright/test' + +export async function diffTextSelectionPoints(code: Locator, text: string) { + return code.evaluate(async (code, text) => { + const content = code.querySelector('[data-content]')! + const nodes: Text[] = [] + for (const row of content.querySelectorAll('[data-line]')) { + if (nodes.length > 0 && !nodes.at(-1)!.data.endsWith('\n')) { + // Pierre renders line breaks as separate rows rather than text nodes. + nodes.push(document.createTextNode('\n')) + } + const walker = document.createTreeWalker(row, NodeFilter.SHOW_TEXT) + while (walker.nextNode()) { + nodes.push(walker.currentNode as Text) + } + } + const contents = nodes.map((node) => node.data).join('') + const start = contents.indexOf(text) + if (start === -1) { + throw new Error('Selection text is absent from the rendered diff') + } + const position = (offset: number) => { + for (const node of nodes) { + if (offset < node.length) { + return { node, offset } + } + offset -= node.length + } + throw new Error('Selection endpoint is outside the rendered diff') + } + const glyph = (offset: number) => { + const point = position(offset) + const range = document.createRange() + range.setStart(point.node, point.offset) + range.setEnd(point.node, point.offset + 1) + return range + } + const first = glyph(start) + const last = glyph(start + text.length - 1) + const viewport = code.getBoundingClientRect() + const left = Math.min(first.getBoundingClientRect().left, last.getBoundingClientRect().left) + const right = Math.max(first.getBoundingClientRect().right, last.getBoundingClientRect().right) + code.scrollLeft += left - viewport.left - Math.max(24, (viewport.width - (right - left)) / 2) + await new Promise((resolve) => requestAnimationFrame(() => resolve())) + const point = (range: Range, end: boolean) => { + const rect = range.getBoundingClientRect() + const x = end ? rect.right - 0.5 : rect.left + 0.5 + const y = rect.top + rect.height / 2 + const root = code.getRootNode() as ShadowRoot + const hit = root.elementFromPoint(x, y) + if (!hit || !content.contains(hit) || document.elementFromPoint(x, y) !== root.host) { + throw new Error('Selection endpoint is clipped or covered by another diff pane') + } + return { x, y } + } + return { start: point(first, false), end: point(last, true) } + }, text) +}