Skip to content

Commit 5933c96

Browse files
palmer-clcaio-pizzol
palmer-cl
and
caio-pizzol
authored
fix(links): link styles should be removed when tracked change is rejected (#2465)
* fix(links): link styles should be removed when tracked change link is rejected * fix(track-changes): ensure textStyle attributes are preserved when rejecting tracked changes * refactor: use shared helper for visual marks * test: move from behavior to visual test * refactor: move hasAnyMark to test helpers Test-only utility was defined in the production documentHelpers module. Move it to @tests/helpers/helpers.js so it doesn't ship in the production bundle. --------- Co-authored-by: Caio Pizzol <caio@harbourshare.com>
1 parent 973fc2e commit 5933c96

15 files changed

Lines changed: 767 additions & 53 deletions

File tree

‎packages/layout-engine/layout-bridge/src/cache.ts‎

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import type {
1212
import { fieldAnnotationKey } from './field-annotation-key.js';
1313
import { hasTrackedChange, resolveTrackedChangesEnabled } from './tracked-changes-utils.js';
1414
import { hashParagraphBorders, hashTableBorders, hashCellBorders } from './paragraph-hash-utils.js';
15+
import { hashRunVisualMarks } from './run-visual-marks.js';
1516

1617
/**
1718
* Comment annotation structure attached to runs.
@@ -152,23 +153,7 @@ const hashRuns = (block: FlowBlock): string => {
152153
// (Fix for PR #1551: previously /\s+/g normalization caused cache collisions)
153154
const text = 'text' in run && typeof run.text === 'string' ? run.text : '';
154155

155-
// Include formatting marks that affect measurement (mirroring paragraph approach)
156-
const bold = 'bold' in run ? run.bold : false;
157-
const italic = 'italic' in run ? run.italic : false;
158-
const color = 'color' in run ? run.color : undefined;
159-
const fontSize = 'fontSize' in run ? run.fontSize : undefined;
160-
const fontFamily = 'fontFamily' in run ? run.fontFamily : undefined;
161-
const highlight = 'highlight' in run ? run.highlight : undefined;
162-
163-
// Build marks string including all formatting properties
164-
const marks = [
165-
bold ? 'b' : '',
166-
italic ? 'i' : '',
167-
color ?? '',
168-
fontSize !== undefined ? `fs:${fontSize}` : '',
169-
fontFamily ? `ff:${fontFamily}` : '',
170-
highlight ? `hl:${highlight}` : '',
171-
].join('');
156+
const marks = hashRunVisualMarks(run);
172157

173158
// Use type guard to safely access comment metadata
174159
const commentHash = hasComments(run)
@@ -291,20 +276,7 @@ const hashRuns = (block: FlowBlock): string => {
291276
// Text is used verbatim without normalization - whitespace affects measurements
292277
// (Fix for PR #1551: previously /\s+/g normalization caused cache collisions)
293278
const text = 'src' in run || run.kind === 'lineBreak' || run.kind === 'break' ? '' : (run.text ?? '');
294-
const bold = 'bold' in run ? run.bold : false;
295-
const italic = 'italic' in run ? run.italic : false;
296-
const color = 'color' in run ? run.color : undefined;
297-
const fontSize = 'fontSize' in run ? run.fontSize : undefined;
298-
const fontFamily = 'fontFamily' in run ? run.fontFamily : undefined;
299-
const highlight = 'highlight' in run ? run.highlight : undefined;
300-
const marks = [
301-
bold ? 'b' : '',
302-
italic ? 'i' : '',
303-
color ?? '',
304-
fontSize !== undefined ? `fs:${fontSize}` : '',
305-
fontFamily ? `ff:${fontFamily}` : '',
306-
highlight ? `hl:${highlight}` : '',
307-
].join('');
279+
const marks = hashRunVisualMarks(run);
308280

309281
// Include tracked change metadata in hash
310282
let trackedKey = '';

‎packages/layout-engine/layout-bridge/src/diff.ts‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import type {
2121
ParagraphFrame,
2222
} from '@superdoc/contracts';
2323
import { fieldAnnotationKey } from './field-annotation-key.js';
24+
import { hashRunVisualMarks } from './run-visual-marks.js';
2425
import { hasTrackedChange, resolveTrackedChangesEnabled } from './tracked-changes-utils.js';
2526

2627
/**
@@ -404,25 +405,23 @@ const paragraphBlocksEqual = (a: FlowBlock & { kind: 'paragraph' }, b: FlowBlock
404405
for (let i = 0; i < a.runs.length; i += 1) {
405406
const runA = a.runs[i];
406407
const runB = b.runs[i];
407-
if (
408-
('src' in runA || runA.kind === 'lineBreak' || runA.kind === 'break' || runA.kind === 'fieldAnnotation'
408+
const leftText =
409+
'src' in runA || runA.kind === 'lineBreak' || runA.kind === 'break' || runA.kind === 'fieldAnnotation'
409410
? ''
410-
: runA.text) !==
411-
('src' in runB || runB.kind === 'lineBreak' || runB.kind === 'break' || runB.kind === 'fieldAnnotation'
412-
? ''
413-
: runB.text) ||
411+
: runA.text;
412+
const rightText =
413+
'src' in runB || runB.kind === 'lineBreak' || runB.kind === 'break' || runB.kind === 'fieldAnnotation'
414+
? ''
415+
: runB.text;
416+
417+
const mismatch =
418+
leftText !== rightText ||
414419
fieldAnnotationKey(runA) !== fieldAnnotationKey(runB) ||
415-
('bold' in runA ? runA.bold : false) !== ('bold' in runB ? runB.bold : false) ||
416-
('italic' in runA ? runA.italic : false) !== ('italic' in runB ? runB.italic : false) ||
417-
('color' in runA ? runA.color : undefined) !== ('color' in runB ? runB.color : undefined) ||
418-
('fontSize' in runA ? runA.fontSize : undefined) !== ('fontSize' in runB ? runB.fontSize : undefined) ||
419-
('fontFamily' in runA ? runA.fontFamily : undefined) !== ('fontFamily' in runB ? runB.fontFamily : undefined) ||
420-
('highlight' in runA ? runA.highlight : undefined) !== ('highlight' in runB ? runB.highlight : undefined) ||
420+
hashRunVisualMarks(runA) !== hashRunVisualMarks(runB) ||
421421
getTrackedChangeKey(runA) !== getTrackedChangeKey(runB) ||
422-
getCommentKey(runA) !== getCommentKey(runB)
423-
) {
424-
return false;
425-
}
422+
getCommentKey(runA) !== getCommentKey(runB);
423+
424+
if (mismatch) return false;
426425
}
427426
return true;
428427
};

‎packages/layout-engine/layout-bridge/src/run-visual-marks.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import type { Run } from '@superdoc/contracts';
2+
3+
/**
4+
* Builds a deterministic substring for measure-cache keys and dirty-run comparison.
5+
*
6+
* Include every run property that affects text measurement or painted output but is not
7+
* covered elsewhere (e.g. tracked-change / comment keys). When adding a new visual
8+
* property, update this function only — `cache.ts` and `diff.ts` both depend on it.
9+
*
10+
* @param run - Flow run (text, tab, image, etc.); unknown fields are ignored safely.
11+
* @returns Stable string encoding bold/italic/underline/strike/color/font/highlight/link.
12+
*/
13+
export const hashRunVisualMarks = (run: Run): string => {
14+
const bold = 'bold' in run ? run.bold : false;
15+
const italic = 'italic' in run ? run.italic : false;
16+
const underline = 'underline' in run ? run.underline : undefined;
17+
const strike = 'strike' in run ? run.strike : false;
18+
const color = 'color' in run ? run.color : undefined;
19+
const fontSize = 'fontSize' in run ? run.fontSize : undefined;
20+
const fontFamily = 'fontFamily' in run ? run.fontFamily : undefined;
21+
const highlight = 'highlight' in run ? run.highlight : undefined;
22+
const link = 'link' in run ? run.link : undefined;
23+
24+
return [
25+
bold ? 'b' : '',
26+
italic ? 'i' : '',
27+
underline ? `u:${JSON.stringify(underline)}` : '',
28+
strike ? 's' : '',
29+
color ?? '',
30+
fontSize !== undefined ? `fs:${fontSize}` : '',
31+
fontFamily ? `ff:${fontFamily}` : '',
32+
highlight ? `hl:${highlight}` : '',
33+
link ? `ln:${JSON.stringify(link)}` : '',
34+
].join('');
35+
};

‎packages/layout-engine/layout-bridge/test/cache.test.ts‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -941,6 +941,103 @@ describe('MeasureCache', () => {
941941
expect(cache.get(table2, 800, 600)).toBeUndefined();
942942
});
943943

944+
it('invalidates cache when underline changes in table cells', () => {
945+
const table1: TableBlock = {
946+
kind: 'table',
947+
id: 'table-underline',
948+
rows: [
949+
{
950+
id: 'row-0',
951+
cells: [
952+
{
953+
id: 'cell-0',
954+
paragraph: {
955+
kind: 'paragraph',
956+
id: 'para-0',
957+
runs: [{ text: 'Hello', fontFamily: 'Arial', fontSize: 12, underline: { style: 'single' } }],
958+
},
959+
},
960+
],
961+
},
962+
],
963+
};
964+
965+
const table2: TableBlock = {
966+
kind: 'table',
967+
id: 'table-underline',
968+
rows: [
969+
{
970+
id: 'row-0',
971+
cells: [
972+
{
973+
id: 'cell-0',
974+
paragraph: {
975+
kind: 'paragraph',
976+
id: 'para-0',
977+
runs: [{ text: 'Hello', fontFamily: 'Arial', fontSize: 12 }],
978+
},
979+
},
980+
],
981+
},
982+
],
983+
};
984+
985+
cache.set(table1, 800, 600, { totalHeight: 50 });
986+
expect(cache.get(table2, 800, 600)).toBeUndefined();
987+
});
988+
989+
it('invalidates cache when hyperlink mark changes in table cells', () => {
990+
const table1: TableBlock = {
991+
kind: 'table',
992+
id: 'table-link',
993+
rows: [
994+
{
995+
id: 'row-0',
996+
cells: [
997+
{
998+
id: 'cell-0',
999+
paragraph: {
1000+
kind: 'paragraph',
1001+
id: 'para-0',
1002+
runs: [
1003+
{
1004+
text: 'Hello',
1005+
fontFamily: 'Arial',
1006+
fontSize: 12,
1007+
link: { href: 'https://example.com' } as any,
1008+
},
1009+
],
1010+
},
1011+
},
1012+
],
1013+
},
1014+
],
1015+
};
1016+
1017+
const table2: TableBlock = {
1018+
kind: 'table',
1019+
id: 'table-link',
1020+
rows: [
1021+
{
1022+
id: 'row-0',
1023+
cells: [
1024+
{
1025+
id: 'cell-0',
1026+
paragraph: {
1027+
kind: 'paragraph',
1028+
id: 'para-0',
1029+
runs: [{ text: 'Hello', fontFamily: 'Arial', fontSize: 12 }],
1030+
},
1031+
},
1032+
],
1033+
},
1034+
],
1035+
};
1036+
1037+
cache.set(table1, 800, 600, { totalHeight: 50 });
1038+
expect(cache.get(table2, 800, 600)).toBeUndefined();
1039+
});
1040+
9441041
it('creates cache hit when table formatting is identical', () => {
9451042
const table1: TableBlock = {
9461043
kind: 'table',

‎packages/layout-engine/layout-bridge/test/diff.test.ts‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,51 @@ describe('computeDirtyRegions', () => {
9898
expect(result.firstDirtyIndex).toBe(0);
9999
});
100100

101+
it('detects underline changes', () => {
102+
const prev = [
103+
{
104+
kind: 'paragraph' as const,
105+
id: '0-paragraph',
106+
runs: [{ text: 'Hello', fontFamily: 'Arial', fontSize: 12, underline: { style: 'single' as const } }],
107+
},
108+
];
109+
const next = [
110+
{
111+
kind: 'paragraph' as const,
112+
id: '0-paragraph',
113+
runs: [{ text: 'Hello', fontFamily: 'Arial', fontSize: 12 }],
114+
},
115+
];
116+
const result = computeDirtyRegions(prev, next);
117+
expect(result.firstDirtyIndex).toBe(0);
118+
});
119+
120+
it('detects hyperlink changes', () => {
121+
const prev = [
122+
{
123+
kind: 'paragraph' as const,
124+
id: '0-paragraph',
125+
runs: [
126+
{
127+
text: 'Hello',
128+
fontFamily: 'Arial',
129+
fontSize: 12,
130+
link: { href: 'https://example.com' } as any,
131+
},
132+
],
133+
},
134+
];
135+
const next = [
136+
{
137+
kind: 'paragraph' as const,
138+
id: '0-paragraph',
139+
runs: [{ text: 'Hello', fontFamily: 'Arial', fontSize: 12 }],
140+
},
141+
];
142+
const result = computeDirtyRegions(prev, next);
143+
expect(result.firstDirtyIndex).toBe(0);
144+
});
145+
101146
it('treats identical fontSize and fontFamily as stable', () => {
102147
const prev = [
103148
{

0 commit comments

Comments
 (0)