fix(paragraph): guard listRendering destructure against null#2896
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".
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 ( |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@kendaller thanks for jumping in - the fix is right, reproduced the crash locally. one out-of-diff thing: the branch at lines 86-88 that handles non-list paragraphs doesn't cancel a scheduled re-render, so the list marker can reappear after the paragraph stops being a list. left three inline - happy to push the cancel fix as a follow-up if easier.
Addresses review feedback on superdoc-dev#2896: - Update #initList JSDoc `@param` to include `| null`, matching the no-op-on-null behavior added in the previous commit. - Add a test that mounts a ParagraphNodeView with `listRendering: null` (the constructor path, not just update()), confirming the null guards in #initList and #updateListStyles cover first-render too. - Add a test for the space→null→tab transition that verifies the separator swaps from a text node back to a span when listRendering returns with a different suffix.
|
@caio-pizzol thanks for the review! JSDoc + both tests pushed in 86da5c8 (rebased onto your main-merge). On the out-of-diff #removeList not cancelling the scheduled RAF — agreed that's a real bug but going to leave it for a follow-up PR to keep this one scoped to the null destructure. |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @kendaller! thanks for addressing last round review.
all three points landed as suggested: jsdoc now matches the nullable contract, and both the mount-null and space → null → tab recovery tests are in.
lgtm.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.43 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.45 |
|
🎉 This PR is included in template-builder v1.6.0-next.8 The release is available on GitHub release |
|
🎉 This PR is included in esign v2.3.0-next.45 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.28.0-next.8 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.8 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.6.0-next.44 |
3 participants
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.