diff --git a/src/components/RevisionPanel.test.tsx b/src/components/RevisionPanel.test.tsx index 2390cb3..68574d0 100644 --- a/src/components/RevisionPanel.test.tsx +++ b/src/components/RevisionPanel.test.tsx @@ -2,7 +2,7 @@ import { describe, it, expect, vi } from 'vitest' import { render, screen } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { RevisionPanel } from './RevisionPanel' -import type { DocxDiffRevision as Revision, RevisionListEntry } from 'docxodus/core' +import type { DocxDiffRevision as Revision, RevisionListEntry, FormatChangeDetails } from 'docxodus/core' const mockRevisions: Revision[] = [ { @@ -45,12 +45,18 @@ describe('RevisionPanel', () => { expect(onAccept).toHaveBeenCalledWith('rev2-native-1'); }); - it('retains structural diagnostics and disables unsafe resolution', async () => { + it.each([ + { type: 'structure', family: 'cell_merge', filter: 'structural', label: 'cell merge' }, + { type: 'format', family: 'properties_change', filter: 'formatting', label: 'Formatted' }, + ] as const)('retains $type diagnostics and disables unsafe resolution', async ({ type, family, filter, label }) => { const onAccept = vi.fn(); - render(); - await userEvent.setup().selectOptions(screen.getByRole('combobox'), 'structural'); - expect(screen.getByText('cell merge')).toBeInTheDocument(); - expect(screen.getByText('The cell merge has conflicting markers.')).toBeInTheDocument(); + const revision = native({ type, family, resolutionStatus: 'ambiguous', diagnostic: { code: 'ambiguous_pair', message: 'This change has conflicting markers.' } }); + render(); + await userEvent.setup().selectOptions(screen.getByRole('combobox'), filter); + expect(screen.getByText(label)).toBeInTheDocument(); + expect(screen.getByText('This change has conflicting markers.')).toBeInTheDocument(); expect(screen.getByRole('button', { name: 'Accept' })).toBeDisabled(); expect(screen.getByRole('button', { name: 'Accept all' })).toBeDisabled(); expect(onAccept).not.toHaveBeenCalled(); @@ -61,6 +67,69 @@ describe('RevisionPanel', () => { expect(screen.getByText('No tracked changes found in this document.')).toBeInTheDocument() }) + it.each(['comparison', 'native'] as const)('shows only actual formatting differences for %s revisions', async (source) => { + const unchanged: FormatChangeDetails = { + oldProperties: { justification: 'Left', spacingAfter: '0' }, + newProperties: { justification: 'Left', spacingAfter: '0' }, + } + const changed: FormatChangeDetails = { + oldProperties: { ...unchanged.oldProperties, bold: 'false', fontSize: '20', underline: 'single' }, + newProperties: { ...unchanged.newProperties, bold: 'true', fontSize: '24', italic: 'true' }, + } + const entries = [ + { text: 'Unchanged heading', details: unchanged }, + { text: 'Changed heading', details: changed }, + ] + const revisions = entries.map(({ text, details }) => source === 'native' + ? native({ id: text, type: 'format', family: 'properties_change', text }) + : { ...mockRevisions[0], revisionType: 'FormatChanged', text, formatChange: details }) + const { container } = render( [text, details]))} + />) + + expect(Array.from(container.querySelectorAll('.rdv-format-change'), row => row.textContent)).toEqual([ + 'BoldNo→Yes', 'Font Size20→24', 'UnderlineSingle(removed)', 'ItalicYes(added)', + ]) + expect(screen.queryByText('Unchanged heading')).not.toBeInTheDocument() + expect(screen.getByText('This is inserted text')).toBeInTheDocument() + expect(screen.getByText('2 changes')).toBeInTheDocument() + expect(screen.getByRole('option', { name: 'Formatting (1)' })).toBeInTheDocument() + await userEvent.setup().selectOptions(screen.getByRole('combobox'), 'formatting') + expect(screen.getByText('Changed heading')).toBeInTheDocument() + expect(screen.queryByText('This is inserted text')).not.toBeInTheDocument() + }) + + it('keeps changes with incomplete evidence or unprintable properties', () => { + const entries: Array<{ text: string; formatChange?: FormatChangeDetails }> = [ + { text: 'No formatting evidence' }, + { text: 'Missing old snapshot', formatChange: { newProperties: {} } }, + { text: 'Missing new snapshot', formatChange: { oldProperties: {} } }, + { text: 'Empty snapshots', formatChange: { oldProperties: {}, newProperties: {} } }, + { text: 'Unmodeled run change', formatChange: { oldProperties: {}, newProperties: {}, changedPropertyNames: [] } }, + { text: 'Unmodeled change beside equal bold', formatChange: { oldProperties: { bold: 'true' }, newProperties: { bold: 'true' }, changedPropertyNames: [] } }, + { text: 'Table shell changed', formatChange: { oldProperties: {}, newProperties: {}, changedPropertyNames: ['shell'], scope: 'table' } }, + { text: 'Border XML changed', formatChange: { oldProperties: { border: '' }, newProperties: { border: '' } } }, + ] + const { container } = render( ({ + ...mockRevisions[0], revisionType: 'FormatChanged', ...entry, + }))} />) + for (const { text } of entries) expect(screen.getByText(text)).toBeInTheDocument() + expect(screen.getByText('8 changes')).toBeInTheDocument() + expect(container.querySelector('.rdv-format-change')).not.toBeInTheDocument() + }) + + it('shows the empty state when arriving evidence proves the only revision unchanged', () => { + const revision = native({ type: 'format', family: 'properties_change', text: 'Same paragraph' }) + const { rerender } = render() + expect(screen.getByText('Same paragraph')).toBeInTheDocument() + rerender() + expect(screen.getByText('No tracked changes found in this document.')).toBeInTheDocument() + expect(screen.queryByText('Same paragraph')).not.toBeInTheDocument() + }) + it('displays revision count in stats', () => { render() expect(screen.getByText('3 changes')).toBeInTheDocument() diff --git a/src/components/RevisionPanel.tsx b/src/components/RevisionPanel.tsx index 27f8567..5f0f540 100644 --- a/src/components/RevisionPanel.tsx +++ b/src/components/RevisionPanel.tsx @@ -12,7 +12,8 @@ export interface RevisionPanelProps { onReject?: (id: string) => ReturnType; onAcceptAll?: Resolution; onRejectAll?: Resolution; - /** Optional formatting evidence keyed by native revision id. Comparison records carry their own. */ + /** Formatting snapshots by native revision id. Equal snapshots without changedPropertyNames + * are treated as unchanged; keep that metadata for unmodeled changes. Comparison records carry their own. */ formatDetails?: Record; } @@ -62,7 +63,7 @@ function truncateText(text: string, maxLength: number = 150): string { } // Filter out raw XML values and clean up property names -function isValidPropertyValue(value: string): boolean { +function isValidPropertyValue(value: string | undefined): value is string { if (!value || typeof value !== 'string') return false; // Filter out raw XML data if (value.includes('<') || value.includes('xmlns') || value.includes('Unid=')) return false; @@ -99,37 +100,24 @@ interface FormatChangeItem { function getFormatChanges(details?: FormatChangeDetails): FormatChangeItem[] { if (!details) return []; const { oldProperties, newProperties } = details; - const changes: FormatChangeItem[] = []; - const processedKeys = new Set(); - - // Process old properties - if (oldProperties) { - for (const [key, value] of Object.entries(oldProperties)) { - if (!isValidPropertyValue(value)) continue; - processedKeys.add(key); - const newValue = newProperties?.[key]; - changes.push({ - property: formatPropertyName(key), - oldValue: formatPropertyValue(value), - newValue: newValue && isValidPropertyValue(newValue) ? formatPropertyValue(newValue) : undefined, - }); - } - } - - // Process new properties not in old - if (newProperties) { - for (const [key, value] of Object.entries(newProperties)) { - if (processedKeys.has(key)) continue; - if (!isValidPropertyValue(value)) continue; - changes.push({ - property: formatPropertyName(key), - oldValue: undefined, - newValue: formatPropertyValue(value), - }); - } - } + const keys = new Set([...Object.keys(oldProperties ?? {}), ...Object.keys(newProperties ?? {})]); + return [...keys].flatMap(key => { + const before = oldProperties?.[key], after = newProperties?.[key]; + if (before === after) return []; + const oldValue = isValidPropertyValue(before) ? formatPropertyValue(before) : undefined; + const newValue = isValidPropertyValue(after) ? formatPropertyValue(after) : undefined; + return oldValue || newValue ? [{ property: formatPropertyName(key), oldValue, newValue }] : []; + }); +} - return changes; +// Engine records carry changedPropertyNames, even [] for unmodeled changes. +// Only infer a no-op from nonempty, equal snapshots supplied without that metadata. +function isUnchangedFormat(details?: FormatChangeDetails): boolean { + if (!details?.oldProperties || !details.newProperties || details.changedPropertyNames !== undefined) return false; + const { oldProperties, newProperties } = details; + const keys = new Set([...Object.keys(oldProperties), ...Object.keys(newProperties)]); + return keys.size > 0 && [...keys].every(key => Object.hasOwn(oldProperties, key) && Object.hasOwn(newProperties, key) + && oldProperties[key] === newProperties[key]); } export function RevisionPanel({ revisions, onSelect, onAccept, onReject, onAcceptAll, onRejectAll, formatDetails }: RevisionPanelProps) { @@ -148,10 +136,17 @@ export function RevisionPanel({ revisions, onSelect, onAccept, onReject, onAccep // Pair each revision with its stable index in the original array so that // filtering and expansion state stay tied to the revision itself, not to a // shifting position within the filtered subset. + const displayedRevisions = useMemo(() => revisions.flatMap((revision, index) => { + const details = 'family' in revision ? formatDetails?.[revision.id] : revision.formatChange; + const hasDiagnostic = 'family' in revision && revision.resolutionStatus !== 'supported'; + if (isFormatChange(revision) && !hasDiagnostic && isUnchangedFormat(details)) return []; + const id = 'family' in revision ? revision.id : `${revision.leftAnchor ?? ''}:${revision.rightAnchor ?? ''}:${revision.revisionType}:${index}`; + return [{ revision, id, formatChanges: getFormatChanges(details) }]; + }), [revisions, formatDetails]); + const filteredRevisions = useMemo(() => { - const withIds = revisions.map((revision, index) => ({ revision, id: 'family' in revision ? revision.id : `${revision.leftAnchor ?? ''}:${revision.rightAnchor ?? ''}:${revision.revisionType}:${index}` })); - if (filter === 'all') return withIds; - return withIds.filter(({ revision }) => { + if (filter === 'all') return displayedRevisions; + return displayedRevisions.filter(({ revision }) => { switch (filter) { case 'insertions': return isInsertion(revision); case 'deletions': return isDeletion(revision); @@ -161,16 +156,19 @@ export function RevisionPanel({ revisions, onSelect, onAccept, onReject, onAccep default: return true; } }); - }, [revisions, filter]); + }, [displayedRevisions, filter]); - const stats = useMemo(() => ({ - total: revisions.length, - insertions: revisions.filter(isInsertion).length, - deletions: revisions.filter(isDeletion).length, - moves: revisions.filter(isMove).length, - formatting: revisions.filter(isFormatChange).length, - structural: revisions.filter(isStructural).length, - }), [revisions]); + const stats = useMemo(() => { + const visible = displayedRevisions.map(({ revision }) => revision); + return { + total: visible.length, + insertions: visible.filter(isInsertion).length, + deletions: visible.filter(isDeletion).length, + moves: visible.filter(isMove).length, + formatting: visible.filter(isFormatChange).length, + structural: visible.filter(isStructural).length, + }; + }, [displayedRevisions]); const toggleExpanded = (index: string) => { setExpandedIds((prev) => { @@ -184,7 +182,7 @@ export function RevisionPanel({ revisions, onSelect, onAccept, onReject, onAccep }); }; - if (revisions.length === 0) { + if (displayedRevisions.length === 0) { return (
@@ -244,10 +242,9 @@ export function RevisionPanel({ revisions, onSelect, onAccept, onReject, onAccep
- {filteredRevisions.map(({ revision, id }) => { + {filteredRevisions.map(({ revision, id, formatChanges }) => { const isExpanded = expandedIds.has(id); const needsTruncation = revision.text.length > 150; - const formatChanges = getFormatChanges('family' in revision ? formatDetails?.[revision.id] : revision.formatChange); return (