Skip to content

Reduce rendering work and reuse unchanged Portable Text blocks - #359

Open
stipsan with Copilot wants to merge 4 commits into
mainfrom
copilot/speedup-first-render
Open

stipsan with Copilot wants to merge 4 commits into
mainfrom
copilot/speedup-first-render

Conversation

Copilot AI commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Target first-render and update costs by eliminating discarded serialization and reusing unchanged blocks rather than rebuilding the entire document.

  • First-render paths

    • Serialize styled list items once.
    • Fast-path single unmarked spans, generated text, and built-in block/list-item markup.
    • Skip unused text aggregation for built-in marks; preserve custom marks’ text contract.
  • Incremental rendering

    • Add compiler-managed node boundaries with stable renderer configuration.
    • Cache marks-tree preparation independently of presentation.
    • Preserve custom dispatch, context updates, keys, and hook-free RSC rendering without cross-request caches.
  • Production benchmarks

    • Add reproducible SSR/browser benchmarks covering mounts, updates, invalidation, and hydration.
    • Synthetic Chrome 154 results:
    Operation Before After
    Nested styled-list mount 18.8 ms 1.7 ms
    Marked-content single-block edit 4.5 ms 0.2 ms
    Plain-paragraph single-block edit 1.1 ms 0.5 ms
    Plain-paragraph mount 2.8 ms 3.4 ms
  • Tradeoffs

    • Per-block bookkeeping can slow simple mounts and fully recreated documents.
    • Reuse depends on immutable block identities; generated nested-list nodes may still rebuild.

Note

Medium Risk
Rendering pipeline and incremental-update semantics changed (reuse boundaries, native default tags, missing-component callbacks on reused nodes); custom components should behave the same but edge cases around lists and warnings deserve regression testing.

Overview
Improves @portabletext/react rendering cost by restructuring the React Compiler build around per-block RenderedNode boundaries, cached marks-tree preparation, and first-render fast paths—while documenting reuse rules and adding benchmarks/tests.

The renderer is split into stable useRenderer / useNestedBlocks scopes with 'use memo' so unchanged blocks can skip re-rendering when value is structurally shared (stable _key, indices, and referentially stable components / onMissingComponent). Plain single-span blocks bypass buildMarksTree; styled list items are no longer serialized twice; default block styles and list items emit native tags instead of default wrapper components (cleaner DevTools, slightly different missing-component timing on reused output).

README and PortableTextProps.value JSDoc explain when reuse applies and tradeoffs. New Vitest suites (fast-paths, compiler-project compiled-reuse) lock behavior; pnpm bench / browser fixtures under test/performance measure SSR and client update matrices.

Reviewed by Cursor Bugbot for commit 6d6b626. Bugbot is set up for automated code reviews on this repo. Configure here.

Copilot AI and others added 3 commits October 12, 2026 09:08
…sing

Co-authored-by: stipsan <81981+stipsan@users.noreply.github.com>
…ndaries

Co-authored-by: stipsan <81981+stipsan@users.noreply.github.com>
…offs

Co-authored-by: stipsan <81981+stipsan@users.noreply.github.com>
@changeset-bot

changeset-bot Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6d6b626

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@portabletext/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@stipsan
stipsan marked this pull request as ready for review October 12, 2026 16:03
Comment thread src/react-portable-text.tsx Outdated
Comment on lines +141 to +144
function renderNode<N extends TypedObject>(
options: Serializable<N>,
children?: BlockChildren,
): ReactNode {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API regression (verified against dist/index.js): the renderNode handed to every custom component is now this 2-arity function, so anything that forwards extra arguments feeds children. [{node, index, isInline, renderNode}].map(renderNode) renders <p>A</p> on main and throws TypeError: ….map is not a function on this branch (prepared = 0 survives ?? and reaches serializeBlock). The public NodeRenderer type is still 1-arity, so TS won't catch callers doing this.

Suggestion: keep the exposed function single-argument (e.g. const renderNode: NodeRenderer = (options) => render(options) and give RenderedNode the internal render(options, prepared)), and add a regression test for .map(renderNode).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6d6b626: the public renderNode is unary and delegates to a separate internal renderer. Added a regression test using .map(renderNode) with two blocks.

Comment on lines +196 to +207
let children: ReactNode
if (node.style && node.style !== 'normal') {
// Wrap any other style in whatever the block serializer says to use
const {listItem: _listItem, ...blockNode} = node
children = renderNode({
node: blockNode,
index,
isInline: false,
renderNode,
})
children = renderNode({node: blockNode, index, isInline: false, renderNode}, prepared)
} else {
children = serializeBlock(
{node, index, isInline: false, renderNode},
components,
prepared,
).children
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is the real fix in the PR and it is bigger than the description suggests: on main a styled list item is serialized twice per nesting level, so buildMarksTree runs 2^(depth+1)−2 times. I ran the new test against main and got 126 calls for 6 nested items, and the styledLists SSR fixture goes 33 ms → 1.5 ms (22–25×) in pnpm bench. It also fires onMissingComponent once per duplicate (the "warns once" test fails on main with 2 calls).

Since this is a ~10-line change with no API or perf trade-offs, please split it into its own PR with a patch changeset so it can ship (and be bisected) independently of the memoization architecture below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single-serialization fix and nested-list call-count tests remain, and 6d6b626 adds a patch changeset describing the behavior changes. It has not been split into another PR: this session cannot open one, and the maintainer has acknowledged that limitation.

Comment thread src/react-portable-text.tsx Outdated
<Span
key={key}
text={spanToPlainText(node)}
text={builtInMarks.has(Span) ? '' : spanToPlainText(node)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This passes text="" to built-in marks purely because today's defaults happen to ignore the prop: a hidden contract on component identity that silently breaks the moment a default mark (or defaultComponents.marks.*, which is public and can be reused by consumers under another mark name) reads text, and it makes the props visible in DevTools lie.

spanToPlainText is a forEach string concat over the span's children. I could not find a measurable win: the whole marks fixture (which includes every other change in this PR) moves 4.87 → 4.55 ms SSR (−7%), and the browser newComponents gain comes from marks-tree reuse, not from this. Unless you have a profile showing this line matters, drop it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this optimization in 6d6b626. All marks receive spanToPlainText(node) again, with regression coverage inspecting the built-in mark's actual text prop through the public renderer.

Comment thread src/react-portable-text.tsx Outdated
Comment on lines +110 to +118
'use memo'
const custom = node._type in renderer.types
const listItem = 'listItem' in node && isPortableTextListItemBlock(node)
const prepare = listItem
? !(custom && node.style && node.style !== 'normal')
: !custom && isPortableTextBlock(node)
const children = useBlockChildren(prepare ? (node as PortableTextBlock) : undefined)
const {renderNode} = renderer
return renderNode({node, index, isInline, renderNode}, children)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prepare decision re-implements renderNode's dispatch order (list item before custom type, custom type before block). Two consequences:

  1. The rules now live in two places that must stay in sync; a future reorder in renderNode silently turns useBlockChildren into wasted or missing work.
  2. isPortableTextListItemBlock / isPortableTextBlock (both scan children.every(...)) run here and again inside renderNode for every block on every memo miss. That double dispatch is part of why the plain-paragraph mount / recreate scenarios regress (numbers in the summary).

Suggest moving classification into getNodeRenderer (e.g. renderer.classify(node) returning 'block' | 'listItem' | 'other' that both this component and renderNode consume). That also removes the node as PortableTextBlock cast (the repo has a TODO to enable no-unsafe-type-assertion). Also isInline is always false for RenderedNode; drop the prop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6d6b626: getNodeRenderer owns a shared, typed classify function whose result drives both preparation and rendering. The boundary caches classification, and the duplicate preparation dispatch, block cast, and always-false isInline prop are gone.

Comment on lines +307 to +310
const Tag = builtInBlocks.get(Block)
if (Tag) {
return <Tag key={key}>{props.children}</Tag>
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identity-based special-casing of the default components (here and Li === DefaultListItem at L209) means DefaultNormal, DefaultH1–H6, DefaultBlockquote and DefaultListItem are never rendered as components anymore but still ship (~1.4 KB compiled, they are reachable via defaultComponents) as identity tokens, while DefaultUnknownBlockStyle, the list components and hardBreak still go through the component path. Inconsistent and surprising to read; it also changes the component stacks React prints in warnings.

Measured benefit (headless Chrome 154, 3 runs, medians, prose / lists defaultMount): 2.20 vs 2.40 ms and 1.00 vs 1.30 ms with these two fast paths removed, i.e. ~0.2–0.3 µs per block, at the 0.1 ms timer-resolution floor. It only partially pays for the RenderedNode wrapper (main is 2.10 ms). Either drop both fast paths, or keep them with a comment saying they exist to offset the memo boundary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept both native-tag fast paths and added the requested comment explaining that they offset the block-level memo boundary overhead (6d6b626). The changeset explicitly documents their effect on DevTools and warning stacks.

Comment on lines +64 to +72
const rendered = nested.map((node, index) => (
<RenderedNode
key={node._key || `node-${index}`}
node={node}
index={index}
isInline={false}
renderer={renderer}
/>
))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse caveat that the README/JSDoc should state (they currently say "replace edited blocks and retain unchanged block references"): index is a memo dependency of RenderedNode, so any insertion or deletion shifts every following block and defeats the reuse. Verified against dist/index.js with 100 plain blocks and a counting block component:

update block renders
new array, same blocks 0
replace one block 1
append at end 1
swap two blocks 2
insert one block at top 101
delete first block 99
inline components={{…}} (what every README example does) 100

Only in-place replacement benefits; "add a paragraph above" does not. Not a bug, but the docs over-promise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented in the README and value JSDoc in 6d6b626: stable indices are required, insertions/deletions rerender shifted blocks, and new component maps defeat output reuse. Added prepend/delete-first benchmark scenarios and compiled tests for keyed index changes.

Comment on lines +93 to +102
// Separate compiler scopes keep the renderer stable when only the document changes.
// These helpers use no React hooks in the uncompiled react-server build.
function useRenderer(
overrides: PortableTextComponents | undefined,
handleMissingComponent: MissingComponentHandler,
): Renderer {
'use memo'
const components = overrides ? mergeComponents(defaultComponents, overrides) : defaultComponents
return getNodeRenderer(components, handleMissingComponent)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the directives are load-bearing: with 'use memo' removed, oxc's infer mode leaves useRenderer / useNestedBlocks / useBlockChildren uncompiled (they call no hooks, so they are not inferred) and the reuse disappears, while every test in pnpm test still passes because vitest runs the uncompiled src/. Nothing in CI exercises the compiled artifact's memoization; only the click-to-run browser page does.

Please add a CI test asserting the 0/1-render cases against the compiled build: either jsdom + react-dom/client against dist/index.js (jsdom is already in the lockfile), or a vitest project using @vitejs/plugin-react's compiler option, which vite.config.demo.ts already uses. Otherwise a dropped directive or a compiler heuristic change ships silently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a compiler-enabled Vitest project with jsdom in 6d6b626. pnpm test now checks 0/1-render updates, renderer identity, preparation reuse, invalidation, context, lists, and hydration. Verified that removing each of the four helper memo directives causes failures; the full suite passed 106 tests.

Comment thread src/react-portable-text.tsx Outdated
Comment on lines +363 to +370
const tree = prepared ?? prepareBlockChildren(node)
// Keep the one-span case allocation-light, but preserve custom toolkit text renderers.
const children =
typeof tree === 'string' && !('@text' in components.types)
? [tree]
: (typeof tree === 'string' ? buildMarksTree(node) : tree).map((child, i) =>
renderNode({node: child, isInline: true, index: i, renderNode}),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When '@text' in components.types, the string returned by the memoized prepareBlockChildren is discarded and buildMarksTree(node) runs here un-memoized on every render, so the fast-path detection becomes pure overhead for exactly those users. Either compute hasCustomText once in getNodeRenderer and skip the string path up front, or spell out the trade-off (keeping the prepared cache presentation-independent) in the comment; "allocation-light" does not explain why the string is rebuilt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6d6b626 without making preparation presentation-dependent: a prepared string now supplies the equivalent virtual @text node directly to the custom renderer. No marks tree is rebuilt. Compiled tests cover adding, repeating, switching, and removing the override with zero buildMarksTree calls.

Comment on lines +146 to +148
if (isPortableTextToolkitTextNode(node) && !hasCustomComponentForNode(node)) {
return node.text === '\n' ? renderText(node, node._key || `node-${index}`) : node.text
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this duplicates renderText's '\n' check and leaves renderText handling only the hard-break branch. Either call renderText(node, key) here or inline the HardBreak branch and delete renderText. Moving the text-node check to the top of the dispatch is a good call, though; it is the most common node type in a marks tree.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified in 6d6b626: the text dispatch calls renderText(node, key) for both ordinary text and hard breaks, removing the duplicated newline check.

Comment thread src/types.ts Outdated
Comment on lines +27 to +28
* Treat as immutable: replace edited blocks and retain unchanged block references
* to allow the compiled build to reuse their rendered output.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the contract consumers will actually read, so it should be complete: reuse also requires stable _keys, a referentially stable components object (inline components={{…}} re-renders every block), and is defeated for every block after an insertion or deletion because index changes. Either state those or link to the README section instead of restating a partial rule here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanded the value JSDoc in 6d6b626 to state all these requirements: stable block identities, _key values, components, missing-component handler, and indices. It explicitly explains shifted indices and fully recreated values.

Comment thread README.md Outdated
Comment on lines +68 to +77
Representative Linux/Chrome 154 production measurements (20 samples after 5 warmups):

| Fixture / operation | Before | After |
| ---------------------------------- | ---------------------------: | ----------------------: |
| Nested styled lists, default mount | 18.8 ms | 1.7 ms |
| Marked paragraphs, edit one block | 4.5 ms / 300 block renders | 0.2 ms / 1 block render |
| Plain paragraphs, edit one block | 1.1 ms / 1,000 block renders | 0.5 ms / 1 block render |
| Plain paragraphs, default mount | 2.8 ms | 3.4 ms |

These are synthetic results, not universal speedup guarantees. Per-block bookkeeping can cost more on simple mounts or fully recreated documents. Generated nested-list items are not all reusable. In a separate allocation-sampling run of the full browser suite, estimated allocations fell from 8.8 GB to 1.6 GB and GC events from 290 to 71, largely because deeply styled lists no longer build discarded subtrees; these totals are not retained heap size.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-ran this exact harness (headless Chrome 154, Linux, 3 runs, medians of the per-run medians) for main vs this branch and the table is missing the regressions:

fixture / scenario main PR
prose defaultMount 2.10 ms 2.20 ms
prose recreate (fresh objects, e.g. after a refetch) 1.00 ms 1.30 ms (+30%)
lists mount 1.00 ms 1.30 ms (+30%)
lists recreate 0.80 ms 1.10 ms (+38%)
prose / lists newComponents 0.80 / 0.70 ms 0.90 / 0.80 ms
prose oneEdit 0.80 ms (1000 renders) 0.40 ms (1 render)
marks newArray 3.90 ms (300) 0.20 ms (0)
styledLists mount 15.9 ms 1.4 ms

recreate is the most common update pattern for CMS content (new fetch → new objects), and mount is the only operation most consumers ever do, so these belong in the table. The absolute numbers also do not reproduce (expected, machine-dependent), which is the problem with committing them to the README: they rot and cannot be audited. The "8.8 GB → 1.6 GB allocations" claim has no script behind it.

Suggest keeping the two paragraphs of immutability guidance, moving the how-to into test/performance/README.md, and dropping the numbers (or publishing the full matrix including regressions, with the commit they were taken from).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6d6b626: removed the timing table and allocation claims from the README, moved benchmark instructions to test/performance/README.md, and explicitly documented potential mount, refetch, and new-component-map regressions. The guide asks for the full matrix and comparison commit/environment details; the harness now also measures prepending and deleting the first block.

Comment thread README.md Outdated

- `pnpm bench` measures first-render static markup from both production entry points and checks their output matches.
- `pnpm build:bench && pnpm bench:browser` serves a production-browser benchmark page. Click **Run benchmarks and regression checks**. It checks block reuse, component/handler invalidation, context, keys, and list edits, then measures mounts and updates with identical props, new arrays, one edited block, recreated values, and changed components.
- To compare an earlier build, set `PORTABLETEXT_BENCHMARK_BASELINE` to a directory containing its `index.js` and `index.react-server.js` when running `pnpm bench`. For the browser build, set `PORTABLETEXT_BENCHMARK_ENTRY` to the earlier build's absolute `index.js` path when running `pnpm build:bench`, then open the page with `?baseline`. That flag skips optimization-specific assertions, not output checks. The earlier artifacts must be able to resolve their dependencies.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha to document, or better fix in render.bench.ts: the baseline dist must resolve the same react instance as the current build. Pointing PORTABLETEXT_BENCHMARK_BASELINE at another checkout's dist/ (the obvious thing to do) crashes with TypeError: Cannot read properties of null (reading 'useMemoCache') from react/compiler-runtime; copying the directory into this repo works. "Must be able to resolve their dependencies" does not convey that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented explicitly in test/performance/README.md in 6d6b626: both builds must resolve the same React instance, including react/compiler-runtime. The guide explains the separate-checkout failure and how to place baseline artifacts under this checkout's ignored node_modules/ so they share dependency resolution.

Comment thread package.json Outdated
Comment on lines +42 to +44
"bench:browser": "vite preview test/performance --config test/performance/vite.config.ts --outDir ../../demo/dist/performance",
"build": "tsdown",
"build:bench": "pnpm build && vite build test/performance --config test/performance/vite.config.ts --outDir ../../demo/dist/performance",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--outDir ../../demo/dist/performance is outside the vite root, so vite build warns outDir … is not inside project root and will not be emptied and hashed bundles accumulate across runs; it also writes into the gh-pages demo output directory, which is surprising. A gitignored test/performance/dist with --emptyOutDir would be cleaner. Also pnpm bench runs pnpm build first, so dist/ must be writable; fine, but worth a note since the other scripts don't chain builds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6d6b626: browser builds use ignored test/performance/dist/ with --emptyOutDir; preview uses that directory, and pnpm clean removes it. The benchmark guide notes that pnpm bench builds first and requires writable dist/.

Comment thread test/fast-paths.test.tsx
Comment on lines +7 to +13
vi.mock('@portabletext/toolkit', async (importOriginal) => {
const toolkit = await importOriginal<typeof import('@portabletext/toolkit')>()
return {
...toolkit,
buildMarksTree: vi.fn(toolkit.buildMarksTree),
spanToPlainText: vi.fn(toolkit.spanToPlainText),
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming/scope: these are behavioural call-count tests for the fast paths (and good ones: 8 of the 17 fail on main, so they do lock in the fix), not performance tests; fast-paths.test.tsx or similar would be clearer. Note that none of the reuse semantics, the headline feature, are covered here or anywhere in pnpm test; they only live in the click-to-run browser page. The PR description's "unit tests locking the fast paths and reuse semantics" is only half true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the suite to test/fast-paths.test.tsx in 6d6b626. Reuse semantics now have a separate compiler-enabled test/compiled-reuse.test.tsx suite that runs under pnpm test, rather than relying on the manual browser page.

Comment thread test/rendering-performance.test.tsx Outdated
Comment on lines +178 to +183
test('empty and malformed span marks do not incorrectly take the fast path', () => {
const value = block('Text')
value.children[0]!.marks = [42]
const onMissingComponent = vi.fn()
render({value, onMissingComponent})
expect(buildMarksTree).toHaveBeenCalledTimes(1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title says "empty and malformed" but only marks: [42] is exercised, and [42] cannot reach the fast path regardless of isPortableTextSpan (marks.length === 1 already fails the check), so the guard this test claims to cover is untested. The case that matters is a non-string text (e.g. text: 42), which would throw on .includes without isPortableTextSpan. Please add that one and fix the title.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6d6b626: renamed the test to describe malformed spans and parameterized it over both malformed marks and {text: 42, marks: []}. The latter exercises the span guard before .includes.

Comment thread test/render.bench.ts Outdated
Comment on lines +26 to +33
for (const [label, path] of entries) {
const {PortableText} = (await import(
/* @vite-ignore */ pathToFileURL(path!).href
)) as typeof import('../src')
const render = () => renderToStaticMarkup(createElement(PortableText, {value}))
const html = render()
reference ??= html
expect(html).toBe(reference)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: type entries as Array<[string, string]> to drop the path! / label! assertions. More importantly, expect(html).toBe(reference) runs during collection (inside the describe callback), so a markup mismatch between builds surfaces as a suite collection error rather than a named failing test. Move it into a bench/test body or beforeAll.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6d6b626: entries is typed as Array<[string, string]>, the path/label assertions are removed, and output equality checks run in beforeAll instead of during collection.

Comment thread test/performance/main.tsx Outdated
Comment on lines +31 to +42
const block = (key: string, text: string) => {
const value = paragraph(key, text, ['annotation'])
value.markDefs = [{_type: 'label', _key: 'annotation'}]
const find = value.markDefs.find.bind(value.markDefs)
Object.defineProperty(value.markDefs, 'find', {
value: (...args: Parameters<typeof find>) => {
lookups += 1
return find(...args)
},
})
return value
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Monkey-patching markDefs.find to count buildMarksTree calls couples the harness to a toolkit implementation detail (a Map lookup in the toolkit would zero lookups and fail every assertion in this file). Count via a spied custom mark component or a Proxy over children instead.

Bigger point: checkUpdates() is the only place the reuse semantics are asserted and it needs a build, a server and a click. I ran it headless via CDP and it passes on both builds (with ?baseline for main), so it is portable; please move these assertions into a vitest file (jsdom is already in the lockfile) so CI runs them, and leave this page for timings only.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6d6b626: removed checkUpdates() and the markDefs.find monkey-patch from the browser page, leaving it for timings. Automated compiler-enabled Vitest/jsdom tests now check reuse and invalidation using custom-component spies and a direct buildMarksTree mock, with context, indices, lists, and hydration coverage.

@cursor

cursor Bot commented Oct 12, 2026

Copy link
Copy Markdown

Review: request changes

I checked out copilot/speedup-first-render (0e51a96) and ran everything locally: pnpm lint, pnpm type-check, pnpm test (90/90), pnpm build, pnpm test:next (RSC + client routes OK), pnpm bench against a main baseline, and the browser harness in headless Chrome 154 (3 runs each for main, this branch, and this branch without the native-tag fast paths). I also ran the new tests against main and probed the built dist/index.js with jsdom. Details are in the inline comments; the summary:

Blocking

  1. API regression: the public renderNode is now 2-arity; [opts].map(renderNode) works on main and throws here (comment on renderNode).
  2. The headline feature is not tested in CI. Reuse only works because of 'use memo' directives in hook-named helpers; remove them and pnpm test is still green while the feature is gone. Only the manual browser page asserts it.
  3. Missing changeset (changeset-bot already flagged). This PR changes observable behaviour and needs release notes.
  4. The perf story is incomplete. The PR/README table omits regressions in the paths most consumers hit. Headless Chrome 154, medians of 3 runs: prose recreate 1.00 → 1.30 ms (+30%), lists mount 1.00 → 1.30 ms (+30%), lists recreate 0.80 → 1.10 ms (+38%), newComponents +13%. The wins are real but conditional: 0/1 block renders on newArray/oneEdit (2–20×) only when callers hoist components and structurally share value; inserting/deleting a block invalidates everything after it via index. Whether a content-rendering library should trade mount/refetch cost for in-place-edit speed is a maintainer call, but it needs to be made with the full numbers.

Please split out

The styled-list fix in renderListItem is an exponential bug on main (2^(depth+1)−2 buildMarksTree calls; 126 for 6 items; styledLists SSR 33 ms → 1.5 ms, 22–25×). It is ~10 lines, has no trade-offs, and should ship as its own patch regardless of what happens to the memoization architecture.

SSR (pnpm bench, this branch vs main dist, same deps)

fixture compiled build react-server build
prose 1.23 → 1.23 ms (0%) 1.12 → 0.91 ms (−19%)
marks 4.87 → 4.55 ms (−7%) 4.66 → 4.59 ms (−1%)
lists 1.10 → 1.03 ms (−6%) 1.06 → 0.77 ms (−27%)
styledLists 33.1 → 1.51 ms 31.5 → 1.26 ms

On the compiled build the fast paths and the RenderedNode/memo-cache overhead cancel out for plain prose.

Bundle

dist/index.js 13,301 → 15,860 B raw (+19%), 2,894 → 3,538 B gzip (+22%); index.react-server.js 10,236 → 12,230 B (+19%), 2,508 → 3,030 B gzip (+21%). ~1.4 KB of that is default block/list-item components that are now only identity tokens.

Behaviour changes to list in the changeset

  • Built-in marks now receive text="" (identity-based; I recommend dropping this one).
  • DefaultNormal/DefaultH1–H6/DefaultBlockquote/DefaultListItem no longer appear in the component tree; RenderedNode wrappers do (visible in React warning stacks and DevTools).
  • onMissingComponent now fires once per block memo-miss instead of on every render (3 → 1 in my probe across two unrelated updates), and once instead of 2^depth times for styled list items.
  • Every top-level block and list item is now its own component boundary (also in the RSC build, where it is pure overhead).

Smaller items (inline)

Dispatch logic duplicated between RenderedNode and renderNode (double children.every scan per block); @text override path discards the prepared string and rebuilds un-memoized; renderText half-dead; isInline always false on RenderedNode; new as cast against the repo's lint TODO; bench baseline must share the react instance or it crashes; bench output written into demo/dist; test naming and a malformed-marks test that does not test its guard.

PR description

The repo has no PR template, so nothing to conform to. The description matches the diff structurally but: "reproducible browser benchmarks" are a manual click harness; "unit tests locking … reuse semantics" is not true for CI; the regressions and the behaviour changes above are not mentioned; no SSR numbers are given despite adding an SSR bench.

@stipsan

stipsan commented Oct 12, 2026

Copy link
Copy Markdown
Member

@copilot address cursors code review ^

… reuse tests

Co-authored-by: stipsan <81981+stipsan@users.noreply.github.com>

Copilot AI commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot address cursors code review ^

Addressed in 6d6b626: restored unary renderNode and built-in mark text, centralized classification, added compiler-enabled CI regressions (removing memo directives now fails tests), fixed benchmark tooling/tests, documented index/refetch tradeoffs, and added a patch changeset. Native-tag fast paths remain with the requested rationale.

106 tests, lint, types, builds, benchmarks, and Next.js RSC/SSR checks passed; CodeQL found no alerts and independent review found no significant issues.

The separate styled-list PR remains a maintainer action: I can’t open another PR from this session.

Copilot AI requested a review from stipsan October 12, 2026 16:46
@stipsan

stipsan commented Oct 12, 2026

Copy link
Copy Markdown
Member

@copilot it's fine that you can't open another PR from this session, just reply to all open PR review comments please

Copilot AI commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

@copilot it's fine that you can't open another PR from this session, just reply to all open PR review comments please

Replied to all 17 open review threads with the fixes in 6d6b626, the retained fast-path rationale, or the separate-PR limitation. No additional code changes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants