fix: intermittent scroll bug (#2052)

.changeset/short-source-no-blank-space.md

@@ -0,0 +1,5 @@
+---
+"@read-frog/extension": patch
+---
+
+fix(selection): remove blank gap under short source text and restore wheel scrolling

.changeset/streaming-follow-keeps-user-scroll.md

@@ -0,0 +1,5 @@
+---
+"@read-frog/extension": patch
+---
+
+fix(selection): stop streamed custom-action output from yanking the popover scroll back to the bottom while the user is reading earlier content

src/components/ui/base-ui/scroll-area.tsx

@@ -4,7 +4,16 @@ import { ScrollArea as ScrollAreaPrimitive } from "@base-ui/react/scroll-area"
 import * as React from "react"
 import { cn } from "@/utils/styles/utils"
 
-function ScrollArea({ className, children, ...props }: ScrollAreaPrimitive.Root.Props) {
+interface ScrollAreaProps extends ScrollAreaPrimitive.Root.Props {
+  /**
+   * Classes for the scrollable viewport. Put height constraints here (e.g.
+   * `max-h-*`) so the area can shrink to its content instead of the root
+   * reserving a fixed box.
+   */
+  viewportClassName?: string
+}
+
+function ScrollArea({ className, viewportClassName, children, ...props }: ScrollAreaProps) {
   return (
     <ScrollAreaPrimitive.Root
       data-slot="scroll-area"
@@ -13,7 +22,10 @@ function ScrollArea({ className, children, ...props }: ScrollAreaPrimitive.Root.
     >
       <ScrollAreaPrimitive.Viewport
         data-slot="scroll-area-viewport"
-        className="size-full rounded-[inherit] transition-[color,box-shadow] outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 focus-visible:outline-1"
+        className={cn(
+          "size-full rounded-[inherit] transition-[color,box-shadow] outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50 focus-visible:outline-1",
+          viewportClassName,
+        )}
       >
         {children}
       </ScrollAreaPrimitive.Viewport>

src/components/ui/selection-popover/__tests__/use-prevent-scroll-through.test.tsx

@@ -0,0 +1,87 @@
+// @vitest-environment jsdom
+import { render } from "@testing-library/react"
+import { beforeEach, describe, expect, it } from "vitest"
+import { usePreventScrollThrough } from "../use-prevent-scroll-through"
+
+interface Metrics {
+  scrollTop: number
+  scrollHeight: number
+  clientHeight: number
+}
+
+/** jsdom has no layout, so scroll metrics have to be declared explicitly. */
+function setMetrics(element: HTMLElement, { scrollTop, scrollHeight, clientHeight }: Metrics) {
+  Object.defineProperties(element, {
+    scrollTop: { configurable: true, value: scrollTop, writable: true },
+    scrollHeight: { configurable: true, value: scrollHeight },
+    clientHeight: { configurable: true, value: clientHeight },
+  })
+}
+
+function Harness({ element }: { element: HTMLElement | null }) {
+  usePreventScrollThrough({ isEnabled: true, element })
+  return null
+}
+
+let body: HTMLDivElement
+let nested: HTMLDivElement
+let leaf: HTMLParagraphElement
+
+beforeEach(() => {
+  body = document.createElement("div")
+  nested = document.createElement("div")
+  leaf = document.createElement("p")
+
+  nested.style.overflowY = "scroll"
+  nested.append(leaf)
+  body.append(nested)
+  document.body.append(body)
+})
+
+function wheel(target: HTMLElement, deltaY: number) {
+  const event = new WheelEvent("wheel", { deltaY, bubbles: true, cancelable: true })
+  target.dispatchEvent(event)
+  return event
+}
+
+describe("usePreventScrollThrough", () => {
+  it("lets a nested scroller consume the wheel when it still has room", () => {
+    // The popover body itself cannot scroll — the case that used to swallow
+    // every wheel event and leave the nested area draggable-scrollbar only.
+    setMetrics(body, { scrollTop: 0, scrollHeight: 200, clientHeight: 200 })
+    setMetrics(nested, { scrollTop: 0, scrollHeight: 180, clientHeight: 72 })
+
+    render(<Harness element={body} />)
+
+    expect(wheel(leaf, 120).defaultPrevented).toBe(false)
+  })
+
+  it("still blocks scroll-through once the nested scroller is exhausted", () => {
+    setMetrics(body, { scrollTop: 0, scrollHeight: 200, clientHeight: 200 })
+    setMetrics(nested, { scrollTop: 108, scrollHeight: 180, clientHeight: 72 })
+
+    render(<Harness element={body} />)
+
+    expect(wheel(leaf, 120).defaultPrevented).toBe(true)
+  })
+
+  it("ignores overflowing children that are not scrollers", () => {
+    setMetrics(body, { scrollTop: 0, scrollHeight: 200, clientHeight: 200 })
+    nested.style.overflowY = "hidden"
+    setMetrics(nested, { scrollTop: 0, scrollHeight: 180, clientHeight: 72 })
+
+    render(<Harness element={body} />)
+
+    expect(wheel(leaf, 120).defaultPrevented).toBe(true)
+  })
+
+  it("keeps blocking at the body's own boundaries", () => {
+    setMetrics(body, { scrollTop: 0, scrollHeight: 400, clientHeight: 200 })
+    setMetrics(nested, { scrollTop: 0, scrollHeight: 72, clientHeight: 72 })
+
+    render(<Harness element={body} />)
+
+    expect(wheel(leaf, -120).defaultPrevented).toBe(true)
+    expect(wheel(leaf, 120).defaultPrevented).toBe(false)
+  })
+})

src/components/ui/selection-popover/use-prevent-scroll-through.ts

@@ -5,12 +5,51 @@ interface UsePreventScrollThroughOptions {
   element: HTMLElement | null
 }
 
+const SCROLLABLE_OVERFLOW_Y = new Set(["auto", "scroll", "overlay"])
+
+function isElement(node: EventTarget): node is HTMLElement {
+  return "nodeType" in node && (node as Node).nodeType === Node.ELEMENT_NODE
+}
+
+/**
+ * Whether this element is a scroller with room left in the wheel's direction,
+ * i.e. the browser would scroll it if we let the event through.
+ */
+function canAbsorbWheel(node: HTMLElement, deltaY: number) {
+  const { scrollTop, scrollHeight, clientHeight } = node
+
+  // Cheapest discriminator first: most nodes have nothing to scroll, so this
+  // keeps the per-wheel-event walk from hitting `getComputedStyle` at all.
+  if (scrollHeight <= clientHeight) {
+    return false
+  }
+
+  if (!SCROLLABLE_OVERFLOW_Y.has(getComputedStyle(node).overflowY)) {
+    return false
+  }
+
+  return deltaY < 0 ? scrollTop > 0 : scrollTop + clientHeight < scrollHeight - 1
+}
+
 export function usePreventScrollThrough({ isEnabled, element }: UsePreventScrollThroughOptions) {
   const handleWheel = useEffectEvent((event: WheelEvent) => {
     if (!element) {
       return
     }
 
+    // The listener sits on the popover body, so it also sees wheels aimed at
+    // nested scrollers (e.g. the collapsible source text). Preventing those
+    // would leave them scrollable only by dragging the scrollbar.
+    for (const node of event.composedPath()) {
+      if (node === element) {
+        break
+      }
+
+      if (isElement(node) && canAbsorbWheel(node, event.deltaY)) {
+        return
+      }
+    }
+
     const { scrollTop, scrollHeight, clientHeight } = element
     const isAtTop = event.deltaY < 0 && scrollTop === 0
     const isAtBottom = event.deltaY > 0 && scrollTop + clientHeight >= scrollHeight - 1

src/entrypoints/selection.content/components/__tests__/selection-source-content.test.tsx

@@ -0,0 +1,47 @@
+// @vitest-environment jsdom
+import { fireEvent, render, screen } from "@testing-library/react"
+import { describe, expect, it, vi } from "vitest"
+import { SelectionSourceContent } from "../selection-source-content"
+
+vi.mock("../copy-button", () => ({
+  CopyButton: () => <button type="button">Copy</button>,
+}))
+
+vi.mock("../speak-button", () => ({
+  SpeakButton: () => <button type="button">Speak</button>,
+}))
+
+function getViewport(container: HTMLElement) {
+  const viewport = container.querySelector<HTMLElement>("[data-slot=scroll-area-viewport]")
+  expect(viewport).not.toBeNull()
+  return viewport as HTMLElement
+}
+
+describe("selectionSourceContent", () => {
+  it("caps the expanded source text with a max height so short text keeps no blank space", () => {
+    const { container } = render(<SelectionSourceContent text="use" />)
+
+    const paragraph = screen.getByText("use")
+    expect(paragraph).toHaveClass("line-clamp-3")
+
+    // The chevron toggle is the first button rendered inside the source row.
+    const toggle = container.querySelector("button")
+    expect(toggle).not.toBeNull()
+    fireEvent.click(toggle as HTMLButtonElement)
+
+    expect(paragraph).not.toHaveClass("line-clamp-3")
+
+    const viewport = getViewport(container)
+    // `max-h-*` lets the area shrink to a one-word selection; a fixed `h-*`
+    // would always reserve the full box and leave a blank gap under the text.
+    expect(viewport).toHaveClass("max-h-18")
+    expect(viewport.className).not.toMatch(/(?:^|\s)h-\d/)
+    expect(getViewport(container).parentElement?.className).not.toMatch(/(?:^|\s)h-\d/)
+  })
+
+  it("does not constrain the height while collapsed", () => {
+    const { container } = render(<SelectionSourceContent text="use" />)
+
+    expect(getViewport(container).className).not.toMatch(/max-h-/)
+  })
+})

src/entrypoints/selection.content/components/selection-source-content.tsx

@@ -27,7 +27,10 @@ export function SelectionSourceContent({
     <>
       <div className="space-y-2">
         <div className="flex items-start justify-between gap-2">
-          <ScrollArea className={cn("min-w-0 flex-1", actionsExpanded && "h-18 overflow-hidden")}>
+          <ScrollArea
+            className="min-w-0 flex-1"
+            viewportClassName={cn(actionsExpanded && "max-h-18")}
+          >
             <p
               className={cn(
                 "text-sm [overflow-wrap:anywhere] break-words whitespace-pre-wrap text-zinc-600 dark:text-zinc-400",

src/entrypoints/selection.content/selection-toolbar/custom-action-button/use-custom-action-execution.ts

@@ -75,7 +75,23 @@ interface CustomActionExecutionRequest {
   }
 }
 
+const FOLLOW_STREAM_BOTTOM_THRESHOLD = 8
+
 function scrollSelectionPopoverBodyToBottom(ref: RefObject<HTMLDivElement | null>) {
+  const node = ref.current
+  if (!node) {
+    return
+  }
+
+  // Measured before the chunk renders: a reader who scrolled up to reread
+  // earlier output must not be yanked back down, and measuring after the
+  // append would misread "was at the bottom" as "far from it" whenever a
+  // chunk adds more height than the threshold.
+  const distanceToBottom = node.scrollHeight - node.scrollTop - node.clientHeight
+  if (distanceToBottom > FOLLOW_STREAM_BOTTOM_THRESHOLD) {
+    return
+  }
+
   requestAnimationFrame(() => {
     if (ref.current) {
       ref.current.scrollTop = ref.current.scrollHeight