From 3d54937be677300a48ef3538513c4dcbfac920b3 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Fri, 20 Mar 2026 00:25:59 -0700 Subject: [PATCH] fix: layout shift stuff --- package.json | 8 +- src/Memoizer.ts | 87 ++++++++++-- tests/Markdown.memoization.test.tsx | 164 +++++++++++++++++++++- tests/Memoizer.test.tsx | 206 ++++++++++++++++++++++++++++ tests/prediction.test.ts | 5 +- 5 files changed, 449 insertions(+), 21 deletions(-) create mode 100644 tests/Memoizer.test.tsx diff --git a/package.json b/package.json index 96ca073dcd8ac677ea026312552078e1b6151d40..8fd719e6c8971d573d3735a4d7f77aaca1e5b8db 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,10 @@ { "name": "@clo/react-markdown", "type": "module", + "exports": { + ".": "./src/Markdown.ts", + "./Predict": "./src/Predict.ts" + }, "scripts": { "demo": "vite dev", "all": "pnpm --parallel fmt && pnpm --stream '/check|test$/'", @@ -9,10 +13,6 @@ "check:publish": "deno publish --dry-run --allow-dirty", "fmt": "oxfmt --write ." }, - "exports": { - ".": "./src/Markdown.ts", - "./Predict": "./src/Predict.ts" - }, "dependencies": { "@clo/lib": "npm:@jsr/clo__lib@3.0.0", "@types/react": "^19", diff --git a/src/Memoizer.ts b/src/Memoizer.ts index d339844a00c5ec4bf86bb86fcab91fc9d7f5a8a5..2586edc02dab0139915bab796e0767a068689cf7 100644 --- a/src/Memoizer.ts +++ b/src/Memoizer.ts @@ -4,9 +4,9 @@ import { type Components } from "rehype-react"; import remarkRehype from "remark-rehype"; import { Fragment, jsx } from "react/jsx-runtime"; import { type Processor, unified } from "unified"; -import type { Root as HastRoot } from "hast"; +import type { Root as HastRoot, Root } from "hast"; import type { Literal, Node, Parent } from "unist"; -import { memoizedHastToReact, type RenderState } from "./hast.ts"; +import { memoizedHastToReact, nodeDeepEquals, type RenderState } from "./hast.ts"; import remarkParse from "remark-parse"; import { Predict } from "./Predict.ts"; @@ -38,6 +38,9 @@ export class Memoizer { #content: string = ""; #positions: number[] = []; #parsed: Node[][] = []; + // React block identity must survive index shifts when blocks are inserted above. + #keys: string[] = []; + #nextKey = 0; #renderStates: (RenderState | null)[] = []; #reactNodes: readonly ReactNode[] = []; @@ -67,6 +70,8 @@ export class Memoizer { this.#content = ""; this.#positions = []; this.#parsed = []; + this.#keys = []; + this.#nextKey = 0; this.#renderStates = []; this.#reactNodes = []; this.#predict = predict ? new Predict() : null; @@ -126,15 +131,75 @@ export class Memoizer { const newTransformed = extractBlocks(processor.runSync(parsedTree), newPositions, parseOffset); // React + const keys = this.#keys; + const renderStates = this.#renderStates; + const previousReactNodes = this.#reactNodes; + const previousLength = renderStates.length; let reactNodes: ReactNode[] | null = null; - if (this.#reactNodes.length !== positions.length) { - // If items are removed, the array must be resliced. - reactNodes = this.#reactNodes.slice(0, positions.length); - this.#renderStates.length = positions.length; + + // Insertions and removals above an unchanged tail shift indices without + // changing block content. Matching the shared suffix is enough to keep the + // unaffected tail bound to its previous keys and render states. + let suffixLength = 0; + for (; suffixLength < newTransformed.length; suffixLength += 1) { + const nextIndex = positions.length - 1 - suffixLength; + const previousIndex = previousLength - 1 - suffixLength; + if (nextIndex < blockStart || previousIndex < blockStart) break; + + const next = UNWRAP(newTransformed[nextIndex - blockStart]); + const state = renderStates[previousIndex]; + const prev = state?.node.type === "root" ? (state.node as Root) : null; + if (!prev || !nodeDeepEquals(next, prev.children)) break; + } + + // (If realignment is going to occurs, the array must be cloned) + const previousMiddleEnd = previousLength - suffixLength; + const nextMiddleEnd = positions.length - suffixLength; + const realign = previousLength !== positions.length || previousMiddleEnd !== nextMiddleEnd; + if (realign) reactNodes = previousReactNodes.slice(0, positions.length); + + if (positions.length > previousLength) { + keys.length = renderStates.length = positions.length; + } + // Realignment reads from the previous layout and writes the next layout + // into the same backing arrays. Traversal direction prevents later reads + // from observing values that were already overwritten earlier in the pass. + for ( + let direction = positions.length < previousLength ? 1 : -1, + i = direction === 1 ? 0 : positions.length - 1, + end = direction === 1 ? positions.length : -1; + i !== end; + i += direction + ) { + let previousIndex: number = -1; + if (i < blockStart) { + previousIndex = i; + } else if (i >= nextMiddleEnd) { + previousIndex = previousLength - (positions.length - i); + } else if (i < previousMiddleEnd) { + previousIndex = i; + } + if (previousIndex < 0 || previousIndex >= previousLength) { + keys[i] = String(this.#nextKey); + this.#nextKey += 1; + renderStates[i] = null; + if (reactNodes) reactNodes[i] = undefined as ReactNode | undefined; + continue; + } + + keys[i] = keys[previousIndex] ?? String("m" + this.#nextKey++); + renderStates[i] = renderStates[previousIndex] ?? null; + if (reactNodes) reactNodes[i] = previousReactNodes[previousIndex]; + } + + // Create and splice the React elements + if (positions.length < previousLength) { + keys.length = renderStates.length = positions.length; } for (let i = blockStart, len = positions.length; i < len; i += 1) { const ast = UNWRAP(newTransformed[i - blockStart]); - const previousState = this.#renderStates[i] ?? null; + const previousState = renderStates[i] ?? null; + // `memoizedHastToReact` handles diffing and incrementally updating the AST. let { react: result, state } = memoizedHastToReact( { type: "root", children: ast } as HastRoot, @@ -142,10 +207,10 @@ export class Memoizer { this.#components, ); if (previousState === state) continue; - this.#renderStates[i] = state; + renderStates[i] = state; // Do a little trolling and unwrap the fragment to clean up the Virtual DOM - const key = String("m" + i); + const key = keys[i]!; const rendered = result as JSX.Element; const children = rendered.props.children; if (rendered.type === Fragment && children?.type) { @@ -153,9 +218,7 @@ export class Memoizer { } else { result = setReactKey(rendered, key); } - - // If anything in the array changes, the array must be sliced. - reactNodes ??= this.#reactNodes.slice(); + reactNodes ??= previousReactNodes.slice(0, positions.length); reactNodes[i] = result; } diff --git a/tests/Markdown.memoization.test.tsx b/tests/Markdown.memoization.test.tsx index a9831dc2604f3da725917b96bfdce316343fb9ef..8e67434fe28ca6c01f6915cc427a6d9979b7d810 100644 --- a/tests/Markdown.memoization.test.tsx +++ b/tests/Markdown.memoization.test.tsx @@ -1,15 +1,20 @@ import { cleanup, render, screen } from "@testing-library/react"; import type { Components } from "rehype-react"; import type { ComponentPropsWithoutRef, JSX } from "react"; +import { useRef } from "react"; +import remarkGfm from "remark-gfm"; +import remarkParse from "remark-parse"; +import { unified } from "unified"; import { afterEach, expect, it, vi } from "vitest"; import { Markdown, MarkdownOptionsProvider } from "../src/Markdown.ts"; -import { Memoizer } from "../src/Memoizer.ts"; +import { type BaseProcessor, Memoizer } from "../src/Memoizer.ts"; type MarkdownProps = ComponentPropsWithoutRef & { node?: unknown; }; const renderOptions = { reactStrictMode: false } as const; +const gfmProcessor = unified().use(remarkParse).use(remarkGfm) as BaseProcessor; afterEach(() => { cleanup(); @@ -129,6 +134,79 @@ it("does not rerender an unchanged inline custom renderer with formatted childre expect(Link).toHaveBeenCalledTimes(1); }); +it("keeps an unchanged strong renderer asleep when surrounding paragraph text changes", () => { + const Strong = vi.fn(function Strong(props: MarkdownProps<"strong">) { + return ; + }); + const components = { strong: Strong } satisfies Partial; + + const { rerender } = render( + , + renderOptions, + ); + + const strong = screen.getByTestId("strong"); + expect(Strong).toHaveBeenCalledTimes(1); + + rerender(); + rerender(); + + expect(screen.getByTestId("strong")).toBe(strong); + expect(Strong).toHaveBeenCalledTimes(1); +}); + +it("keeps an unchanged list item renderer asleep when a sibling item changes", () => { + const ListItem = vi.fn(function ListItem(props: MarkdownProps<"li">) { + return
  • ; + }); + const components = { li: ListItem } satisfies Partial; + + const { rerender } = render( + , + renderOptions, + ); + + const stableItem = screen.getByText("stable").closest("li"); + expect(stableItem).not.toBeNull(); + expect(ListItem).toHaveBeenCalledTimes(3); + + rerender(); + + expect(screen.getByText("stable").closest("li")).toBe(stableItem); + expect(ListItem).toHaveBeenCalledTimes(4); +}); + +it("keeps an unchanged table cell renderer asleep when another cell changes", () => { + const TableCell = vi.fn(function TableCell(props: MarkdownProps<"td">) { + return ; + }); + const components = { td: TableCell } satisfies Partial; + + const { rerender } = render( + , + renderOptions, + ); + + const stableCell = screen.getByText("keep").closest("td"); + expect(stableCell).not.toBeNull(); + expect(TableCell).toHaveBeenCalledTimes(2); + + rerender( + , + ); + + expect(screen.getByText("keep").closest("td")).toBe(stableCell); + expect(TableCell).toHaveBeenCalledTimes(3); +}); + it("does not rerender Markdown through the provider when the effective components stay the same", () => { const updateSpy = vi.spyOn(Memoizer.prototype, "update"); const Link = vi.fn(function Link(props: MarkdownProps<"a">) { @@ -169,3 +247,87 @@ it("reprocesses the document when prediction changes", () => { expect(container.querySelector("em")?.textContent).toBe("world"); expect(container.textContent).toBe("hello world"); }); + +it.each([ + { + content: ["alpha\n\nbeta", "alpha\nbeta", "alpha\n\nbeta\n\ngamma", "alpha\n\nbeta"], + name: "paragraph merging edits", + predict: false, + processor: undefined, + }, + { + content: ["[ref][id]\n\n[id]: /one", "[ref][id]\n\n[id]: /two", "[ref][id]"], + name: "reference link definition edits", + predict: false, + processor: undefined, + }, + { + content: [ + "| A | B |\n| --- | --- |\n| one | two |", + "| A | B |\n| --- | --- |\n| one! | two |", + "| A | B |\n| --- | --- |\n| one! | two |\n| three | four |", + ], + name: "gfm table edits", + predict: false, + processor: gfmProcessor, + }, + { + content: ["hello *world", "hello brave *world", "hello brave world", "hello brave `world"], + name: "prediction edits", + predict: true, + processor: undefined, + }, +])("matches a fresh render across $name", ({ content, predict, processor }) => { + const first = content[0]!; + const { container, rerender } = render( + , + renderOptions, + ); + + expect(container.innerHTML).toBe(renderFreshHtml(first, predict, processor)); + + for (const step of content.slice(1)) { + rerender(); + expect(container.innerHTML).toBe(renderFreshHtml(step, predict, processor)); + } +}); + +it("keeps a stable block tied to its own DOM node when blocks are inserted or removed above it", () => { + function Link(props: MarkdownProps<"a">) { + const instanceRef = useRef(Symbol("instance")); + return ; + } + + const components = { a: Link } satisfies Partial; + const { rerender } = render( + , + renderOptions, + ); + + const stableLink = screen.getByRole("link"); + + rerender( + , + ); + expect(screen.getByRole("link")).toBe(stableLink); + + rerender(); + expect(screen.getByRole("link")).toBe(stableLink); +}); + +function renderFreshHtml( + content: string, + predict: boolean | undefined, + processor: BaseProcessor | undefined, +) { + const { container, unmount } = render( + , + renderOptions, + ); + const html = container.innerHTML; + unmount(); + return html; +} diff --git a/tests/Memoizer.test.tsx b/tests/Memoizer.test.tsx new file mode 100644 index 0000000000000000000000000000000000000000..26ee0f3a44136d3678d1c8475dc41f8a19708882 --- /dev/null +++ b/tests/Memoizer.test.tsx @@ -0,0 +1,206 @@ +import type { ReactNode } from "react"; +import { Fragment } from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import remarkGfm from "remark-gfm"; +import remarkParse from "remark-parse"; +import { unified } from "unified"; +import type { Literal, Node, Parent } from "unist"; +import { describe, expect, it } from "vitest"; +import { + componentsAreEqual, + defaultProcessor, + extractBlocks, + indexOfDiff, + Memoizer, + nodeAffectsDocument, +} from "../src/Memoizer.ts"; + +const gfmProcessor = unified().use(remarkParse).use(remarkGfm); + +describe("Memoizer utilities", () => { + it("finds the first differing index for edits in the middle, start, and end", () => { + expect(indexOfDiff("alpha", "alpha!")).toBe(5); + expect(indexOfDiff("!alpha", "alpha")).toBe(0); + expect(indexOfDiff("alXha", "alpha")).toBe(2); + }); + + it("treats semantically equal component maps as equal", () => { + const Link = () => null; + const Paragraph = () => null; + + expect(componentsAreEqual({ a: Link, p: Paragraph }, { p: Paragraph, a: Link })).toBe(true); + expect(componentsAreEqual({ a: Link }, { a: Link, p: Paragraph })).toBe(false); + expect(componentsAreEqual({ a: Link }, { a: Paragraph })).toBe(false); + }); + + it("detects document-wide nodes even when nested", () => { + expect( + nodeAffectsDocument({ + children: [ + { + children: [ + { + children: [], + type: "paragraph", + } as Parent, + { + identifier: "ref", + label: "ref", + title: null, + type: "definition", + url: "https://example.com", + } as Node, + ], + type: "container", + } as Parent, + ], + type: "root", + } as Parent), + ).toBe(true); + + expect( + nodeAffectsDocument({ + children: [ + { + children: [], + type: "paragraph", + } as Parent, + ], + type: "root", + } as Parent), + ).toBe(false); + }); + + it("groups transformed nodes into block buckets and ignores pure whitespace text", () => { + const blocks = extractBlocks( + { + children: [ + { + children: [], + position: { + end: { column: 6, line: 1, offset: 5 }, + start: { column: 1, line: 1, offset: 0 }, + }, + type: "paragraph", + } as Node, + { + type: "text", + value: "\n\n", + } as Literal, + { + children: [], + position: { + end: { column: 5, line: 3, offset: 14 }, + start: { column: 1, line: 3, offset: 10 }, + }, + type: "paragraph", + } as Node, + ], + type: "root", + } as Parent, + [0, 10], + 0, + ); + + expect(blocks).toHaveLength(2); + expect(blocks[0]).toHaveLength(1); + expect(blocks[1]).toHaveLength(1); + }); +}); + +describe("Memoizer incremental rendering", () => { + it("returns the same array reference when content does not change", () => { + const memoizer = createMemoizer(); + + const first = memoizer.update("hello"); + const second = memoizer.update("hello"); + + expect(second).toBe(first); + }); + + it("reuses unchanged block react nodes when editing a different block", () => { + const memoizer = createMemoizer(); + const first = memoizer.update("alpha\n\nbeta"); + const second = memoizer.update("alpha!\n\nbeta"); + const third = memoizer.update("alpha!\n\nbeta!"); + + expect(second[1]).toBe(first[1]); + expect(third[0]).toBe(second[0]); + expect(third[1]).not.toBe(second[1]); + }); + + it("reprocesses reference-link consumers when a later definition changes", () => { + const memoizer = createMemoizer(); + + const before = renderNodes(memoizer.update("[ref][id]\n\n[id]: /one")); + const after = renderNodes(memoizer.update("[ref][id]\n\n[id]: /two")); + + expect(before).toContain('href="/one"'); + expect(after).toContain('href="/two"'); + }); + + it.each([ + { + content: ["alpha\n\nbeta", "alpha\nbeta", "alpha\n\nbeta\n\ngamma", "alpha\n\nbeta"], + name: "paragraphs merging and splitting", + predict: false, + processor: defaultProcessor, + }, + { + content: ["[ref][id]\n\n[id]: /one", "[ref][id]\n\n[id]: /two", "[ref][id]\n\n[id]: /three"], + name: "reference link definitions", + predict: false, + processor: defaultProcessor, + }, + { + content: [ + "| A | B |\n| --- | --- |\n| one | two |", + "| A | B |\n| --- | --- |\n| one! | two |", + "| A | B |\n| --- | --- |\n| one! | two |\n| three | four |", + ], + name: "gfm tables", + predict: false, + processor: gfmProcessor, + }, + { + content: ["- [ ] open\n- [x] done", "- [x] open\n- [x] done", "- [x] open\n- [ ] done"], + name: "gfm task lists", + predict: false, + processor: gfmProcessor, + }, + { + content: ["hello *world", "hello brave *world", "hello brave world", "hello brave `world"], + name: "prediction across mid-document edits", + predict: true, + processor: defaultProcessor, + }, + ])("matches a fresh memoizer for $name", ({ content, predict, processor }) => { + const incremental = createMemoizer(processor, predict); + + for (const step of content) { + const fresh = createMemoizer(processor, predict); + + expect(renderNodes(incremental.update(step))).toBe(renderNodes(fresh.update(step))); + } + }); + + it("keeps a stable block tied to its own react node when blocks are inserted or removed above it", () => { + const memoizer = createMemoizer(); + const first = memoizer.update("before\n\n[stable](https://example.com)"); + const second = memoizer.update("intro\n\nbefore\n\n[stable](https://example.com)"); + const third = memoizer.update("intro\n\n[stable](https://example.com)"); + + expect(second[2]).toBe(first[1]); + expect(third[1]).toBe(second[2]); + }); +}); + +function createMemoizer(processor = defaultProcessor, predict = false) { + const memoizer = new Memoizer(); + memoizer.reconfigure(processor, predict, {}); + return memoizer; +} + +function renderNodes(nodes: readonly ReactNode[]) { + return renderToStaticMarkup({nodes}); +} diff --git a/tests/prediction.test.ts b/tests/prediction.test.ts index 54208f3682d19d90365084d66a86d501eb9ded64..ac967ef68bd5138928d8a6bef22fd5be457ee5ed 100644 --- a/tests/prediction.test.ts +++ b/tests/prediction.test.ts @@ -207,10 +207,7 @@ describe.each(["stateless", "stateful"] as const)("%s processing", (mode) => { ["hello **world ", "hello **world** "], ["hello _world ", "hello _world_ "], ["hello __world ", "hello __world__ "], - ])( - "closes before trailing whitespace for %j", - (input, expected) => check(input, expected), - ); + ])("closes before trailing whitespace for %j", (input, expected) => check(input, expected)); }); describe("soft line breaks", () => { -- 2.54.0