From 90e6901a6077c01da6e6be3eef6e950b77c9ba17 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 10 Jul 2026 11:11:16 -0400 Subject: [PATCH] fix(edit-history): harden diff after review Address findings from 2 review agents on the edit-history diff: - Perf: diffWords is O(n*m); cap at 2000 tokens/side and fall back to a coarse whole-block replaced diff above that, so a very large multi-edit message can't freeze the main thread. Memoize the per-row diff in DiffText. (Added a unit test for the coarse fallback.) - Perceivability (a11y/design): the added-word highlight was color-fill only, which is faint against the modal surface in the lotus themes. Add a Success.ContainerLine border + horizontal padding (so the rounded corners read as a chip) + box-decoration-break: clone for clean wrapping, so the "added" cue survives low fill contrast. - Consistency: a media/no-body edit now renders "(no text)" in diff mode too (matched the toggle-off view; was blank). - Softened the code comment's screen-reader claim (bare / aren't announced by default). Co-Authored-By: Claude Opus 4.8 --- .../features/room/message/EditHistoryModal.tsx | 13 +++++++++---- src/app/utils/textDiff.test.ts | 12 ++++++++++++ src/app/utils/textDiff.ts | 15 +++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/app/features/room/message/EditHistoryModal.tsx b/src/app/features/room/message/EditHistoryModal.tsx index f1461c7c6..425799dda 100644 --- a/src/app/features/room/message/EditHistoryModal.tsx +++ b/src/app/features/room/message/EditHistoryModal.tsx @@ -1,4 +1,4 @@ -import React, { ReactNode, useCallback, useEffect, useState } from 'react'; +import React, { ReactNode, useCallback, useEffect, useMemo, useState } from 'react'; import parse from 'html-react-parser'; import Linkify from 'linkify-react'; import FocusTrap from 'focus-trap-react'; @@ -113,10 +113,11 @@ function getVersionText(evt: MatrixEvent): string { } // Renders a word-level diff of prev -> next: added words highlighted, removed -// words struck-through. Uses semantic / so screen readers announce -// the change. Plain-text only (formatting isn't diffed — see the toggle). +// words struck-through, using semantic / elements. Plain-text only +// (formatting isn't diffed — see the "Highlight changes" toggle). function DiffText({ prev, next }: { prev: string; next: string }) { - const segments = diffWords(prev, next); + const segments = useMemo(() => diffWords(prev, next), [prev, next]); + if (segments.length === 0) return <>(no text); return ( <> {segments.map((seg, i) => { @@ -128,8 +129,12 @@ function DiffText({ prev, next }: { prev: string; next: string }) { style={{ background: color.Success.Container, color: color.Success.OnContainer, + border: `${config.borderWidth.B300} solid ${color.Success.ContainerLine}`, borderRadius: config.radii.R300, + padding: `0 ${config.space.S100}`, textDecoration: 'none', + boxDecorationBreak: 'clone', + WebkitBoxDecorationBreak: 'clone', }} > {seg.text} diff --git a/src/app/utils/textDiff.test.ts b/src/app/utils/textDiff.test.ts index 91df2fd20..ee059b311 100644 --- a/src/app/utils/textDiff.test.ts +++ b/src/app/utils/textDiff.test.ts @@ -78,6 +78,18 @@ test('diffWords does not mutate its inputs', () => { assert.equal(b, 'alpha gamma'); }); +test('very large inputs fall back to a coarse whole-block diff', () => { + const big = Array.from({ length: 5000 }, (_, i) => `w${i}`).join(' '); + const out = diffWords(big, `${big} extra`); + // Coarse fallback: one removed block + one added block (no per-word LCS). + assert.deepEqual( + out.map((s) => s.type), + ['removed', 'added'], + ); + assert.equal(oldText(out), big); + assert.equal(newText(out), `${big} extra`); +}); + test('word-level granularity (not character-level)', () => { const out = diffWords('cat', 'cats'); // "cat" and "cats" are different tokens → full removed + added, no partial. diff --git a/src/app/utils/textDiff.ts b/src/app/utils/textDiff.ts index 4479c9c3a..3eb403b42 100644 --- a/src/app/utils/textDiff.ts +++ b/src/app/utils/textDiff.ts @@ -10,6 +10,18 @@ export type DiffSegment = { // spacing/newlines are both preserved when segments are re-joined. const tokenize = (text: string): string[] => text.match(/\s+|\S+/g) ?? []; +// Above this many tokens per side the O(n*m) LCS table gets expensive/large, so +// we fall back to a coarse whole-block "replaced" diff. Message bodies this long +// are rare; the exact word diff isn't worth a multi-million-cell allocation. +const MAX_DIFF_TOKENS = 2000; + +const coarseDiff = (oldText: string, newText: string): DiffSegment[] => { + const segments: DiffSegment[] = []; + if (oldText) segments.push({ type: 'removed', text: oldText }); + if (newText) segments.push({ type: 'added', text: newText }); + return segments; +}; + /** * Word-level diff of `oldText` → `newText` via a classic LCS. Returns segments * in reading order: `equal` runs, `removed` runs (present only in old), and @@ -19,6 +31,9 @@ const tokenize = (text: string): string[] => text.match(/\s+|\S+/g) ?? []; export function diffWords(oldText: string, newText: string): DiffSegment[] { const a = tokenize(oldText); const b = tokenize(newText); + if (a.length > MAX_DIFF_TOKENS || b.length > MAX_DIFF_TOKENS) { + return coarseDiff(oldText, newText); + } const n = a.length; const m = b.length;