fix(paragraph): guard listRendering destructure against null#2896
fix(paragraph): guard listRendering destructure against null#2896kendaller wants to merge 2 commits intosuperdoc-dev:mainfrom
Conversation
ParagraphNodeView destructured `{ suffix, justification }` from
`this.node.attrs.listRendering` without a null-check in #updateListStyles,
and accessed `listRendering.markerText`/`.suffix` directly in #initList.
When a paragraph node carried `listRendering: null` — which can happen
after certain editor mutations (e.g. dispatching a transaction that
combines `setDocAttribute('bodySectPr', …)` with a paragraph delete) —
the post-transaction list-styles re-pass threw:
TypeError: Cannot destructure property 'suffix' of
'this.node.attrs.listRendering' as it is null
Use `?? {}` and optional chaining so a null value falls back through
the existing defaults (`suffix ?? 'tab'` and the `suffix == null` branch
in #createSeparator).
Adds a regression test.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 655c60a846
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let { suffix, justification } = this.node.attrs.listRendering ?? {}; | ||
| suffix = suffix ?? 'tab'; |
There was a problem hiding this comment.
When listRendering is null, this fallback forces suffix to 'tab', but update() can still execute a queued RAF callback that first calls #initList(node.attrs.listRendering) with stale data from the previous node. If that stale list had suffix: 'space' or 'nothing', #initList creates a text-node separator, and then #updateListStyles takes the forced tab path and eventually writes this.separator.style.cssText, which throws because text nodes have no style. This means the null-guarded path can still crash in mixed-suffix updates instead of safely no-oping.
Useful? React with 👍 / 👎.
Previously the null-guarded path fell back to `suffix = 'tab'` and still invoked `#calculateMarkerStyle`/`#calculateTabSeparatorStyle`. Reviewer (codex-connector) flagged that in mixed-suffix updates — where a queued RAF callback runs after a node transitions from `suffix: 'space'` to `listRendering: null` — the separator may still be a Text node. Writing `this.separator.style.cssText` on a Text node throws. Change #updateListStyles and #initList to return early when `listRendering` is null, leaving the existing marker/separator untouched. Future updates (when the node gets a real listRendering or isList() returns false) will clean up as before. Adds a regression test covering the space→null transition.
|
Good catch — addressed in 0307343. Changed the null-guarded path from "force Added a second regression test ( |
1 participant
Summary
Fixes a
TypeError: Cannot destructure property 'suffix' of 'this.node.attrs.listRendering' as it is nullthrown fromParagraphNodeView.Reproduction
When a paragraph node carries
listRendering: null(as an explicit null, not undefined) and the post-transaction list-styles re-pass runs, two call sites crash:#updateListStyles(line 235) destructures{ suffix, justification }directly.#initList(lines 286-287) accesseslistRendering.markerTextand.suffix.In my case this surfaced when dispatching a ProseMirror transaction that combined
tr.setDocAttribute('bodySectPr', …)with a paragraph delete — the re-pass walks paragraphs and blows up on the first one whoselistRenderingis null.Fix
One-line guards at each call site —
?? {}on the destructure, optional chaining on the property access. Existing defaults (suffix ?? 'tab'and thesuffix == nullbranch in#createSeparator) absorb the null case cleanly.Test plan
does not throw when listRendering is null) that calls.update()with alistRendering: nullnode and asserts no throw.pnpm vitest run src/editors/v1/extensions/paragraph/ParagraphNodeView.test.js.pnpm run lint— 0 errors (warnings are pre-existinganyusage elsewhere).pnpm run format:check— clean.