From a142c84ede203dc47d8cbf519853b451be9bb6b5 Mon Sep 17 00:00:00 2001 From: gatsby74 <166927047+gatsby74@users.noreply.github.com> Date: Mon, 27 Jul 2026 08:48:20 +0200 Subject: [PATCH] Fix diff notes overlapping following lines (#7803) Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> --- .../DiffCommentCard.resize.test.tsx | 104 ++++++++++++++ .../diff-comments/DiffCommentCard.tsx | 54 ++++++-- .../diff-comments/useDiffCommentDecorator.tsx | 10 +- tests/e2e/diff-note-layout.spec.ts | 128 ++++++++++++++++++ tests/e2e/global-setup.ts | 1 + tests/e2e/helpers/seeded-test-repo.ts | 1 + 6 files changed, 285 insertions(+), 13 deletions(-) create mode 100644 src/renderer/src/components/diff-comments/DiffCommentCard.resize.test.tsx create mode 100644 tests/e2e/diff-note-layout.spec.ts diff --git a/src/renderer/src/components/diff-comments/DiffCommentCard.resize.test.tsx b/src/renderer/src/components/diff-comments/DiffCommentCard.resize.test.tsx new file mode 100644 index 000000000..f17dfbb76 --- /dev/null +++ b/src/renderer/src/components/diff-comments/DiffCommentCard.resize.test.tsx @@ -0,0 +1,104 @@ +// @vitest-environment happy-dom +import { act, cleanup, render } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { DiffCommentCard } from './DiffCommentCard' + +describe('DiffCommentCard content resize', () => { + let notifyObservedResize: ResizeObserverCallback + let constructObserver = vi.fn<() => void>() + let disconnect: ReturnType + let frameCallbacks: Map + let nextFrameId: number + + beforeEach(() => { + constructObserver = vi.fn<() => void>() + disconnect = vi.fn() + frameCallbacks = new Map() + nextFrameId = 1 + + class TestResizeObserver { + constructor(callback: ResizeObserverCallback) { + constructObserver() + notifyObservedResize = callback + } + + observe = vi.fn() + unobserve = vi.fn() + disconnect = disconnect + } + + vi.stubGlobal('ResizeObserver', TestResizeObserver) + vi.stubGlobal( + 'requestAnimationFrame', + vi.fn((callback: FrameRequestCallback) => { + const frameId = nextFrameId++ + frameCallbacks.set(frameId, callback) + return frameId + }) + ) + vi.stubGlobal( + 'cancelAnimationFrame', + vi.fn((frameId: number) => frameCallbacks.delete(frameId)) + ) + }) + + afterEach(() => { + cleanup() + vi.unstubAllGlobals() + }) + + it('re-measures after wrapping and coalesces observer notifications', () => { + const onContentResize = vi.fn() + const view = render( + + ) + + expect(constructObserver).toHaveBeenCalledOnce() + expect(onContentResize).toHaveBeenCalledOnce() + + act(() => { + notifyObservedResize([], {} as ResizeObserver) + notifyObservedResize([], {} as ResizeObserver) + }) + expect(requestAnimationFrame).toHaveBeenCalledOnce() + + const callback = frameCallbacks.values().next().value + frameCallbacks.clear() + act(() => callback?.(0)) + expect(onContentResize).toHaveBeenCalledTimes(2) + + const latestOnContentResize = vi.fn() + view.rerender( + + ) + expect(disconnect).not.toHaveBeenCalled() + + act(() => notifyObservedResize([], {} as ResizeObserver)) + const latestCallback = frameCallbacks.values().next().value + act(() => latestCallback?.(0)) + expect(latestOnContentResize).toHaveBeenCalledOnce() + + view.unmount() + expect(disconnect).toHaveBeenCalledOnce() + }) + + it('does not observe cards outside Monaco view zones', () => { + const onContentResize = vi.fn() + render( + + ) + + expect(constructObserver).not.toHaveBeenCalled() + expect(onContentResize).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/diff-comments/DiffCommentCard.tsx b/src/renderer/src/components/diff-comments/DiffCommentCard.tsx index a854a43ae..9e7ddf229 100644 --- a/src/renderer/src/components/diff-comments/DiffCommentCard.tsx +++ b/src/renderer/src/components/diff-comments/DiffCommentCard.tsx @@ -27,10 +27,10 @@ type Props = { url?: string onDelete?: () => void // Why: Monaco view zones have a fixed `heightInPx` set at insertion time - // and aren't auto-measured. While the user is in edit mode the textarea - // grows, so the parent decorator passes a callback we fire on resize and - // it re-syncs the zone height. Without this the editor inputs would clip. + // and aren't auto-measured. The parent decorator re-syncs that height when + // the rendered card wraps or grows so it cannot overlap following lines. onContentResize?: () => void + observeRenderedSize?: boolean onSubmitEdit?: (body: string) => Promise headerActions?: ReactNode } @@ -47,6 +47,7 @@ export function DiffCommentCard({ url, onDelete, onContentResize, + observeRenderedSize, onSubmitEdit, headerActions }: Props): React.JSX.Element { @@ -54,17 +55,52 @@ export function DiffCommentCard({ const [draft, setDraft] = useState(body) const [submitting, setSubmitting] = useState(false) const mountedRef = useMountedRef() + const cardRef = useRef(null) const textareaRef = useRef(null) const resizeAfterCloseRef = useRef(false) + const observesRenderedSize = observeRenderedSize === true && onContentResize !== undefined - // Why: stash `onContentResize` in a ref so the layout/resize effects only - // re-run on `editing` transitions. The decorator passes a fresh arrow every - // render; depending on it directly would re-fire the layout effect on every - // unrelated parent render and yank the caret to the textarea's end while - // the user is mid-edit. + // Why: stash `onContentResize` in a ref so resize effects do not depend on + // the decorator's fresh arrow each render. Re-running the edit layout effect + // would yank the caret to the textarea's end while the user is mid-edit. const onContentResizeRef = useRef(onContentResize) onContentResizeRef.current = onContentResize + useLayoutEffect(() => { + const card = cardRef.current + if (!card || !observesRenderedSize) { + return + } + onContentResizeRef.current?.() + let frameId: number | null = null + const notifyResize = (): void => { + if (frameId !== null) { + return + } + frameId = requestAnimationFrame(() => { + frameId = null + onContentResizeRef.current?.() + }) + } + if (typeof ResizeObserver === 'undefined') { + return () => { + if (frameId !== null) { + cancelAnimationFrame(frameId) + } + } + } + // Why: narrow diff panes can wrap body/header text after Monaco's initial + // estimate; observe the real card height in either diff layout. + const observer = new ResizeObserver(() => notifyResize()) + observer.observe(card) + return () => { + observer.disconnect() + if (frameId !== null) { + cancelAnimationFrame(frameId) + } + } + }, [observesRenderedSize]) + // Why: focus + auto-grow the textarea on entering edit mode. Layout effect // so the height is set before the browser paints — a measurement pass on // the next animation frame would visibly jump from 0 to N px. @@ -138,7 +174,7 @@ export function DiffCommentCard({ } return ( -
+
{/* Header Row */}
diff --git a/src/renderer/src/components/diff-comments/useDiffCommentDecorator.tsx b/src/renderer/src/components/diff-comments/useDiffCommentDecorator.tsx index addfcb4e3..2c3214070 100644 --- a/src/renderer/src/components/diff-comments/useDiffCommentDecorator.tsx +++ b/src/renderer/src/components/diff-comments/useDiffCommentDecorator.tsx @@ -381,12 +381,13 @@ export function useDiffCommentDecorator({ const verticalPadding = Number.parseFloat(wrapperStyle.paddingTop) + Number.parseFloat(wrapperStyle.paddingBottom) // Monaco pins the zone node to its previous height (scrollHeight can't shrink), so measure the rendered card+padding to allow collapse. - const measured = Math.ceil( - (child?.getBoundingClientRect().height ?? entry.domNode.scrollHeight) + verticalPadding - ) - if (measured <= 0) { + const childHeight = child?.getBoundingClientRect().height ?? 0 + // React can commit while Monaco's zone is detached; preserve the safe + // initial estimate until the observer sees a measurable card. + if (childHeight <= 0) { return } + const measured = Math.ceil(childHeight + verticalPadding) if (entry.delegate.heightInPx === measured) { return } @@ -447,6 +448,7 @@ export function useDiffCommentDecorator({ : undefined } onContentResize={() => resizeZone(comment.id)} + observeRenderedSize headerActions={ worktreeId && comment.author === undefined ? ( { + const card = page.locator('.orca-diff-comment-card').first() + const followingLine = page + .locator('.modified-in-monaco-diff-editor .view-lines .view-line') + .filter({ hasText: FOLLOWING_LINE }) + .first() + + await expect(card).toBeVisible({ timeout: 15_000 }) + await expect(followingLine).toBeVisible({ timeout: 15_000 }) + await expect + .poll(async () => (await card.boundingBox())?.height ?? 0) + .toBeGreaterThan(INITIAL_ZONE_HEIGHT) + await expect + .poll( + async () => { + const [cardBox, lineBox] = await Promise.all([ + card.boundingBox(), + followingLine.boundingBox() + ]) + return cardBox && lineBox ? lineBox.y - (cardBox.y + cardBox.height) : -1 + }, + { message: 'saved note overlaps the following diff line' } + ) + .toBeGreaterThanOrEqual(0) +} + +async function attachDiffScreenshot(page: Page, testInfo: TestInfo, name: string): Promise { + const diff = page.locator('.monaco-diff-editor').first() + const screenshotPath = testInfo.outputPath(`${name}.png`) + await diff.screenshot({ path: screenshotPath }) + await testInfo.attach(name, { path: screenshotPath, contentType: 'image/png' }) +} + +test.describe('Diff note layout', () => { + test.beforeEach(async ({ orcaPage }) => { + await waitForSessionReady(orcaPage) + await waitForActiveWorktree(orcaPage) + }) + + test('saved notes reserve their rendered height in both diff layouts', async ({ + orcaPage + }, testInfo) => { + await orcaPage.setViewportSize({ width: 1200, height: 800 }) + const worktreeId = await waitForActiveWorktree(orcaPage) + const relativePath = await orcaPage.evaluate(async (wId) => { + const store = window.__store + if (!store) { + throw new Error('window.__store is not available') + } + const state = store.getState() + const worktree = Object.values(state.worktreesByRepo) + .flat() + .find((entry) => entry.id === wId) + if (!worktree) { + throw new Error('active worktree not found') + } + const separator = worktree.path.includes('\\') ? '\\' : '/' + const relative = `src${separator}diff-note-layout.ts` + const lines = Array.from({ length: 14 }, (_, index) => { + const number = String(index + 1).padStart(2, '0') + const value = index + 1 === 7 ? 'following-line-marker' : `value-${number}` + return `export const line${number} = "${value}"` + }) + await window.api.fs.writeFile({ + filePath: `${worktree.path}${separator}${relative}`, + content: `${lines.join('\n')}\n` + }) + await state.updateSettings({ diffDefaultView: 'side-by-side' }) + return relative + }, worktreeId) + + const added = await orcaPage.evaluate( + ({ wId, filePath, lineNumber, body }) => + window.__store?.getState().addDiffComment({ + worktreeId: wId, + filePath, + source: 'diff', + lineNumber, + body, + side: 'modified' + }), + { wId: worktreeId, filePath: relativePath, lineNumber: NOTE_LINE, body: NOTE_BODY } + ) + expect(added, 'addDiffComment returned null').not.toBeNull() + + await orcaPage.evaluate( + ({ wId, filePath }) => { + const state = window.__store?.getState() + const worktree = Object.values(state?.worktreesByRepo ?? {}) + .flat() + .find((entry) => entry.id === wId) + if (!state || !worktree) { + throw new Error('active worktree not found') + } + const separator = worktree.path.includes('\\') ? '\\' : '/' + state.openDiff( + wId, + `${worktree.path}${separator}${filePath}`, + filePath, + 'typescript', + false + ) + }, + { wId: worktreeId, filePath: relativePath } + ) + + await expect(orcaPage.locator('button:has(svg.lucide-rows-2)')).toBeVisible() + await assertCardClearsFollowingLine(orcaPage) + await attachDiffScreenshot(orcaPage, testInfo, 'side-by-side-diff-note-layout') + + await orcaPage.evaluate(() => + window.__store?.getState().updateSettings({ diffDefaultView: 'inline' }) + ) + await expect(orcaPage.locator('button:has(svg.lucide-columns-2)')).toBeVisible() + await assertCardClearsFollowingLine(orcaPage) + await attachDiffScreenshot(orcaPage, testInfo, 'inline-diff-note-layout') + }) +}) diff --git a/tests/e2e/global-setup.ts b/tests/e2e/global-setup.ts index 27d66ee23..b67eb68d7 100644 --- a/tests/e2e/global-setup.ts +++ b/tests/e2e/global-setup.ts @@ -104,6 +104,7 @@ export default function globalSetup(): void { writeFileSync(path.join(testRepoDir, '.gitignore'), 'node_modules/\n') mkdirSync(path.join(testRepoDir, 'src'), { recursive: true }) writeFileSync(path.join(testRepoDir, 'src', 'index.ts'), 'export const hello = "world"\n') + writeFileSync(path.join(testRepoDir, 'src', 'diff-note-layout.ts'), 'export const seed = true\n') execSync('git add -A', { cwd: testRepoDir, stdio: 'pipe' }) execSync('git commit -m "Initial commit for E2E tests"', { cwd: testRepoDir, stdio: 'pipe' }) diff --git a/tests/e2e/helpers/seeded-test-repo.ts b/tests/e2e/helpers/seeded-test-repo.ts index 3ae984193..dd88351b2 100644 --- a/tests/e2e/helpers/seeded-test-repo.ts +++ b/tests/e2e/helpers/seeded-test-repo.ts @@ -50,6 +50,7 @@ export function createSeededTestRepo(): string { writeFileSync(path.join(testRepoDir, '.gitignore'), 'node_modules/\n') mkdirSync(path.join(testRepoDir, 'src'), { recursive: true }) writeFileSync(path.join(testRepoDir, 'src', 'index.ts'), 'export const hello = "world"\n') + writeFileSync(path.join(testRepoDir, 'src', 'diff-note-layout.ts'), 'export const seed = true\n') execSync('git add -A', { cwd: testRepoDir, stdio: 'pipe' }) execSync('git commit -m "Initial commit for E2E tests"', { cwd: testRepoDir, stdio: 'pipe' })