fix(session-ui): retry local Markdown images and stop leaking gutter comment buttons (#61972)

packages/session-ui/src/components/line-comment-annotations.test.ts

@@ -0,0 +1,81 @@
+import { GlobalRegistrator } from "@happy-dom/global-registrator"
+import { afterAll, beforeAll, expect, test } from "bun:test"
+import type { SelectedLineRange } from "@pierre/diffs"
+import { createRoot } from "solid-js"
+import type { HoverCommentLine } from "../pierre/comment-hover"
+import { createLineCommentGutterRenderer } from "./line-comment-annotations"
+
+beforeAll(() => GlobalRegistrator.register())
+
+afterAll(() => GlobalRegistrator.unregister())
+
+/** What a Pierre instance reports as hovered, and how often the button asked. */
+type Viewer = { hovered: HoverCommentLine | undefined; reads: number }
+
+function setup() {
+  const opened: SelectedLineRange[] = []
+  const viewer: Viewer = { hovered: undefined, reads: 0 }
+
+  const getHoveredLine = () => {
+    viewer.reads++
+
+    return viewer.hovered
+  }
+
+  return createRoot((dispose) => ({
+    opened,
+    viewer,
+    getHoveredLine,
+    dispose,
+    render: createLineCommentGutterRenderer({
+      label: "Comment",
+      getSelectedRange: () => null,
+      onOpenDraft: (range) => opened.push(range),
+    }),
+  }))
+}
+
+test("tracks the hovered line when the pointer moves before Pierre inserts the button", () => {
+  const gutter = setup()
+  const button = gutter.render(gutter.getHoveredLine)!
+  document.dispatchEvent(new Event("pointermove"))
+  document.dispatchEvent(new Event("scroll"))
+  document.body.replaceChildren(button)
+
+  gutter.viewer.hovered = { lineNumber: 5, side: "additions" }
+  document.dispatchEvent(new Event("pointermove"))
+  gutter.viewer.hovered = undefined
+  button.click()
+
+  expect(gutter.opened).toEqual([{ start: 5, end: 5, side: "additions" }])
+  gutter.dispose()
+})
+
+test("reuses one button across Pierre rerenders and detaches it on cleanup", () => {
+  const gutter = setup()
+  const button = gutter.render(gutter.getHoveredLine)!
+  document.body.replaceChildren(button)
+
+  expect(gutter.render(gutter.getHoveredLine)).toBe(button)
+
+  // A reset viewer is a new Pierre instance with its own `getHoveredLine`.
+  const next = { reads: 0 }
+
+  const replaced = gutter.render(() => {
+    next.reads++
+
+    return undefined
+  })!
+
+  expect(replaced).not.toBe(button)
+  const reads = gutter.viewer.reads
+  document.dispatchEvent(new Event("pointermove"))
+  expect(gutter.viewer.reads).toBe(reads)
+
+  document.body.replaceChildren(replaced)
+  document.dispatchEvent(new Event("pointermove"))
+  expect(next.reads).toBe(1)
+  gutter.dispose()
+  document.dispatchEvent(new Event("pointermove"))
+  expect(next.reads).toBe(1)
+})

packages/session-ui/src/components/line-comment-annotations.tsx

@@ -2,7 +2,7 @@ import { type DiffLineAnnotation, type SelectedLineRange } from "@pierre/diffs"
 import { createEffect, createMemo, createSignal, onCleanup, Show, untrack, type Accessor, type JSX } from "solid-js"
 import { render as renderSolid } from "solid-js/web"
 import { useI18n } from "@opencode/ui/context/i18n"
-import { createHoverCommentUtility } from "../pierre/comment-hover"
+import { createHoverComment, type HoverComment, type HoverCommentLine } from "../pierre/comment-hover"
 import { cloneSelectedLineRange, formatSelectedLineLabel, lineInSelectedRange } from "../pierre/selection-bridge"
 import { LineComment, LineCommentEditor, type LineCommentEditorProps } from "./line-comment"
 import {
@@ -37,11 +37,6 @@ type LineCommentAnnotationsWithSideProps<T> = LineCommentAnnotationsProps<T> & {
   getSide: (range: SelectedLineRange) => "additions" | "deletions"
 }
 
-type HoverCommentLine = {
-  lineNumber: number
-  side?: "additions" | "deletions"
-}
-
 /** A line-comment mode owned by the caller, such as one file's entry in a multi-file review. Omit it to keep the mode inside the controller. */
 export type LineCommentMachine = {
   mode: Accessor<LineCommentMode>
@@ -538,8 +533,17 @@ export function createLineCommentGutterRenderer(props: {
   getSelectedRange: Accessor<SelectedLineRange | null>
   onOpenDraft: (range: SelectedLineRange) => void
 }) {
-  return (getHoveredLine: () => HoverCommentLine | undefined) =>
-    createHoverCommentUtility({
+  let active: { comment: HoverComment; getHoveredLine: () => HoverCommentLine | undefined } | undefined
+
+  onCleanup(() => active?.comment.dispose())
+
+  // Pierre calls this on every render and rerender but inserts the result only when it holds no button yet, so hand
+  // back the same button for the same viewer instead of building one with fresh document listeners each time.
+  return (getHoveredLine: () => HoverCommentLine | undefined) => {
+    if (active?.getHoveredLine === getHoveredLine && active.comment.canReuse()) return active.comment.button
+    active?.comment.dispose()
+
+    const comment = createHoverComment({
       label: props.label,
       getHoveredLine,
       onSelect: (hovered) => {
@@ -560,4 +564,9 @@ export function createLineCommentGutterRenderer(props: {
         props.onOpenDraft(range)
       },
     })
+
+    active = comment && { comment, getHoveredLine }
+
+    return comment?.button
+  }
 }

packages/session-ui/src/components/markdown-image-machine.test.ts

@@ -0,0 +1,136 @@
+import { expect, test } from "bun:test"
+import {
+  DISPOSED_MARKDOWN_IMAGES_STATE,
+  IMAGE_READ_TIMEOUT_MS,
+  INITIAL_MARKDOWN_IMAGES_STATE,
+  NO_MARKDOWN_IMAGE_EFFECTS,
+  selectMarkdownImageUrl,
+  transitionMarkdownImages,
+  type MarkdownImagesEffect,
+  type MarkdownImagesEvent,
+  type MarkdownImagesState,
+} from "./markdown-image-machine"
+
+const synced = (...paths: string[]): MarkdownImagesEvent => ({ type: "paths.synced", paths: new Set(paths) })
+
+const step = (state: MarkdownImagesState, event: MarkdownImagesEvent) => transitionMarkdownImages(state, event)
+
+const read = (path: string, attempt: number): MarkdownImagesEffect => ({
+  type: "read",
+  path,
+  attempt,
+  timeoutMs: IMAGE_READ_TIMEOUT_MS,
+})
+
+test("retries a failed read after a backoff and loads, while streaming updates change nothing", () => {
+  const loading = step(INITIAL_MARKDOWN_IMAGES_STATE, synced("chart.png"))
+  expect(loading.effects).toEqual([read("chart.png", 1)])
+
+  const waiting = step(loading.state, { type: "read.failed", path: "chart.png", attempt: 1 })
+  expect(waiting.effects).toEqual([
+    { type: "cancel", attempt: 1 },
+    { type: "wait", path: "chart.png", attempt: 2, delayMs: 1000 },
+  ])
+
+  const streamed = step(waiting.state, synced("chart.png"))
+  expect(streamed.state).toBe(waiting.state)
+  expect(streamed.effects).toBe(NO_MARKDOWN_IMAGE_EFFECTS)
+
+  const retried = step(waiting.state, { type: "retry.due", path: "chart.png", attempt: 2 })
+  expect(retried.effects).toEqual([{ type: "cancel", attempt: 2 }, read("chart.png", 3)])
+
+  const loaded = step(retried.state, {
+    type: "read.succeeded",
+    path: "chart.png",
+    attempt: 3,
+    url: "blob:1",
+    revoke: true,
+  })
+
+  expect(loaded.effects).toEqual([{ type: "cancel", attempt: 3 }])
+  expect(selectMarkdownImageUrl(loaded.state, "chart.png")).toBe("blob:1")
+})
+
+test("times out a hung read and gives up after the last retry", () => {
+  const timeOut = (state: MarkdownImagesState, attempt: number) =>
+    step(state, { type: "read.timed-out", path: "chart.png", attempt })
+
+  const retry = (state: MarkdownImagesState, attempt: number) =>
+    step(state, { type: "retry.due", path: "chart.png", attempt }).state
+
+  const wait = (attempt: number, delayMs: number): MarkdownImagesEffect => ({
+    type: "wait",
+    path: "chart.png",
+    attempt,
+    delayMs,
+  })
+
+  const first = timeOut(step(INITIAL_MARKDOWN_IMAGES_STATE, synced("chart.png")).state, 1)
+  expect(first.effects).toEqual([{ type: "cancel", attempt: 1 }, wait(2, 1000)])
+  const second = timeOut(retry(first.state, 2), 3)
+  expect(second.effects).toEqual([{ type: "cancel", attempt: 3 }, wait(4, 2000)])
+  const third = timeOut(retry(second.state, 4), 5)
+  expect(third.effects).toEqual([{ type: "cancel", attempt: 5 }, wait(6, 4000)])
+
+  const failed = timeOut(retry(third.state, 6), 7)
+  expect(failed.effects).toEqual([{ type: "cancel", attempt: 7 }])
+  expect(failed.state.status === "active" && failed.state.entries.get("chart.png")).toEqual({ status: "failed" })
+})
+
+test("settles an empty read as failed without a retry", () => {
+  const loading = step(INITIAL_MARKDOWN_IMAGES_STATE, synced("notes.txt"))
+  const failed = step(loading.state, { type: "read.empty", path: "notes.txt", attempt: 1 })
+
+  expect(failed.effects).toEqual([{ type: "cancel", attempt: 1 }])
+  expect(failed.state.status === "active" && failed.state.entries.get("notes.txt")).toEqual({ status: "failed" })
+})
+
+test("ignores late results and revokes their blob URLs", () => {
+  const loading = step(INITIAL_MARKDOWN_IMAGES_STATE, synced("chart.png")).state
+
+  const late = (state: MarkdownImagesState, revoke: boolean) =>
+    step(state, { type: "read.succeeded", path: "chart.png", attempt: 1, url: "blob:stale", revoke })
+
+  const timedOut = step(loading, { type: "read.timed-out", path: "chart.png", attempt: 1 }).state
+  const removed = step(loading, synced()).state
+  const disposed = step(loading, { type: "disposed" }).state
+  const moved = [timedOut, removed, disposed]
+  moved.forEach((state) => {
+    const result = late(state, true)
+    expect(result.state).toBe(state)
+    expect(result.effects).toEqual([{ type: "revoke-url", url: "blob:stale" }])
+    expect(late(state, false).effects).toBe(NO_MARKDOWN_IMAGE_EFFECTS)
+  })
+
+  const stale = step(timedOut, { type: "read.failed", path: "chart.png", attempt: 1 })
+  expect(stale.state).toBe(timedOut)
+  expect(step(timedOut, { type: "retry.due", path: "chart.png", attempt: 1 }).state).toBe(timedOut)
+})
+
+test("cancels and revokes removed entries and everything on dispose", () => {
+  const started = step(INITIAL_MARKDOWN_IMAGES_STATE, synced("a.png", "b.png", "c.png", "d.png")).state
+
+  const events: MarkdownImagesEvent[] = [
+    { type: "read.failed", path: "b.png", attempt: 2 },
+    { type: "read.succeeded", path: "c.png", attempt: 3, url: "blob:c", revoke: true },
+    { type: "read.succeeded", path: "d.png", attempt: 4, url: "data:d", revoke: false },
+  ]
+
+  const settled = events.reduce((state, event) => step(state, event).state, started)
+
+  const removed = step(settled, synced("a.png"))
+  expect(removed.effects).toEqual([
+    { type: "cancel", attempt: 5 },
+    { type: "revoke-url", url: "blob:c" },
+  ])
+  expect(removed.state.status === "active" && [...removed.state.entries.keys()]).toEqual(["a.png"])
+
+  const disposed = step(settled, { type: "disposed" })
+  expect(disposed.state).toBe(DISPOSED_MARKDOWN_IMAGES_STATE)
+  expect(disposed.effects).toEqual([
+    { type: "cancel", attempt: 1 },
+    { type: "cancel", attempt: 5 },
+    { type: "revoke-url", url: "blob:c" },
+  ])
+  expect(step(disposed.state, synced("a.png")).effects).toBe(NO_MARKDOWN_IMAGE_EFFECTS)
+})

packages/session-ui/src/components/markdown-image-machine.ts

@@ -0,0 +1,170 @@
+/** A read that has not settled after this long is aborted and retried, so a stalled request cannot hang the image. */
+export const IMAGE_READ_TIMEOUT_MS = 15_000
+
+/** The first retry waits this long; each later retry doubles it. */
+export const IMAGE_RETRY_BASE_MS = 1_000
+
+export const IMAGE_MAX_RETRIES = 3
+
+/**
+ * One local Markdown image path while it is in the container.
+ * - `loading`: a read is in flight.
+ * - `waiting`: a read failed or timed out and the next one is backing off.
+ * - `loaded`: resolved to an image `src`; `revoke` marks a `blob:` URL the machine must release (an SVG `data:` URL
+ *   needs no release).
+ * - `failed`: the reader has nothing to show (`undefined`) or every retry failed; the image keeps its alt text.
+ */
+export type MarkdownImageEntry =
+  | { readonly status: "loading"; readonly attempt: number; readonly failures: number }
+  | { readonly status: "waiting"; readonly attempt: number; readonly failures: number }
+  | { readonly status: "loaded"; readonly url: string; readonly revoke: boolean }
+  | { readonly status: "failed" }
+
+export type MarkdownImagesState =
+  | {
+      readonly status: "active"
+      /** Last attempt id handed out. Each read and each backoff gets a new one, so late results are recognised. */
+      readonly attempts: number
+      readonly entries: ReadonlyMap<string, MarkdownImageEntry>
+    }
+  | { readonly status: "disposed" }
+
+export type MarkdownImagesEvent =
+  | { readonly type: "paths.synced"; readonly paths: ReadonlySet<string> }
+  | {
+      readonly type: "read.succeeded"
+      readonly path: string
+      readonly attempt: number
+      readonly url: string
+      readonly revoke: boolean
+    }
+  | { readonly type: "read.empty"; readonly path: string; readonly attempt: number }
+  | { readonly type: "read.failed"; readonly path: string; readonly attempt: number }
+  | { readonly type: "read.timed-out"; readonly path: string; readonly attempt: number }
+  | { readonly type: "retry.due"; readonly path: string; readonly attempt: number }
+  | { readonly type: "disposed" }
+
+export type MarkdownImagesEffect =
+  | { readonly type: "read"; readonly path: string; readonly attempt: number; readonly timeoutMs: number }
+  | { readonly type: "wait"; readonly path: string; readonly attempt: number; readonly delayMs: number }
+  | { readonly type: "cancel"; readonly attempt: number }
+  | { readonly type: "revoke-url"; readonly url: string }
+
+export type MarkdownImagesTransition = {
+  readonly state: MarkdownImagesState
+  readonly effects: readonly MarkdownImagesEffect[]
+}
+
+export const INITIAL_MARKDOWN_IMAGES_STATE: MarkdownImagesState = Object.freeze({
+  status: "active",
+  attempts: 0,
+  entries: new Map(),
+})
+
+export const DISPOSED_MARKDOWN_IMAGES_STATE: MarkdownImagesState = Object.freeze({ status: "disposed" })
+
+export const NO_MARKDOWN_IMAGE_EFFECTS: readonly MarkdownImagesEffect[] = Object.freeze([])
+
+export function transitionMarkdownImages(
+  state: MarkdownImagesState,
+  event: MarkdownImagesEvent,
+): MarkdownImagesTransition {
+  if (state.status === "disposed") return ignore(state, event)
+
+  if (event.type === "disposed")
+    return { state: DISPOSED_MARKDOWN_IMAGES_STATE, effects: [...state.entries.values()].flatMap(release) }
+
+  if (event.type === "paths.synced") return sync(state, event.paths)
+
+  if (event.type === "retry.due") {
+    const entry = state.entries.get(event.path)
+
+    if (entry?.status !== "waiting" || entry.attempt !== event.attempt) return ignore(state, event)
+    const attempt = state.attempts + 1
+
+    return {
+      state: withEntry(state, event.path, { status: "loading", attempt, failures: entry.failures }, attempt),
+      effects: [
+        { type: "cancel", attempt: entry.attempt },
+        { type: "read", path: event.path, attempt, timeoutMs: IMAGE_READ_TIMEOUT_MS },
+      ],
+    }
+  }
+
+  const entry = state.entries.get(event.path)
+
+  if (entry?.status !== "loading" || entry.attempt !== event.attempt) return ignore(state, event)
+  const cancel = { type: "cancel", attempt: entry.attempt } as const
+
+  if (event.type === "read.succeeded")
+    return {
+      state: withEntry(state, event.path, { status: "loaded", url: event.url, revoke: event.revoke }, state.attempts),
+      effects: [cancel],
+    }
+
+  if (event.type === "read.empty" || entry.failures >= IMAGE_MAX_RETRIES)
+    return { state: withEntry(state, event.path, { status: "failed" }, state.attempts), effects: [cancel] }
+  const attempt = state.attempts + 1
+  const failures = entry.failures + 1
+
+  return {
+    state: withEntry(state, event.path, { status: "waiting", attempt, failures }, attempt),
+    effects: [cancel, { type: "wait", path: event.path, attempt, delayMs: IMAGE_RETRY_BASE_MS * 2 ** entry.failures }],
+  }
+}
+
+export function selectMarkdownImageUrl(state: MarkdownImagesState, path: string) {
+  if (state.status === "disposed") return
+  const entry = state.entries.get(path)
+
+  return entry?.status === "loaded" ? entry.url : undefined
+}
+
+function sync(
+  state: Extract<MarkdownImagesState, { status: "active" }>,
+  paths: ReadonlySet<string>,
+): MarkdownImagesTransition {
+  const removed = [...state.entries].filter(([path]) => !paths.has(path))
+  const added = [...paths].filter((path) => path && !state.entries.has(path))
+
+  if (removed.length === 0 && added.length === 0) return { state, effects: NO_MARKDOWN_IMAGE_EFFECTS }
+  const entries = new Map(state.entries)
+  removed.forEach(([path]) => entries.delete(path))
+
+  const reads = added.map((path, index) => {
+    const attempt = state.attempts + index + 1
+    entries.set(path, { status: "loading", attempt, failures: 0 })
+
+    return { type: "read", path, attempt, timeoutMs: IMAGE_READ_TIMEOUT_MS } as const
+  })
+
+  return {
+    state: { status: "active", attempts: state.attempts + added.length, entries },
+    effects: [...removed.flatMap(([, entry]) => release(entry)), ...reads],
+  }
+}
+
+function release(entry: MarkdownImageEntry): MarkdownImagesEffect[] {
+  if (entry.status === "loading" || entry.status === "waiting") return [{ type: "cancel", attempt: entry.attempt }]
+
+  if (entry.status === "loaded" && entry.revoke) return [{ type: "revoke-url", url: entry.url }]
+
+  return []
+}
+
+function withEntry(
+  state: Extract<MarkdownImagesState, { status: "active" }>,
+  path: string,
+  entry: MarkdownImageEntry,
+  attempts: number,
+): MarkdownImagesState {
+  return { status: "active", attempts, entries: new Map(state.entries).set(path, entry) }
+}
+
+// A reader that ignores its abort signal can still hand back a blob URL after the entry moved on; release it.
+function ignore(state: MarkdownImagesState, event: MarkdownImagesEvent): MarkdownImagesTransition {
+  if (event.type === "read.succeeded" && event.revoke)
+    return { state, effects: [{ type: "revoke-url", url: event.url }] }
+
+  return { state, effects: NO_MARKDOWN_IMAGE_EFFECTS }
+}

packages/session-ui/src/components/markdown-image.test.ts

@@ -1,5 +1,6 @@
-import { expect, test } from "bun:test"
-import { localImagePath, localLinkPath } from "./markdown-image"
+import { GlobalRegistrator } from "@happy-dom/global-registrator"
+import { afterAll, afterEach, beforeAll, describe, expect, jest, test } from "bun:test"
+import { createMarkdownImages, localImagePath, localLinkPath } from "./markdown-image"
 
 test.each([
   ["./out/report.html", "./out/report.html"],
@@ -58,3 +59,50 @@ test.each([
 ])("does not read non-local or invalid source %s", (source) => {
   expect(localImagePath(source)).toBeUndefined()
 })
+
+describe("createMarkdownImages", () => {
+  // Happy DOM's fetch cannot read `blob:` URLs; Bun's can, and fails once the URL is revoked.
+  const nativeFetch = fetch
+
+  // Lets the read's promise chain settle; fake timers do not drive microtasks.
+  const settle = (ticks = 10): Promise<void> =>
+    ticks === 0 ? Promise.resolve() : Promise.resolve().then(() => settle(ticks - 1))
+
+  beforeAll(() => GlobalRegistrator.register())
+
+  afterAll(() => GlobalRegistrator.unregister())
+
+  afterEach(() => jest.useRealTimers())
+
+  test("retries a failed read and shows the image without another update", async () => {
+    jest.useFakeTimers()
+    const reads: string[] = []
+
+    const images = createMarkdownImages(async (path) => {
+      reads.push(path)
+
+      if (reads.length === 1) throw new Error("not written yet")
+
+      return new Blob(["png"], { type: "image/png" })
+    })
+
+    const root = document.createElement("div")
+    root.innerHTML = '<img data-local-image="./out/chart.png" alt="chart">'
+    document.body.replaceChildren(root)
+    const image = root.querySelector("img")!
+
+    images.update(root)
+    await settle()
+    expect(image.getAttribute("src")).toBeNull()
+
+    jest.advanceTimersToNextTimer()
+    await settle()
+    const url = image.getAttribute("src")
+    expect(reads).toEqual(["./out/chart.png", "./out/chart.png"])
+    expect(url).toStartWith("blob:")
+    expect(await nativeFetch(url!).then((response) => response.text())).toBe("png")
+
+    images.dispose()
+    await expect(nativeFetch(url!)).rejects.toThrow()
+  })
+})

packages/session-ui/src/components/markdown-image.ts

@@ -1,6 +1,13 @@
 import { abort } from "@opencode/util/abort"
 import { isLineRangeHash, parsePathLineSuffix } from "@opencode/util/path"
 import type { ReadMarkdownImage } from "../context/markdown"
+import {
+  INITIAL_MARKDOWN_IMAGES_STATE,
+  selectMarkdownImageUrl,
+  transitionMarkdownImages,
+  type MarkdownImagesEffect,
+  type MarkdownImagesEvent,
+} from "./markdown-image-machine"
 
 export function localImagePath(source: string) {
   // Marked percent-encodes image sources, so `C:\tmp\chart.png` arrives as `C:%5Ctmp%5Cchart.png`.
@@ -63,63 +70,87 @@ function decodePath(value: string) {
   }
 }
 
-type ImageEntry = { controller: AbortController; result: Promise<string | undefined>; url?: string }
-
 export function createMarkdownImages(read: ReadMarkdownImage) {
-  const entries = new Map<string, ImageEntry>()
+  let state = INITIAL_MARKDOWN_IMAGES_STATE
+  let root: HTMLElement | undefined
+  const pending = new Map<number, { controller?: AbortController; timer: ReturnType<typeof setTimeout> }>()
+
+  const images = (container: HTMLElement) =>
+    Array.from(container.querySelectorAll<HTMLImageElement>("img[data-local-image]"))
+
+  const syncDom = (container: HTMLElement) =>
+    images(container).forEach((image) => {
+      const url = selectMarkdownImageUrl(state, image.dataset.localImage ?? "")
+
+      if (url && image.getAttribute("src") !== url) image.src = url
+    })
+
+  const dispatch = (event: MarkdownImagesEvent) => {
+    const next = transitionMarkdownImages(state, event)
+    const changed = next.state !== state
+    state = next.state
+
+    if (changed && root) syncDom(root)
+    next.effects.forEach(run)
+  }
+
+  const run = (effect: MarkdownImagesEffect) => {
+    if (effect.type === "revoke-url") return URL.revokeObjectURL(effect.url)
+
+    if (effect.type === "cancel") {
+      const item = pending.get(effect.attempt)
+      pending.delete(effect.attempt)
+
+      if (!item) return
+      clearTimeout(item.timer)
+      abort(item.controller)
+
+      return
+    }
+
+    if (effect.type === "wait") {
+      const retry = { type: "retry.due", path: effect.path, attempt: effect.attempt } as const
+      pending.set(effect.attempt, { timer: setTimeout(() => dispatch(retry), effect.delayMs) })
+
+      return
+    }
+
+    const controller = new AbortController()
+    const result = { path: effect.path, attempt: effect.attempt }
+    const timer = setTimeout(() => dispatch({ type: "read.timed-out", ...result }), effect.timeoutMs)
+    pending.set(effect.attempt, { controller, timer })
+    void read(effect.path, controller.signal)
+      .then(async (blob) => {
+        if (controller.signal.aborted) return
+
+        if (!blob) return dispatch({ type: "read.empty", ...result })
+
+        // SVG documents must not inherit the app origin if opened outside the image element.
+        if (blob.type === "image/svg+xml") {
+          const url = `data:image/svg+xml;charset=utf-8,${encodeURIComponent(await blob.text())}`
+
+          return dispatch({ type: "read.succeeded", ...result, url, revoke: false })
+        }
+
+        dispatch({ type: "read.succeeded", ...result, url: URL.createObjectURL(blob), revoke: true })
+      })
+      .catch(() => {
+        if (controller.signal.aborted) return
+        dispatch({ type: "read.failed", ...result })
+      })
+  }
 
   return {
-    update(root: HTMLElement) {
-      const images = Array.from(root.querySelectorAll<HTMLImageElement>("img[data-local-image]"))
-      const paths = new Set(images.map((image) => image.dataset.localImage))
-      entries.forEach((entry, path) => {
-        if (paths.has(path)) return
-        abort(entry.controller)
-
-        if (entry.url) URL.revokeObjectURL(entry.url)
-        entries.delete(path)
-      })
-      images.forEach((image) => {
-        const path = image.dataset.localImage
-
-        if (!path) return
-        const existing = entries.get(path)
-
-        const entry: ImageEntry = existing ?? {
-          controller: new AbortController(),
-          result: Promise.resolve(undefined),
-        }
-
-        if (!existing) {
-          entries.set(path, entry)
-          entry.result = read(path, entry.controller.signal)
-            .then(async (blob) => {
-              if (!blob || entry.controller.signal.aborted) return
-
-              // SVG documents must not inherit the app origin if opened outside the image element.
-              if (blob.type === "image/svg+xml")
-                return `data:image/svg+xml;charset=utf-8,${encodeURIComponent(await blob.text())}`
-              entry.url = URL.createObjectURL(blob)
-
-              return entry.url
-            })
-            .catch(() => undefined)
-        }
-
-        void entry.result.then((url) => {
-          if (!url || entry.controller.signal.aborted || !root.contains(image) || image.dataset.localImage !== path)
-            return
-          image.src = url
-        })
-      })
+    update(container: HTMLElement) {
+      root = container
+      const paths = new Set(images(container).flatMap((image) => image.dataset.localImage || []))
+      dispatch({ type: "paths.synced", paths })
+      // Newly inserted images of an already loaded path get their `src` before paint.
+      syncDom(container)
     },
     dispose() {
-      entries.forEach((entry) => {
-        abort(entry.controller)
-
-        if (entry.url) URL.revokeObjectURL(entry.url)
-      })
-      entries.clear()
+      root = undefined
+      dispatch({ type: "disposed" })
     },
   }
 }

packages/session-ui/src/pierre/comment-hover-machine.test.ts

@@ -0,0 +1,47 @@
+import { expect, test } from "bun:test"
+import {
+  DISPOSED_HOVER_COMMENT_STATE,
+  INITIAL_HOVER_COMMENT_STATE,
+  selectHoverActive,
+  selectHoverLine,
+  transitionHoverComment,
+  type HoverCommentEvent,
+  type HoverCommentSide,
+} from "./comment-hover-machine"
+
+const hover = (lineNumber?: number, side?: HoverCommentSide): HoverCommentEvent => ({
+  type: "hover.synced",
+  lineNumber,
+  side,
+})
+
+test("keeps the last hovered line after the pointer leaves the code", () => {
+  const hovered = transitionHoverComment(INITIAL_HOVER_COMMENT_STATE, hover(12, "additions"))
+  const left = transitionHoverComment(hovered, hover())
+
+  expect(left).toBe(hovered)
+  expect(selectHoverLine(left)).toEqual({ lineNumber: 12, side: "additions" })
+  expect(selectHoverLine(transitionHoverComment(left, hover(12, "deletions")))).toEqual({
+    lineNumber: 12,
+    side: "deletions",
+  })
+})
+
+test("returns the same state when the hovered line does not change", () => {
+  const hovered = transitionHoverComment(INITIAL_HOVER_COMMENT_STATE, hover(7, "deletions"))
+
+  expect(transitionHoverComment(INITIAL_HOVER_COMMENT_STATE, hover())).toBe(INITIAL_HOVER_COMMENT_STATE)
+  expect(transitionHoverComment(hovered, hover(7, "deletions"))).toBe(hovered)
+  expect(transitionHoverComment(DISPOSED_HOVER_COMMENT_STATE, hover(7))).toBe(DISPOSED_HOVER_COMMENT_STATE)
+})
+
+test("stops listening and forgets the line once disposed", () => {
+  const disposed = transitionHoverComment(transitionHoverComment(INITIAL_HOVER_COMMENT_STATE, hover(3)), {
+    type: "disposed",
+  })
+
+  expect(disposed).toBe(DISPOSED_HOVER_COMMENT_STATE)
+  expect(selectHoverActive(disposed)).toBe(false)
+  expect(selectHoverLine(disposed)).toBeUndefined()
+  expect(transitionHoverComment(disposed, { type: "disposed" })).toBe(disposed)
+})

packages/session-ui/src/pierre/comment-hover-machine.ts

@@ -0,0 +1,53 @@
+export type HoverCommentSide = "additions" | "deletions"
+
+export type HoverCommentLine = {
+  readonly lineNumber: number
+  readonly side?: HoverCommentSide
+}
+
+/**
+ * One gutter comment `+` button. Runs on every pointer move and scroll, so it returns the same object when nothing
+ * changes and reads its events from one reused object.
+ * - `tracking`: listens to the document and caches the last hovered line, so a click still has one after the pointer
+ *   leaves the code. Leaving the document does not end it: Pierre inserts the button after creating it, and may detach
+ *   and reattach the viewer.
+ * - `disposed`: its owner replaced or unmounted it; the document listeners are gone.
+ */
+export type HoverCommentState =
+  | { readonly status: "tracking"; readonly line: HoverCommentLine | undefined }
+  | { readonly status: "disposed" }
+
+/** The line Pierre reports as hovered. Mutable so the driver can reuse one object for every pointer move. */
+export type HoverCommentSync = {
+  type: "hover.synced"
+  lineNumber: number | undefined
+  side: HoverCommentSide | undefined
+}
+
+export type HoverCommentEvent = Readonly<HoverCommentSync> | { readonly type: "disposed" }
+
+export const INITIAL_HOVER_COMMENT_STATE: HoverCommentState = Object.freeze({ status: "tracking", line: undefined })
+
+export const DISPOSED_HOVER_COMMENT_STATE: HoverCommentState = Object.freeze({ status: "disposed" })
+
+export function transitionHoverComment(state: HoverCommentState, event: HoverCommentEvent): HoverCommentState {
+  if (state.status === "disposed") return state
+
+  if (event.type === "disposed") return DISPOSED_HOVER_COMMENT_STATE
+  const lineNumber = event.lineNumber
+
+  // No hovered line (the pointer left the code) keeps the cached one.
+  if (lineNumber === undefined || (lineNumber === state.line?.lineNumber && event.side === state.line?.side))
+    return state
+
+  return { status: "tracking", line: { lineNumber, side: event.side } }
+}
+
+/** Whether the button still listens to the document, and so whether a render may hand it back again. */
+export function selectHoverActive(state: HoverCommentState) {
+  return state.status !== "disposed"
+}
+
+export function selectHoverLine(state: HoverCommentState) {
+  return state.status === "disposed" ? undefined : state.line
+}

packages/session-ui/src/pierre/comment-hover.ts

@@ -1,13 +1,28 @@
-export type HoverCommentLine = {
-  lineNumber: number
-  side?: "additions" | "deletions"
+import {
+  INITIAL_HOVER_COMMENT_STATE,
+  selectHoverActive,
+  selectHoverLine,
+  transitionHoverComment,
+  type HoverCommentEvent,
+  type HoverCommentLine,
+  type HoverCommentSync,
+} from "./comment-hover-machine"
+
+export type { HoverCommentLine }
+
+/** A gutter `+` button and the hover tracking behind it. Dispose it when its viewer stops rendering it. */
+export type HoverComment = {
+  button: HTMLButtonElement
+  dispose: () => void
+  /** Whether the next render may return this button instead of building another. */
+  canReuse: () => boolean
 }
 
-export function createHoverCommentUtility(props: {
+export function createHoverComment(props: {
   label: string
   getHoveredLine: () => HoverCommentLine | undefined
   onSelect: (line: HoverCommentLine) => void
-}) {
+}): HoverComment | undefined {
   if (typeof document === "undefined") return
 
   const button = document.createElement("button")
@@ -42,40 +57,35 @@ export function createHoverCommentUtility(props: {
   button.style.left = "30px"
   button.style.top = "calc((var(--diffs-line-height, 24px) - 20px) / 2)"
 
-  let line: HoverCommentLine | undefined
+  let state = INITIAL_HOVER_COMMENT_STATE
+  // Reused for every pointer move and scroll so tracking allocates nothing.
+  const syncEvent: HoverCommentSync = { type: "hover.synced", lineNumber: undefined, side: undefined }
 
-  const sync = () => {
-    const next = props.getHoveredLine()
+  const dispatch = (event: HoverCommentEvent) => {
+    const next = transitionHoverComment(state, event)
 
-    if (!next) return
-    line = next
-  }
+    if (next === state) return
+    const active = selectHoverActive(state)
+    state = next
 
-  // The hovered line changes when the pointer moves or when content scrolls under
-  // a stationary pointer, so track both with passive listeners instead of polling
-  // every animation frame. There is no teardown hook for this utility; like the
-  // rAF loop this replaced, the listeners self-detach on the first event after
-  // the button leaves the DOM.
-  const onHoverInvalidated = () => {
-    if (!button.isConnected) {
-      document.removeEventListener("pointermove", onHoverInvalidated)
-      document.removeEventListener("scroll", onHoverInvalidated, true)
-
-      return
+    if (active && !selectHoverActive(state)) {
+      document.removeEventListener("pointermove", sync)
+      document.removeEventListener("scroll", sync, true)
     }
-
-    sync()
   }
 
-  const open = () => {
-    const next = props.getHoveredLine() ?? line
-
-    if (!next) return
-    props.onSelect(next)
+  // The hovered line changes when the pointer moves or when content scrolls under a stationary pointer, so track both
+  // with passive listeners instead of polling every animation frame.
+  const sync = () => {
+    if (!button.isConnected) return
+    const hovered = props.getHoveredLine()
+    syncEvent.lineNumber = hovered?.lineNumber
+    syncEvent.side = hovered?.side
+    dispatch(syncEvent)
   }
 
-  document.addEventListener("pointermove", onHoverInvalidated, { passive: true })
-  document.addEventListener("scroll", onHoverInvalidated, { passive: true, capture: true })
+  document.addEventListener("pointermove", sync, { passive: true })
+  document.addEventListener("scroll", sync, { passive: true, capture: true })
   button.addEventListener("mouseenter", sync)
   button.addEventListener("mousemove", sync)
   button.addEventListener("pointerdown", (event) => {
@@ -91,8 +101,15 @@ export function createHoverCommentUtility(props: {
   button.addEventListener("click", (event) => {
     event.preventDefault()
     event.stopPropagation()
-    open()
+    sync()
+    const line = selectHoverLine(state)
+
+    if (line) props.onSelect(line)
   })
 
-  return button
+  return {
+    button,
+    dispose: () => dispatch({ type: "disposed" }),
+    canReuse: () => selectHoverActive(state),
+  }
 }