fix(admin): emit nested-list level in prosemirrorToPortableText#1142
fix(admin): emit nested-list level in prosemirrorToPortableText#1142OrangeManLi wants to merge 13 commits into
Conversation
`PortableTextEditor`'s `convertList` walks `listItem` content but skips any nested `bulletList`/`orderedList` siblings of the inner paragraph, and hardcodes `level: 1` on every emitted block. As a result, Tab- indenting a bullet in the editor (which produces a valid nested ProseMirror tree) saves a flat list where every item has `level: 1`. `packages/core/src/content/converters/prosemirror-to-portable-text.ts` already has the correct recursive behavior — this change mirrors it in the admin's local copy so the editor's onChange output matches. Adds a focused unit test covering single-level, 2-level nest, mixed bullet+number nesting, and 3-level deep nesting.
🦋 Changeset detectedLatest commit: e63fc7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Heads-up on the Translation Overview check failure — I dug in and I don't think it's caused by this PR. The job dies with: ``` That constraint comes from I checked the other currently-open PRs and the same Translation Overview failure is present on #1141 and #1143, which corroborates that the lockfile drift is base-side, not introduced here. Happy to push a lockfile regeneration on this branch if maintainers prefer to fix it through here, but I figured it'd be cleaner to surface it for a |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and mirrors the core prosemirror-to-portable-text.ts converter behaviorally — I walked through both forms (core's 4-function convertList → convertListItem → convertListItemNested → convertListItem vs. this PR's collapsed single-function recursion) and they produce the same output for the documented cases.
Verified
- Top-level call sites (lines 236, 239) still invoke
convertList(content, type)with the defaultlevel = 1— no caller migration needed. ✓ _prosemirrorToPortableTextis exported (line 1805), so the test's import path resolves. ✓- Test file follows the existing
PortableTextEditor.table.test.tsconvention (.tsextension, same import style; the browser vitest env is fine since the function is pure JS with no DOM deps). ✓ - Test coverage hits the right cases: flat baseline, 2-level nest, mixed bullet/number nesting (proves
levelandlistItemare tracked independently — an aliasing bug would still pass the all-bullet tests), 3-level. ✓ - Changeset correctly scoped to
@emdash-cms/adminpatch. ✓
Out of scope, worth a follow-up — the inverse direction (convertPTList around line 743) still ignores block.level and flattens nested PT lists back into a single-level PM list on reload. The grouping loop at line 555 also stops at listItem type changes, so the mixed-type case from the new test ([bullet level=1, number level=2]) would come back as two adjacent top-level lists instead of one nested under the other. That's a pre-existing bug, not introduced here, and not what this PR claims to fix — but it means a nested list saved correctly by this fix won't fully round-trip the next time the editor opens. Core's portable-text-to-prosemirror.ts already has the level-aware grouping (its convertList/convertListItem, lines 178–260) — same mirroring exercise as this PR, in the opposite direction. Filing as a note; happy to keep this PR scoped as-is.
Translation Overview check failure is unrelated as the author noted — pnpm-lock.yaml drift on main after the workerd sandbox commit. Worth fixing on main separately.
Nice fix; small, focused, well-tested.
| } else if (child.type === "bulletList") { | ||
| blocks.push(...convertList(child.content || [], "bullet", level + 1)); | ||
| } else if (child.type === "orderedList") { | ||
| blocks.push(...convertList(child.content || [], "number", level + 1)); |
There was a problem hiding this comment.
Correctness check: this matches the core converter's behavior exactly. Core does it through convertListItem → convertListItemNested → convertListItem (one extra hop because convertListItemNested separately iterates the nested list's listItem children); collapsing those hops into a single recursive convertList here is fine because this function already iterates items expecting them to be the list's .content (i.e. a sequence of listItem nodes), which is what child.content is for a nested bulletList/orderedList. Same output, fewer functions. 👍
| it("handles three-level nesting", () => { | ||
| const pmDoc = { | ||
| type: "doc", | ||
| content: [ |
There was a problem hiding this comment.
Good case — the mixed bullet+number nest is the one that proves level and listItem are tracked independently (a bug where the nested recursion reused the parent's listItem type would still pass the all-bullet tests).
|
Review posted successfully. The PR is well-scoped: the fix correctly mirrors the core converter's behavior for nested-list serialization, has solid test coverage (single-level, 2-level, mixed bullet/number, 3-level), and includes a properly scoped changeset. I flagged one out-of-scope follow-up — the inverse |
Mirror the forward-direction fix: `convertPTList` used to flatten every list item into a single sibling array regardless of `block.level`, and `portableTextToProsemirror`'s run-grouping broke on `listItem`-type changes — so a tree like [bullet L1, number L2, bullet L1] came back into the editor as three separate top-level lists instead of one bullet list with a numbered sub-list under the first item. That made nested-list round-trips drop their hierarchy as soon as the user re-opened the document. This change: - Rewrites `convertPTList` to walk root items (`level === 1`) and group trailing `level > 1` blocks as their nested subtree, then recurse with level decremented — same shape as `@emdash-cms/core`'s `portable-text-to-prosemirror.convertList`. - Extends the outer run-grouping in `portableTextToProsemirror` to fold `level > 1` blocks into the current run regardless of their `listItem` type, so a numbered child stays nested inside a bullet parent. - Adds focused tests for PT → PM 2-level nesting, mixed-type nesting, the regression around level=2 type switches, 3-level deep nesting, and a PT → PM → PT round-trip.
|
Pushed a follow-up addressing the inverse direction: Changes in e6ccb5b:
Translation Overview should also clear now — rebased onto |
Mirrors the core companion fix. `convertPTListItem`'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 focused regression test asserting both the PM tree shape and the PT round-trip identity for the 4-block mixed-type case.
github-actions
Bot
added
the
review/needs-rereview
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@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-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
2 participants
What does this PR do?
Fixes nested-list serialization in the admin's Portable Text editor.
PortableTextEditor's localconvertList(packages/admin/src/components/PortableTextEditor.tsx) walks eachlistItem's content but only handlesparagraphchildren — it silently skips any siblingbulletList/orderedList, and emits every block withlevel: 1regardless of nesting depth.As a result, Tab-indenting a bullet in the editor (which produces a valid nested ProseMirror tree with
bulletList → listItem → [paragraph, bulletList → listItem → paragraph]) saves portable-text where every list block haslevel: 1and the nested child items are flattened — or, in the bullet-with-only-a-nested-list shape, dropped entirely.packages/core/src/content/converters/prosemirror-to-portable-text.tsalready has the correct recursive form (convertList → convertListItem → convertListItemNested) — this PR mirrors that behavior in the admin's local copy so the editor'sonChangeoutput matches.Repro (before this fix)
ProseMirror doc:
Output from
_prosemirrorToPortableText:After this fix
Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been run (auto-format workflow already pushed astyle: formatcommit on this branch).changeset/whole-buses-repair.md, patch bump for@emdash-cms/adminAI-generated code disclosure
Screenshots / test output
The added test file (
packages/admin/tests/components/PortableTextEditor.list.test.ts) covers:level: 1)level: 1thenlevel: 2)bullet level: 1thennumber level: 2)level: 1, 2, 3)Each case builds a synthetic ProseMirror doc and asserts the
level/listItem/text tuples returned by_prosemirrorToPortableText.