Skip to content

Commit 2c38373

Browse files
fix(paragraph): guard listRendering destructure against null (#2896)
* fix(paragraph): guard listRendering destructure against null 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. * fix(paragraph): no-op list style updates when listRendering is null 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. * test(paragraph): cover constructor mount and null-to-tab recovery Addresses review feedback on #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. --------- Co-authored-by: Caio Pizzol <97641911+caio-pizzol@users.noreply.github.com>
1 parent f276b34 commit 2c38373

2 files changed

Lines changed: 93 additions & 2 deletions

File tree

packages/super-editor/src/editors/v1/extensions/paragraph/ParagraphNodeView.js

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,16 @@ export class ParagraphNodeView {
232232
* @returns {boolean}
233233
*/
234234
#updateListStyles() {
235-
let { suffix, justification } = this.node.attrs.listRendering;
235+
const listRendering = this.node.attrs.listRendering;
236+
// When listRendering is null (can happen transiently during certain
237+
// transactions, e.g. after a setDocAttribute + paragraph delete), leave
238+
// the existing marker/separator untouched. Forcing a default `suffix` here
239+
// would risk writing tab-style CSS onto a text-node separator created by
240+
// a prior 'space'/'nothing' suffix and scheduled RAF pass.
241+
if (!listRendering) {
242+
return true;
243+
}
244+
let { suffix, justification } = listRendering;
236245
suffix = suffix ?? 'tab';
237246
this.#calculateMarkerStyle(justification);
238247
if (suffix === 'tab') {
@@ -280,9 +289,15 @@ export class ParagraphNodeView {
280289
}
281290

282291
/**
283-
* @param {{ markerText: string, suffix?: string }} listRendering
292+
* @param {{ markerText: string, suffix?: string } | null} listRendering
284293
*/
285294
#initList(listRendering) {
295+
// See #updateListStyles: when listRendering is null the previous marker/
296+
// separator are left in place; avoid invoking the create helpers with
297+
// undefined values.
298+
if (!listRendering) {
299+
return;
300+
}
286301
this.#createMarker(listRendering.markerText);
287302
this.#createSeparator(listRendering.suffix);
288303
}

packages/super-editor/src/editors/v1/extensions/paragraph/ParagraphNodeView.test.js

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,82 @@ describe('ParagraphNodeView', () => {
199199
expect(nodeView.separator.textContent).toBe('\u00A0');
200200
});
201201

202+
it('does not throw when listRendering is null', () => {
203+
// Regression: #updateListStyles destructured `{ suffix, justification }`
204+
// from `this.node.attrs.listRendering` without a null-check, throwing
205+
// `TypeError: Cannot destructure property 'suffix' of ... as it is null`
206+
// whenever a paragraph node carried `listRendering: null`.
207+
isList.mockReturnValue(true);
208+
const baseAttrs = createNode().attrs;
209+
const { nodeView } = mountNodeView({ attrs: { ...baseAttrs } });
210+
211+
const nextNode = createNode({
212+
attrs: {
213+
...baseAttrs,
214+
listRendering: null,
215+
},
216+
});
217+
218+
expect(() => nodeView.update(nextNode, [])).not.toThrow();
219+
});
220+
221+
it('does not try to style a text-node separator when switching to null listRendering', () => {
222+
// Regression: when transitioning from a 'space'/'nothing' suffix (which
223+
// creates a text-node separator) to `listRendering: null`, the null-guarded
224+
// path must not fall back to the 'tab' branch, since writing
225+
// `this.separator.style.cssText` on a Text node throws.
226+
isList.mockReturnValue(true);
227+
const spaceAttrs = {
228+
...createNode().attrs,
229+
listRendering: { suffix: 'space', justification: 'left', markerText: '1.' },
230+
};
231+
const { nodeView } = mountNodeView({ attrs: spaceAttrs });
232+
// The separator should be a Text node under the 'space' suffix.
233+
expect(nodeView.separator?.nodeType).toBe(Node.TEXT_NODE);
234+
const textSeparator = nodeView.separator;
235+
236+
const nullNode = createNode({
237+
attrs: { ...spaceAttrs, listRendering: null },
238+
});
239+
240+
expect(() => nodeView.update(nullNode, [])).not.toThrow();
241+
// The text-node separator must be left alone (not replaced, not styled).
242+
expect(nodeView.separator).toBe(textSeparator);
243+
});
244+
245+
it('does not throw when mounted with listRendering null', () => {
246+
// Regression: the null guards in #initList and #updateListStyles must also
247+
// cover the constructor path — mounting a paragraph whose listRendering is
248+
// already null previously threw before update() ever ran.
249+
isList.mockReturnValue(true);
250+
const nullAttrs = { ...createNode().attrs, listRendering: null };
251+
expect(() => mountNodeView({ attrs: nullAttrs })).not.toThrow();
252+
});
253+
254+
it('recovers marker/separator when listRendering returns from null to tab', () => {
255+
// Regression: the null-guarded path leaves the existing marker/separator in
256+
// place. When listRendering clears and later returns with a different suffix
257+
// (here: space → null → tab), the separator has to swap from a text node
258+
// back to a span element — #createSeparator handles this only if the
259+
// recovery path actually runs, so exercise it end-to-end.
260+
isList.mockReturnValue(true);
261+
const spaceAttrs = {
262+
...createNode().attrs,
263+
listRendering: { suffix: 'space', justification: 'left', markerText: '1.' },
264+
};
265+
const { nodeView } = mountNodeView({ attrs: spaceAttrs });
266+
267+
nodeView.update(createNode({ attrs: { ...spaceAttrs, listRendering: null } }), []);
268+
269+
const tabNode = createNode({
270+
attrs: { ...spaceAttrs, listRendering: { suffix: 'tab', justification: 'left', markerText: '2.' } },
271+
});
272+
nodeView.update(tabNode, []);
273+
274+
expect(nodeView.marker?.textContent).toBe('2.');
275+
expect(nodeView.separator?.tagName?.toLowerCase()).toBe('span');
276+
});
277+
202278
it('uses hanging indent width for right-justified tabs and skips tab helper', () => {
203279
isList.mockReturnValue(true);
204280
const attrs = {

0 commit comments

Comments
 (0)