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 <ins> 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 <ins>/<del> aren't announced by default). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -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 parse from 'html-react-parser';
|
||||||
import Linkify from 'linkify-react';
|
import Linkify from 'linkify-react';
|
||||||
import FocusTrap from 'focus-trap-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
|
// Renders a word-level diff of prev -> next: added words highlighted, removed
|
||||||
// words struck-through. Uses semantic <ins>/<del> so screen readers announce
|
// words struck-through, using semantic <ins>/<del> elements. Plain-text only
|
||||||
// the change. Plain-text only (formatting isn't diffed — see the toggle).
|
// (formatting isn't diffed — see the "Highlight changes" toggle).
|
||||||
function DiffText({ prev, next }: { prev: string; next: string }) {
|
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 (
|
return (
|
||||||
<>
|
<>
|
||||||
{segments.map((seg, i) => {
|
{segments.map((seg, i) => {
|
||||||
@@ -128,8 +129,12 @@ function DiffText({ prev, next }: { prev: string; next: string }) {
|
|||||||
style={{
|
style={{
|
||||||
background: color.Success.Container,
|
background: color.Success.Container,
|
||||||
color: color.Success.OnContainer,
|
color: color.Success.OnContainer,
|
||||||
|
border: `${config.borderWidth.B300} solid ${color.Success.ContainerLine}`,
|
||||||
borderRadius: config.radii.R300,
|
borderRadius: config.radii.R300,
|
||||||
|
padding: `0 ${config.space.S100}`,
|
||||||
textDecoration: 'none',
|
textDecoration: 'none',
|
||||||
|
boxDecorationBreak: 'clone',
|
||||||
|
WebkitBoxDecorationBreak: 'clone',
|
||||||
}}
|
}}
|
||||||
>
|
>
|
||||||
{seg.text}
|
{seg.text}
|
||||||
|
|||||||
@@ -78,6 +78,18 @@ test('diffWords does not mutate its inputs', () => {
|
|||||||
assert.equal(b, 'alpha gamma');
|
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)', () => {
|
test('word-level granularity (not character-level)', () => {
|
||||||
const out = diffWords('cat', 'cats');
|
const out = diffWords('cat', 'cats');
|
||||||
// "cat" and "cats" are different tokens → full removed + added, no partial.
|
// "cat" and "cats" are different tokens → full removed + added, no partial.
|
||||||
|
|||||||
@@ -10,6 +10,18 @@ export type DiffSegment = {
|
|||||||
// spacing/newlines are both preserved when segments are re-joined.
|
// spacing/newlines are both preserved when segments are re-joined.
|
||||||
const tokenize = (text: string): string[] => text.match(/\s+|\S+/g) ?? [];
|
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
|
* Word-level diff of `oldText` → `newText` via a classic LCS. Returns segments
|
||||||
* in reading order: `equal` runs, `removed` runs (present only in old), and
|
* 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[] {
|
export function diffWords(oldText: string, newText: string): DiffSegment[] {
|
||||||
const a = tokenize(oldText);
|
const a = tokenize(oldText);
|
||||||
const b = tokenize(newText);
|
const b = tokenize(newText);
|
||||||
|
if (a.length > MAX_DIFF_TOKENS || b.length > MAX_DIFF_TOKENS) {
|
||||||
|
return coarseDiff(oldText, newText);
|
||||||
|
}
|
||||||
const n = a.length;
|
const n = a.length;
|
||||||
const m = b.length;
|
const m = b.length;
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user