Skip to content

Commit 336958c

Browse files
harbournickharbournick
authored
fix(converter): handle null list lvlText and always clear numbering cache (#2113)
1 parent e8d1480 commit 336958c

File tree

4 files changed

+211
-47
lines changed

4 files changed

+211
-47
lines changed

‎packages/super-editor/src/extensions/paragraph/numberingPlugin.js‎

Lines changed: 50 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -208,58 +208,62 @@ export function createNumberingPlugin(editor) {
208208

209209
// Generate new list properties
210210
numberingManager.enableCache();
211-
newState.doc.descendants((node, pos) => {
212-
let resolvedProps = calculateResolvedParagraphProperties(editor, node, newState.doc.resolve(pos));
213-
if (node.type.name !== 'paragraph' || !resolvedProps.numberingProperties) {
214-
return;
215-
}
211+
try {
212+
newState.doc.descendants((node, pos) => {
213+
let resolvedProps = calculateResolvedParagraphProperties(editor, node, newState.doc.resolve(pos));
214+
if (node.type.name !== 'paragraph' || !resolvedProps.numberingProperties) {
215+
return;
216+
}
216217

217-
// Retrieving numbering definition from docx
218-
const { numId, ilvl: level = 0 } = resolvedProps.numberingProperties;
219-
const definitionDetails = ListHelpers.getListDefinitionDetails({ numId, level, editor });
218+
// Retrieving numbering definition from docx
219+
const { numId, ilvl: level = 0 } = resolvedProps.numberingProperties;
220+
const definitionDetails = ListHelpers.getListDefinitionDetails({ numId, level, editor });
220221

221-
if (!definitionDetails || Object.keys(definitionDetails).length === 0) {
222-
// Treat as normal paragraph if definition is missing
223-
tr.setNodeAttribute(pos, 'listRendering', null);
224-
bumpBlockRev(node, pos);
225-
return;
226-
}
222+
if (!definitionDetails || Object.keys(definitionDetails).length === 0) {
223+
// Treat as normal paragraph if definition is missing
224+
tr.setNodeAttribute(pos, 'listRendering', null);
225+
bumpBlockRev(node, pos);
226+
return;
227+
}
227228

228-
let { lvlText, customFormat, listNumberingType, suffix, justification, abstractId } = definitionDetails;
229-
// Defining the list marker
230-
let markerText = '';
231-
listNumberingType = listNumberingType || 'decimal';
232-
const count = numberingManager.calculateCounter(numId, level, pos, abstractId);
233-
numberingManager.setCounter(numId, level, pos, count, abstractId);
234-
const path = numberingManager.calculatePath(numId, level, pos);
235-
if (listNumberingType !== 'bullet') {
236-
markerText = generateOrderedListIndex({
237-
listLevel: path,
238-
lvlText: lvlText,
239-
listNumberingType,
240-
customFormat,
241-
});
242-
} else {
243-
markerText = docxNumberingHelpers.normalizeLvlTextChar(lvlText);
244-
}
229+
let { lvlText, customFormat, listNumberingType, suffix, justification, abstractId } = definitionDetails;
230+
// Defining the list marker
231+
let markerText = '';
232+
listNumberingType = listNumberingType || 'decimal';
233+
const count = numberingManager.calculateCounter(numId, level, pos, abstractId);
234+
numberingManager.setCounter(numId, level, pos, count, abstractId);
235+
const path = numberingManager.calculatePath(numId, level, pos);
236+
if (listNumberingType !== 'bullet') {
237+
markerText =
238+
generateOrderedListIndex({
239+
listLevel: path,
240+
lvlText: lvlText,
241+
listNumberingType,
242+
customFormat,
243+
}) ?? '';
244+
} else {
245+
markerText = docxNumberingHelpers.normalizeLvlTextChar(lvlText) ?? '';
246+
}
245247

246-
const newListRendering = {
247-
markerText,
248-
suffix,
249-
justification,
250-
path,
251-
numberingType: listNumberingType,
252-
};
248+
const newListRendering = {
249+
markerText,
250+
suffix,
251+
justification,
252+
path,
253+
numberingType: listNumberingType,
254+
};
253255

254-
if (JSON.stringify(node.attrs.listRendering) !== JSON.stringify(newListRendering)) {
255-
// Updating rendering attrs for node view usage
256-
tr.setNodeAttribute(pos, 'listRendering', newListRendering);
257-
bumpBlockRev(node, pos);
258-
}
256+
if (JSON.stringify(node.attrs.listRendering) !== JSON.stringify(newListRendering)) {
257+
// Updating rendering attrs for node view usage
258+
tr.setNodeAttribute(pos, 'listRendering', newListRendering);
259+
bumpBlockRev(node, pos);
260+
}
259261

260-
return false; // no need to descend into a paragraph
261-
});
262-
numberingManager.disableCache();
262+
return false; // no need to descend into a paragraph
263+
});
264+
} finally {
265+
numberingManager.disableCache();
266+
}
263267
return tr.docChanged ? tr : null;
264268
},
265269
});

‎packages/super-editor/src/extensions/paragraph/numberingPlugin.test.js‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -487,4 +487,111 @@ describe('numberingPlugin', () => {
487487
expect(tr.setNodeAttribute).not.toHaveBeenCalledWith(6, 'sdBlockRev', expect.anything());
488488
});
489489
});
490+
491+
describe('null lvlText crash prevention', () => {
492+
it('does not crash when ordered list has null lvlText', () => {
493+
const editor = createEditor();
494+
const plugin = createNumberingPlugin(editor);
495+
const { appendTransaction } = plugin.spec;
496+
497+
const paragraph = {
498+
type: { name: 'paragraph' },
499+
attrs: {
500+
listRendering: null,
501+
paragraphProperties: {
502+
numberingProperties: { numId: 1, ilvl: 0 },
503+
},
504+
},
505+
};
506+
507+
const doc = makeDoc([{ node: paragraph, pos: 5 }]);
508+
const tr = createTransaction();
509+
const transactions = [{ docChanged: true, getMeta: vi.fn().mockReturnValue(false) }];
510+
511+
numberingManager.calculateCounter.mockReturnValue(1);
512+
numberingManager.calculatePath.mockReturnValue([1]);
513+
generateOrderedListIndex.mockReturnValue(null);
514+
ListHelpers.getListDefinitionDetails.mockReturnValue({
515+
lvlText: null,
516+
customFormat: null,
517+
listNumberingType: 'decimal',
518+
suffix: '.',
519+
justification: 'left',
520+
abstractId: 'a1',
521+
});
522+
523+
const result = appendTransaction(transactions, {}, { doc, tr });
524+
525+
expect(tr.setNodeAttribute).toHaveBeenCalledWith(5, 'listRendering', {
526+
markerText: '',
527+
suffix: '.',
528+
justification: 'left',
529+
path: [1],
530+
numberingType: 'decimal',
531+
});
532+
expect(result).toBe(tr);
533+
});
534+
535+
it('produces empty marker for bullet list with null lvlText', () => {
536+
const editor = createEditor();
537+
const plugin = createNumberingPlugin(editor);
538+
const { appendTransaction } = plugin.spec;
539+
540+
const paragraph = {
541+
type: { name: 'paragraph' },
542+
attrs: {
543+
listRendering: null,
544+
paragraphProperties: {
545+
numberingProperties: { numId: 1, ilvl: 0 },
546+
},
547+
},
548+
};
549+
550+
const doc = makeDoc([{ node: paragraph, pos: 5 }]);
551+
const tr = createTransaction();
552+
const transactions = [{ docChanged: true, getMeta: vi.fn().mockReturnValue(false) }];
553+
554+
numberingManager.calculateCounter.mockReturnValue(1);
555+
numberingManager.calculatePath.mockReturnValue([1]);
556+
docxNumberingHelpers.normalizeLvlTextChar.mockReturnValue(undefined);
557+
ListHelpers.getListDefinitionDetails.mockReturnValue({
558+
lvlText: null,
559+
customFormat: null,
560+
listNumberingType: 'bullet',
561+
suffix: '\t',
562+
justification: 'left',
563+
abstractId: 'a1',
564+
});
565+
566+
const result = appendTransaction(transactions, {}, { doc, tr });
567+
568+
expect(tr.setNodeAttribute).toHaveBeenCalledWith(5, 'listRendering', {
569+
markerText: '',
570+
suffix: '\t',
571+
justification: 'left',
572+
path: [1],
573+
numberingType: 'bullet',
574+
});
575+
expect(result).toBe(tr);
576+
});
577+
578+
it('ensures disableCache runs even if descendants scan throws', () => {
579+
const editor = createEditor();
580+
const plugin = createNumberingPlugin(editor);
581+
const { appendTransaction } = plugin.spec;
582+
583+
const doc = {
584+
descendants: vi.fn(() => {
585+
throw new Error('simulated crash');
586+
}),
587+
resolve: vi.fn(),
588+
};
589+
const tr = createTransaction();
590+
const transactions = [{ docChanged: true, getMeta: vi.fn().mockReturnValue(false) }];
591+
592+
expect(() => appendTransaction(transactions, {}, { doc, tr })).toThrow('simulated crash');
593+
expect(numberingManager.enableCache).toHaveBeenCalled();
594+
expect(numberingManager.disableCache).toHaveBeenCalled();
595+
});
596+
});
490597
});

‎shared/common/list-numbering/index.test.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,54 @@ describe('generateOrderedListIndex', () => {
7373
});
7474
expect(result).toBeNull();
7575
});
76+
77+
describe('malformed lvlText', () => {
78+
it('returns null when lvlText is null', () => {
79+
const result = generateOrderedListIndex({
80+
listLevel: [1],
81+
lvlText: null,
82+
listNumberingType: 'decimal',
83+
});
84+
expect(result).toBeNull();
85+
});
86+
87+
it('returns null when lvlText is undefined', () => {
88+
const result = generateOrderedListIndex({
89+
listLevel: [1],
90+
lvlText: undefined,
91+
listNumberingType: 'decimal',
92+
});
93+
expect(result).toBeNull();
94+
});
95+
96+
it('returns null when lvlText is a non-string type', () => {
97+
const result = generateOrderedListIndex({
98+
listLevel: [1],
99+
lvlText: 42 as any,
100+
listNumberingType: 'decimal',
101+
});
102+
expect(result).toBeNull();
103+
});
104+
105+
it('still formats correctly with valid lvlText after guard', () => {
106+
const result = generateOrderedListIndex({
107+
listLevel: [3],
108+
lvlText: '%1.',
109+
listNumberingType: 'decimal',
110+
});
111+
expect(result).toBe('3.');
112+
});
113+
});
114+
115+
it('handles undefined customFormat for custom numbering type', () => {
116+
const result = generateOrderedListIndex({
117+
listLevel: [5],
118+
lvlText: '%1)',
119+
listNumberingType: 'custom',
120+
customFormat: undefined,
121+
});
122+
expect(result).toBe('5)');
123+
});
76124
});
77125

78126
describe('normalizeLvlTextChar', () => {

‎shared/common/list-numbering/index.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ const listIndexMap: Record<string, NumberingHandler> = {
3434

3535
export interface GenerateOrderedListIndexOptions {
3636
listLevel: number[];
37-
lvlText: string;
37+
lvlText: string | null | undefined;
3838
listNumberingType?: string;
3939
customFormat?: string;
4040
}
@@ -45,11 +45,13 @@ export const generateOrderedListIndex = ({
4545
listNumberingType,
4646
customFormat,
4747
}: GenerateOrderedListIndexOptions): string | null => {
48+
if (typeof lvlText !== 'string') return null;
4849
const handler = listIndexMap[listNumberingType as string];
4950
return handler ? handler(listLevel, lvlText, customFormat) : null;
5051
};
5152

5253
const createNumbering = (values: string[], lvlText: string): string => {
54+
if (typeof lvlText !== 'string') return '';
5355
return values.reduce<string>((acc, value, index) => {
5456
return Number(value) > 9
5557
? acc.replace(/^0/, '').replace(`%${index + 1}`, value)
@@ -75,6 +77,9 @@ const decimalZeroFormatter: NumberFormatter = (value, idx) => {
7577
};
7678

7779
const generateFromCustom = (path: number[], lvlText: string, customFormat: string): string => {
80+
if (typeof customFormat !== 'string') {
81+
return generateNumbering(path, lvlText, numberToStringFormatter);
82+
}
7883
if (customFormat.match(/(?:[0]+\d,\s){3}\.{3}/) == null) {
7984
return generateNumbering(path, lvlText, numberToStringFormatter);
8085
}

0 commit comments

Comments
 (0)