fix(core): widen PT → PM list run grouping to include level > 1 of any type - #1166
Conversation
…y type `portableTextToProsemirror` collected consecutive list blocks into a run by matching `current.listItem === listType`. That worked for trees where every nested item shared the parent's list type, but broke as soon as a subtree switched types — a `number` child at `level > 1` under a `bullet` parent ended the run, so the descendant subtree leaked out as its own top-level list (and any same-type sibling that came after started a new run too). For example: [bullet L1, number L2, bullet L1] emitted three sibling lists at the document root (bulletList → orderedList → bulletList) instead of one bulletList where the orderedList is nested under the first listItem. Fix: widen the run to include `level > 1` blocks regardless of `listItem` type — the recursive `convertList` / `convertListItem` inside already handles per-subtree type switches by grouping nested items by their own `listItem` and recursing with `level - 1`. Only `level === 1` with a different `listItem` from the run's anchor type still ends the run (sibling top-level lists keep their separation). Adds focused tests covering 2-level nesting, mixed-type nesting (the regression above), the still-end-on-sibling-root case, 3-level deep nesting with type switches per level, and a PT → PM → PT round-trip on a mixed-type tree.
🦋 Changeset detectedLatest commit: 442c503 The changes in this PR will be included in the next version bump. 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 |
`convertListItem`'s nested-group loop broke the run on any `listItem` type change at any depth, not just at the group's root level. For a mixed-type 3-level tree like bullet L1 → number L2 → bullet L3 → number L2 it emitted the level-3 bullet as a sibling sub-list under the level-1 item (between two level-2 ordered sub-lists) instead of nesting it under the matching level-2 number item. Round-tripping that PM tree back to portable text then degraded the L3 block to L2, permanently shrinking the hierarchy. Fix: track the shallowest level in `nestedItems` as the group's effective root. New groups only start at that root level when `listItem` switches; blocks at deeper levels fold into the current group as descendants regardless of their own type. The recursion adjusts levels (`level - 1`) so each recursive call sees its own root as level 1. Adds a regression test covering the 4-block mixed-type case above asserting both the PM tree shape and the PT round-trip identity.
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the fix and the test cases. The core logic is sound: widening the outer run-grouping to admit any level > 1 block (regardless of listItem type) correctly mirrors the recursive grouping that convertList/convertListItem already do internally. Traced through the regression case and the deeper 3-level mixed-type tree by hand — the resulting tree matches the expected nesting, and convertListItem's minLevel/anchorType rewrite is also correct (the prior (listItem || parentListType) === nestedListType check broke on type switches at any depth, even though only the outer grouping is called out in the PR description).
Tests are well-shaped: they cover the regression, preserved behavior, deep mixed nesting, and a round-trip. One minor inaccuracy in the changeset wording, and one orphan-block behavior change that's worth a brief note but probably acceptable. No blockers from me.
| "@emdash-cms/core": patch | ||
| --- | ||
|
|
||
| Fixes `portableTextToProsemirror` flattening nested lists whose subtree mixes `listItem` types. The outer run-grouping broke on the first nested type switch (e.g. an `orderedList` child under a `bulletList` parent), so an input like `[bullet L1, number L2, bullet L1]` was emitted as three separate top-level lists instead of one bullet list with a numbered sub-list under the first item. Internal `convertList`/`convertListItem` recursion was already correct — only the outer grouping needed to be widened to include `level > 1` blocks regardless of `listItem` type. |
There was a problem hiding this comment.
Minor: this changeset claims "Internal convertList/convertListItem recursion was already correct — only the outer grouping needed to be widened," but the diff also rewrites convertListItem's nested grouping (introducing minLevel/anchorType and the do/while). That rewrite is what the test case keeps deeper nesting under its true parent for mixed-type 3-level trees exercises — without it, [bullet L1, number L2, bullet L3, number L2] would still place C as a sibling sub-list under A instead of nesting under B. Worth tweaking the changeset wording so the release notes reflect both fixes.
| if (isTextBlock(current) && current.listItem === listType) { | ||
| if (!isTextBlock(current) || !current.listItem) break; | ||
| const level = current.level || 1; | ||
| if (level > 1 || current.listItem === listType) { |
There was a problem hiding this comment.
Subtle behavior change worth being aware of: for orphan sequences with no level === 1 anchor, this now absorbs mixed-type level > 1 blocks into the first run instead of starting a new run on type change. e.g. [number L3, bullet L2] used to produce two separate top-level lists; with this change it produces a single orderedList containing both items (the bullet L2's listItem type is effectively discarded by convertList's orphan branch, which calls convertListItem(item, [], parentListType)).
The input is already malformed (orphan level > 1 with no L1 anchor), so this is unlikely to matter in practice, but it's not covered by the existing "orphan level > 1 blocks ... are still rendered as root items" claim in the PR body — the rendering is preserved, but the type of the wrapping list now follows the first orphan instead of each block's own listItem. Worth either calling out explicitly or adding a test that pins down the intended behavior here.
| for (const ni of nestedItems) { | ||
| const level = ni.level || 2; | ||
| if (level < minLevel) minLevel = level; | ||
| } |
There was a problem hiding this comment.
Nit: let minLevel = Infinity; for (...) { ... } works, but since nestedItems is non-empty here (guarded by if (nestedItems.length > 0) two lines up) and every nested item has level >= 2 by construction (the outer collector at line 205 requires level > 1), Math.min(...nestedItems.map(ni => ni.level || 2)) would be equivalent and slightly more idiomatic. Not worth blocking on — the explicit loop is fine and arguably clearer about the || 2 fallback.
|
Review posted successfully. Summary of feedback:
Overall: the fix is correct, well-tested, and the logic holds up against manual trace-through of the regression cases and deeper mixed-type trees. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
the
review/awaiting-author
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped bug fix for portableTextToProsemirror list run grouping. The approach is sound: the outer loop now includes all level > 1 blocks in the same run regardless of their listItem type, and convertListItem’s nested grouping only breaks on a type mismatch at the shallowest nested level (minLevel) rather than at any depth. This prevents mixed-type subtrees from leaking out as separate top-level lists.
I read the full diff, traced the changed loops against the base version, inspected the round-trip converter (prosemirrorToPortableText), and walked through several edge-case inputs (same-level type switches, level-skipping orphans, three-level mixed-type nesting, and non-list blocks interrupting a run). The new logic is a strict improvement over the old behavior in every scenario I checked — it never introduces a mis-nesting that the old code handled correctly.
The tests are comprehensive for the regression cases (five cases covering 2-level nesting, mixed-type nesting, preserved level-1 type-switch behavior, 3-level type switches, and round-trip fidelity). The test file path mirrors the existing converter test layout. A changeset is present for the patch bump.
No AGENTS.md conventions are implicated: no SQL, no API routes, no auth changes, no user-visible strings, and no drive-by refactors.
I found no logic bugs, regressions, or convention violations. Clean fix.
github-actions
Bot
added
review/approved
2 participants
What does this PR do?
Fixes
portableTextToProsemirrorflattening nested lists whose subtree mixeslistItemtypes.packages/core/src/content/converters/portable-text-to-prosemirror.tscollected consecutive list blocks into a run by matchingcurrent.listItem === listType. That worked for trees where every nested item shared the parent's list type, but broke as soon as a subtree switched types — anumberchild atlevel > 1under abulletparent ended the run, so the descendant subtree leaked out as its own top-level list (and any same-type sibling that came after started a new run too).The internal
convertList/convertListItemrecursion was already correct — it groups nested items by their ownlistItemand recurses withlevel - 1for per-subtree type switches. The only problem was the outer grouping inportableTextToProsemirrornever letting a mixed-type subtree into the same run.Companion PR
Same root cause exists in the admin's own copy of these converters; #1142 fixes that one. This PR is the same shape applied to
packages/coreso other consumers (e.g. anyone callingportableTextToProsemirroroutside the React editor — programmatic content import, server-side preview, migration tools) get the same correctness.Repro (before this fix)
Input:
```ts
[
{ listItem: 'bullet', level: 1, ... 'Parent' },
{ listItem: 'number', level: 2, ... 'Numbered child' },
{ listItem: 'bullet', level: 1, ... 'Sibling' },
]
```
Output from
portableTextToProsemirror:```
doc
bulletList → listItem('Parent')
orderedList → listItem('Numbered child') ← leaked to doc root
bulletList → listItem('Sibling') ← new run, not a sibling of Parent
```
After this fix
```
doc
bulletList
listItem
paragraph 'Parent'
orderedList
listItem
paragraph 'Numbered child'
listItem
paragraph 'Sibling'
```
Behavior preserved
level === 1block with a differentlistItemfrom the run's anchor still ends the run, so[bullet L1, bullet L1, number L1]correctly yields two sibling top-level lists (bullet of 2 + number of 1). Test covers this explicitly.level > 1blocks with no precedinglevel === 1anchor are still rendered as root items (existingconvertListbehavior).Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been run.changeset/core-list-nesting-fix.md, patch bump for@emdash-cms/coreAI-generated code disclosure
Screenshots / test output
packages/core/tests/unit/converters/list-nesting.test.tsadds 5 cases:listItem[bullet L1, bullet L1, number L1]— still two sibling top-level lists; the run-end behavior for type-switching atlevel === 1is preservedlevelandlistItemare preserved verbatim