Repository navigation
Conversation
…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 detectedLatest commit: 6d6b626 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| function renderNode<N extends TypedObject>( | ||
| options: Serializable<N>, | ||
| children?: BlockChildren, | ||
| ): ReactNode { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| 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 | ||
| } |
There was a problem hiding this comment.
👍 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.
There was a problem hiding this comment.
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.
| <Span | ||
| key={key} | ||
| text={spanToPlainText(node)} | ||
| text={builtInMarks.has(Span) ? '' : spanToPlainText(node)} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| '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) |
There was a problem hiding this comment.
The prepare decision re-implements renderNode's dispatch order (list item before custom type, custom type before block). Two consequences:
- The rules now live in two places that must stay in sync; a future reorder in
renderNodesilently turnsuseBlockChildreninto wasted or missing work. isPortableTextListItemBlock/isPortableTextBlock(both scanchildren.every(...)) run here and again insiderenderNodefor every block on every memo miss. That double dispatch is part of why the plain-paragraphmount/recreatescenarios 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.
There was a problem hiding this comment.
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.
| const Tag = builtInBlocks.get(Block) | ||
| if (Tag) { | ||
| return <Tag key={key}>{props.children}</Tag> | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const rendered = nested.map((node, index) => ( | ||
| <RenderedNode | ||
| key={node._key || `node-${index}`} | ||
| node={node} | ||
| index={index} | ||
| isInline={false} | ||
| renderer={renderer} | ||
| /> | ||
| )) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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}), | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if (isPortableTextToolkitTextNode(node) && !hasCustomComponentForNode(node)) { | ||
| return node.text === '\n' ? renderText(node, node._key || `node-${index}`) : node.text | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Simplified in 6d6b626: the text dispatch calls renderText(node, key) for both ordinary text and hard breaks, removing the duplicated newline check.
| * Treat as immutable: replace edited blocks and retain unchanged block references | ||
| * to allow the compiled build to reuse their rendered output. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
|
||
| - `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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| "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", |
There was a problem hiding this comment.
--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.
There was a problem hiding this comment.
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/.
| 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), | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Review: request changesI checked out Blocking
Please split outThe styled-list fix in SSR (
|
| 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/DefaultListItemno longer appear in the component tree;RenderedNodewrappers do (visible in React warning stacks and DevTools).onMissingComponentnow 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.
|
@copilot address cursors code review ^ |
… reuse tests Co-authored-by: stipsan <81981+stipsan@users.noreply.github.com>
Addressed in 6d6b626: restored unary 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 it's fine that you can't open another PR from this session, just reply to all open PR review comments please |
Target first-render and update costs by eliminating discarded serialization and reusing unchanged blocks rather than rebuilding the entire document.
First-render paths
textcontract.Incremental rendering
Production benchmarks
Tradeoffs
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/reactrendering cost by restructuring the React Compiler build around per-blockRenderedNodeboundaries, cached marks-tree preparation, and first-render fast paths—while documenting reuse rules and adding benchmarks/tests.The renderer is split into stable
useRenderer/useNestedBlocksscopes with'use memo'so unchanged blocks can skip re-rendering whenvalueis structurally shared (stable_key, indices, and referentially stablecomponents/onMissingComponent). Plain single-span blocks bypassbuildMarksTree; 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.valueJSDoc explain when reuse applies and tradeoffs. New Vitest suites (fast-paths, compiler-projectcompiled-reuse) lock behavior;pnpm bench/ browser fixtures undertest/performancemeasure 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.