diff --git a/packages/session-ui/src/components/line-comment-annotations.test.ts b/packages/session-ui/src/components/line-comment-annotations.test.ts new file mode 100644 index 00000000000..869a6659590 --- /dev/null +++ b/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) +}) diff --git a/packages/session-ui/src/components/line-comment-annotations.tsx b/packages/session-ui/src/components/line-comment-annotations.tsx index 4686d536dd7..789e21c9fa3 100644 --- a/packages/session-ui/src/components/line-comment-annotations.tsx +++ b/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 = LineCommentAnnotationsProps & { 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 @@ -538,8 +533,17 @@ export function createLineCommentGutterRenderer(props: { getSelectedRange: Accessor 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 + } } diff --git a/packages/session-ui/src/components/markdown-image-machine.test.ts b/packages/session-ui/src/components/markdown-image-machine.test.ts new file mode 100644 index 00000000000..afdc834d626 --- /dev/null +++ b/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) +}) diff --git a/packages/session-ui/src/components/markdown-image-machine.ts b/packages/session-ui/src/components/markdown-image-machine.ts new file mode 100644 index 00000000000..ff44ecedf84 --- /dev/null +++ b/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 + } + | { readonly status: "disposed" } + +export type MarkdownImagesEvent = + | { readonly type: "paths.synced"; readonly paths: ReadonlySet } + | { + 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, + paths: ReadonlySet, +): 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, + 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 } +} diff --git a/packages/session-ui/src/components/markdown-image.test.ts b/packages/session-ui/src/components/markdown-image.test.ts index e53e657b7d6..6eac2072e47 100644 --- a/packages/session-ui/src/components/markdown-image.test.ts +++ b/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 => + 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 = '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() + }) +}) diff --git a/packages/session-ui/src/components/markdown-image.ts b/packages/session-ui/src/components/markdown-image.ts index d829d780a3d..a7a8edc0901 100644 --- a/packages/session-ui/src/components/markdown-image.ts +++ b/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; url?: string } - export function createMarkdownImages(read: ReadMarkdownImage) { - const entries = new Map() + let state = INITIAL_MARKDOWN_IMAGES_STATE + let root: HTMLElement | undefined + const pending = new Map }>() + + const images = (container: HTMLElement) => + Array.from(container.querySelectorAll("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("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" }) }, } } diff --git a/packages/session-ui/src/pierre/comment-hover-machine.test.ts b/packages/session-ui/src/pierre/comment-hover-machine.test.ts new file mode 100644 index 00000000000..b3d3337d934 --- /dev/null +++ b/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) +}) diff --git a/packages/session-ui/src/pierre/comment-hover-machine.ts b/packages/session-ui/src/pierre/comment-hover-machine.ts new file mode 100644 index 00000000000..d18ceb163de --- /dev/null +++ b/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 | { 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 +} diff --git a/packages/session-ui/src/pierre/comment-hover.ts b/packages/session-ui/src/pierre/comment-hover.ts index b01ff116d52..80c3d459259 100644 --- a/packages/session-ui/src/pierre/comment-hover.ts +++ b/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), + } }