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>

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<typeof vi.fn>
+  let frameCallbacks: Map<number, FrameRequestCallback>
+  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(
+      <DiffCommentCard
+        lineNumber={26}
+        body="A saved note"
+        onContentResize={onContentResize}
+        observeRenderedSize
+      />
+    )
+
+    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(
+      <DiffCommentCard
+        lineNumber={26}
+        body="A saved note"
+        onContentResize={latestOnContentResize}
+        observeRenderedSize
+      />
+    )
+    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(
+      <DiffCommentCard lineNumber={26} body="A saved note" onContentResize={onContentResize} />
+    )
+
+    expect(constructObserver).not.toHaveBeenCalled()
+    expect(onContentResize).not.toHaveBeenCalled()
+  })
+})

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<boolean>
   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<HTMLDivElement | null>(null)
   const textareaRef = useRef<HTMLTextAreaElement | null>(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 (
-    <div className="orca-diff-comment-card">
+    <div ref={cardRef} className="orca-diff-comment-card">
       <div className="orca-diff-comment-content-col">
         {/* Header Row */}
         <div className="orca-diff-comment-header">

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 ? (
                 <NotesSendMenu

tests/e2e/diff-note-layout.spec.ts

@@ -0,0 +1,128 @@
+import type { Page, TestInfo } from '@stablyai/playwright-test'
+import { expect, test } from './helpers/orca-app'
+import { waitForActiveWorktree, waitForSessionReady } from './helpers/store'
+
+const NOTE_LINE = 6
+const INITIAL_ZONE_HEIGHT = 88
+const FOLLOWING_LINE = 'export const line07 = "following-line-marker"'
+const NOTE_BODY =
+  'This saved note is intentionally one long paragraph so it wraps across several visual lines in narrow and wide diff layouts without adding newline characters to the initial zone estimate.'
+
+async function assertCardClearsFollowingLine(page: Page): Promise<void> {
+  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<void> {
+  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')
+  })
+})

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' })

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' })