Skip to content

Commit 8a03c46

Browse files
committed
refactor(track-changes): improve comment accuracy and deduplicate test helpers
- Update replaceAroundStep.js comment to note that the isNodeMarkupChange heuristic matches both setNodeMarkup and setBlockType (indistinguishable at the step level), with reference to SD-2191 follow-up - Extract findFirstParagraphRange() into testUtils.js, replacing 4 duplicated paragraph-finding loops in replaceAroundStep.test.js - Extract getFirstParagraphStyleId() helper in behavior test, replacing 3 duplicated page.evaluate blocks
1 parent f27557e commit 8a03c46

4 files changed

Lines changed: 50 additions & 83 deletions

File tree

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,14 @@ export const replaceAroundStep = ({
9494
return;
9595
}
9696

97-
// Detect setNodeMarkup steps: they preserve the node type and content,
98-
// only changing attributes (e.g. paragraph styleId for heading changes).
99-
// These are safe to apply — they don't alter document structure.
100-
// We also check step.insert === 1 to exclude lift() operations (insert === 0).
97+
// Detect node-markup-change steps (setNodeMarkup and setBlockType both
98+
// produce this same ReplaceAroundStep shape — they can't be distinguished
99+
// at the step level). Used here to let paragraph style changes through in
100+
// suggesting mode (e.g. Normal → Heading1 via setNodeMarkup).
101+
// step.insert === 1 excludes lift() operations (insert === 0).
102+
// Note: setBlockType is not triggered via UI in suggesting mode, but if
103+
// it were, it would also bypass tracking. SD-2191 will add proper tracked
104+
// change marks for these operations.
101105
const isNodeMarkupChange =
102106
step.structure && step.insert === 1 && step.gapFrom === step.from + 1 && step.gapTo === step.to - 1;
103107

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/replaceAroundStep.test.js

Lines changed: 5 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { replaceAroundStep } from './replaceAroundStep.js';
77
import { TrackDeleteMarkName, TrackInsertMarkName } from '../constants.js';
88
import { TrackChangesBasePluginKey } from '../plugins/trackChangesBasePlugin.js';
99
import { initTestEditor } from '@tests/helpers/helpers.js';
10-
import { findTextPos } from './testUtils.js';
10+
import { findTextPos, findFirstParagraphRange } from './testUtils.js';
1111

1212
describe('replaceAroundStep handler', () => {
1313
let editor;
@@ -53,16 +53,7 @@ describe('replaceAroundStep handler', () => {
5353
// We find the first paragraph and create a step that would "unwrap" it
5454
// by replacing the paragraph's opening and closing tokens while preserving
5555
// the content between them.
56-
let paraStart = null;
57-
let paraEnd = null;
58-
doc.forEach((node, offset) => {
59-
if (paraStart === null && node.type.name === 'paragraph') {
60-
paraStart = offset;
61-
paraEnd = offset + node.nodeSize;
62-
}
63-
});
64-
65-
if (paraStart === null) throw new Error('No paragraph found');
56+
const { paraStart, paraEnd } = findFirstParagraphRange(doc);
6657

6758
// Build a transaction with a ReplaceAroundStep.
6859
// The step unwraps the paragraph: replaces the paragraph node but keeps its inline content.
@@ -159,16 +150,7 @@ describe('replaceAroundStep handler', () => {
159150
);
160151
const state = createState(doc);
161152

162-
// Build a ReplaceAroundStep that matches setNodeMarkup: structure=true, insert=1,
163-
// gapFrom=from+1, gapTo=to-1 (wraps the same content in a new node with different attrs)
164-
let paraStart = null;
165-
let paraEnd = null;
166-
state.doc.forEach((node, offset) => {
167-
if (paraStart === null && node.type.name === 'paragraph') {
168-
paraStart = offset;
169-
paraEnd = offset + node.nodeSize;
170-
}
171-
});
153+
const { paraStart, paraEnd } = findFirstParagraphRange(state.doc);
172154

173155
const newParagraph = schema.nodes.paragraph.create({ paragraphProperties: { styleId: 'Heading1' } });
174156
const step = new ReplaceAroundStep(
@@ -211,14 +193,7 @@ describe('replaceAroundStep handler', () => {
211193
);
212194
const state = createState(doc);
213195

214-
let paraStart = null;
215-
let paraEnd = null;
216-
state.doc.forEach((node, offset) => {
217-
if (paraStart === null && node.type.name === 'paragraph') {
218-
paraStart = offset;
219-
paraEnd = offset + node.nodeSize;
220-
}
221-
});
196+
const { paraStart, paraEnd } = findFirstParagraphRange(state.doc);
222197

223198
// lift-style step: insert=0, structure=true, gap=±1
224199
const step = new ReplaceAroundStep(paraStart, paraEnd, paraStart + 1, paraEnd - 1, Slice.empty, 0, true);
@@ -255,14 +230,7 @@ describe('replaceAroundStep handler', () => {
255230
);
256231
const state = createState(doc);
257232

258-
let paraStart = null;
259-
let paraEnd = null;
260-
state.doc.forEach((node, offset) => {
261-
if (paraStart === null && node.type.name === 'paragraph') {
262-
paraStart = offset;
263-
paraEnd = offset + node.nodeSize;
264-
}
265-
});
233+
const { paraStart, paraEnd } = findFirstParagraphRange(state.doc);
266234

267235
const newParagraph = schema.nodes.paragraph.create({ paragraphProperties: { styleId: 'Heading1' } });
268236
const step = new ReplaceAroundStep(

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/testUtils.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,21 @@ export function findTextPos(docNode, exactText) {
1717
});
1818
return found;
1919
}
20+
21+
/**
22+
* Find the start and end positions of the first paragraph node in a document.
23+
* @param {import('prosemirror-model').Node} doc - Document node to search
24+
* @returns {{ paraStart: number, paraEnd: number }}
25+
*/
26+
export function findFirstParagraphRange(doc) {
27+
let paraStart = null;
28+
let paraEnd = null;
29+
doc.forEach((node, offset) => {
30+
if (paraStart === null && node.type.name === 'paragraph') {
31+
paraStart = offset;
32+
paraEnd = offset + node.nodeSize;
33+
}
34+
});
35+
if (paraStart === null) throw new Error('No paragraph found');
36+
return { paraStart, paraEnd };
37+
}

tests/behavior/tests/formatting/heading-style-suggesting-mode.spec.ts

Lines changed: 19 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,20 @@
11
import { test, expect } from '../../fixtures/superdoc.js';
2+
import type { Page } from '@playwright/test';
3+
4+
async function getFirstParagraphStyleId(page: Page): Promise<string | null> {
5+
return page.evaluate(() => {
6+
const editor = (window as any).editor;
7+
let result: string | null = null;
8+
editor.state.doc.descendants((node: any) => {
9+
if (node.type.name === 'paragraph') {
10+
result = node.attrs?.paragraphProperties?.styleId ?? null;
11+
return false;
12+
}
13+
return true;
14+
});
15+
return result;
16+
});
17+
}
218

319
test.use({ config: { toolbar: 'full', comments: 'on', trackChanges: true } });
420

@@ -19,20 +35,7 @@ test.describe('SD-2182 heading style changes in suggesting mode', () => {
1935
});
2036
await superdoc.waitForStable();
2137

22-
// Verify the paragraph now has styleId 'Heading1'
23-
const styleId = await superdoc.page.evaluate(() => {
24-
const editor = (window as any).editor;
25-
let result: string | null = null;
26-
editor.state.doc.descendants((node: any) => {
27-
if (node.type.name === 'paragraph' && node.attrs?.paragraphProperties?.styleId) {
28-
result = node.attrs.paragraphProperties.styleId;
29-
return false;
30-
}
31-
return true;
32-
});
33-
return result;
34-
});
35-
38+
const styleId = await getFirstParagraphStyleId(superdoc.page);
3639
expect(styleId).toBe('Heading1');
3740
});
3841

@@ -55,20 +58,7 @@ test.describe('SD-2182 heading style changes in suggesting mode', () => {
5558

5659
expect(result).toBe(true);
5760

58-
// Verify the paragraph now has styleId 'Heading1'
59-
const styleId = await superdoc.page.evaluate(() => {
60-
const editor = (window as any).editor;
61-
let result: string | null = null;
62-
editor.state.doc.descendants((node: any) => {
63-
if (node.type.name === 'paragraph' && node.attrs?.paragraphProperties?.styleId) {
64-
result = node.attrs.paragraphProperties.styleId;
65-
return false;
66-
}
67-
return true;
68-
});
69-
return result;
70-
});
71-
61+
const styleId = await getFirstParagraphStyleId(superdoc.page);
7262
expect(styleId).toBe('Heading1');
7363
});
7464

@@ -96,20 +86,7 @@ test.describe('SD-2182 heading style changes in suggesting mode', () => {
9686

9787
expect(result).toBe(true);
9888

99-
// Verify the paragraph no longer has Heading1 styleId
100-
const styleId = await superdoc.page.evaluate(() => {
101-
const editor = (window as any).editor;
102-
let result: string | null = null;
103-
editor.state.doc.descendants((node: any) => {
104-
if (node.type.name === 'paragraph') {
105-
result = node.attrs?.paragraphProperties?.styleId ?? null;
106-
return false;
107-
}
108-
return true;
109-
});
110-
return result;
111-
});
112-
89+
const styleId = await getFirstParagraphStyleId(superdoc.page);
11390
expect(styleId).toBeNull();
11491
});
11592
});

0 commit comments

Comments
 (0)