fix(next): sparse block and arrays diffs were not rendered correctly (#13829)

Assuming you have 3 block/array rows and you only modify the middle one
- the version view would still display row 1 and row 3:

<img width="2354" height="1224" alt="Screenshot 2025-09-16 at 16 15
22@2x"
src="https://github.com/user-attachments/assets/5f823276-fda2-4192-a7d3-482f2a2228f9"
/>

After this PR, it's now displayed correctly:

<img width="2368" height="980" alt="Screenshot 2025-09-16 at 16 15
09@2x"
src="https://github.com/user-attachments/assets/7fc5ee25-f925-4c41-b62a-9b33652e19f9"
/>

## The Fix

The generated version fields will contain holes in the `rows` array for
rows that have no changes. The fix is to simply skip rendering those
rows. We still need to keep those holes in order to maintain the correct
row indexes.

Additionally, this PR improves the naming of some legacy variables and
function arguments that I missed out on during the version view
overhaul:

> comparison => valueFrom
> version => valueTo

---
- To see the specific tasks where the Asana app for GitHub is being
used, see below:
  - https://app.asana.com/0/0/1211267837905382
This commit is contained in:
Alessio Gravili
2025-09-17 01:36:34 -07:00
committed by GitHub
parent 91e7f0c2e1
commit 433c5136fc
8 changed files with 287 additions and 155 deletions

View File

@@ -61,31 +61,31 @@ export const DiffCollapser: React.FC<Props> = ({
'DiffCollapser: field must be an array or blocks field when isIterable is true', 'DiffCollapser: field must be an array or blocks field when isIterable is true',
) )
} }
const comparisonRows = valueFrom ?? [] const valueFromRows = valueFrom ?? []
const versionRows = valueTo ?? [] const valueToRows = valueTo ?? []
if (!Array.isArray(comparisonRows) || !Array.isArray(versionRows)) { if (!Array.isArray(valueFromRows) || !Array.isArray(valueToRows)) {
throw new Error( throw new Error(
'DiffCollapser: comparison and version must be arrays when isIterable is true', 'DiffCollapser: valueFrom and valueTro must be arrays when isIterable is true',
) )
} }
changeCount = countChangedFieldsInRows({ changeCount = countChangedFieldsInRows({
comparisonRows,
config, config,
field, field,
locales, locales,
parentIsLocalized, parentIsLocalized,
versionRows, valueFromRows,
valueToRows,
}) })
} else { } else {
changeCount = countChangedFields({ changeCount = countChangedFields({
comparison: valueFrom,
config, config,
fields, fields,
locales, locales,
parentIsLocalized, parentIsLocalized,
version: valueTo, valueFrom,
valueTo,
}) })
} }

View File

@@ -384,7 +384,7 @@ const buildVersionField = ({
const fromRow = (Array.isArray(valueFrom) && valueFrom?.[i]) || {} const fromRow = (Array.isArray(valueFrom) && valueFrom?.[i]) || {}
const toRow = (Array.isArray(valueTo) && valueTo?.[i]) || {} const toRow = (Array.isArray(valueTo) && valueTo?.[i]) || {}
baseVersionField.rows[i] = buildVersionFields({ const versionFields = buildVersionFields({
clientSchemaMap, clientSchemaMap,
customDiffComponents, customDiffComponents,
entitySlug, entitySlug,
@@ -402,6 +402,10 @@ const buildVersionField = ({
versionFromSiblingData: fromRow, versionFromSiblingData: fromRow,
versionToSiblingData: toRow, versionToSiblingData: toRow,
}).versionFields }).versionFields
if (versionFields?.length) {
baseVersionField.rows[i] = versionFields
}
} }
if (!baseVersionField.rows?.length && modifiedOnly) { if (!baseVersionField.rows?.length && modifiedOnly) {
@@ -486,7 +490,7 @@ const buildVersionField = ({
} }
} }
baseVersionField.rows[i] = buildVersionFields({ const versionFields = buildVersionFields({
clientSchemaMap, clientSchemaMap,
customDiffComponents, customDiffComponents,
entitySlug, entitySlug,
@@ -504,7 +508,12 @@ const buildVersionField = ({
versionFromSiblingData: fromRow, versionFromSiblingData: fromRow,
versionToSiblingData: toRow, versionToSiblingData: toRow,
}).versionFields }).versionFields
if (versionFields?.length) {
baseVersionField.rows[i] = versionFields
}
} }
if (!baseVersionField.rows?.length && modifiedOnly) { if (!baseVersionField.rows?.length && modifiedOnly) {
return null return null
} }

View File

@@ -29,14 +29,14 @@ export const Iterable: React.FC<FieldDiffClientProps> = ({
const { selectedLocales } = useSelectedLocales() const { selectedLocales } = useSelectedLocales()
const { config } = useConfig() const { config } = useConfig()
const versionRowCount = Array.isArray(valueTo) ? valueTo.length : 0
const comparisonRowCount = Array.isArray(valueFrom) ? valueFrom.length : 0
const maxRows = Math.max(versionRowCount, comparisonRowCount)
if (!fieldIsArrayType(field) && !fieldIsBlockType(field)) { if (!fieldIsArrayType(field) && !fieldIsBlockType(field)) {
throw new Error(`Expected field to be an array or blocks type but got: ${field.type}`) throw new Error(`Expected field to be an array or blocks type but got: ${field.type}`)
} }
const valueToRowCount = Array.isArray(valueTo) ? valueTo.length : 0
const valueFromRowCount = Array.isArray(valueFrom) ? valueFrom.length : 0
const maxRows = Math.max(valueToRowCount, valueFromRowCount)
return ( return (
<div className={baseClass}> <div className={baseClass}>
<DiffCollapser <DiffCollapser
@@ -59,19 +59,25 @@ export const Iterable: React.FC<FieldDiffClientProps> = ({
> >
{maxRows > 0 && ( {maxRows > 0 && (
<div className={`${baseClass}__rows`}> <div className={`${baseClass}__rows`}>
{Array.from(Array(maxRows).keys()).map((row, i) => { {Array.from({ length: maxRows }, (_, i) => {
const versionRow = valueTo?.[i] || {} const valueToRow = valueTo?.[i] || {}
const comparisonRow = valueFrom?.[i] || {} const valueFromRow = valueFrom?.[i] || {}
const { fields, versionFields } = getFieldsForRowComparison({ const { fields, versionFields } = getFieldsForRowComparison({
baseVersionField, baseVersionField,
comparisonRow,
config, config,
field, field,
row: i, row: i,
versionRow, valueFromRow,
valueToRow,
}) })
if (!versionFields?.length) {
// Rows without a diff create "holes" in the baseVersionField.rows (=versionFields) array - this is to maintain the correct row indexes.
// It does mean that this row has no diff and should not be rendered => skip it.
return null
}
const rowNumber = String(i + 1).padStart(2, '0') const rowNumber = String(i + 1).padStart(2, '0')
const rowLabel = fieldIsArrayType(field) const rowLabel = fieldIsArrayType(field)
? `${t('general:item')} ${rowNumber}` ? `${t('general:item')} ${rowNumber}`
@@ -90,8 +96,8 @@ export const Iterable: React.FC<FieldDiffClientProps> = ({
} }
locales={selectedLocales} locales={selectedLocales}
parentIsLocalized={parentIsLocalized || field.localized} parentIsLocalized={parentIsLocalized || field.localized}
valueFrom={comparisonRow} valueFrom={valueFromRow}
valueTo={versionRow} valueTo={valueToRow}
> >
<RenderVersionFieldsToDiff versionFields={versionFields} /> <RenderVersionFieldsToDiff versionFields={versionFields} />
</DiffCollapser> </DiffCollapser>

View File

@@ -9,10 +9,10 @@ describe('countChangedFields', () => {
{ name: 'a', type: 'text' }, { name: 'a', type: 'text' },
{ name: 'b', type: 'number' }, { name: 'b', type: 'number' },
] ]
const comparison = { a: 'original', b: 123 } const valueFrom = { a: 'original', b: 123 }
const version = { a: 'original', b: 123 } const valueTo = { a: 'original', b: 123 }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(0) expect(result).toBe(0)
}) })
@@ -21,10 +21,10 @@ describe('countChangedFields', () => {
{ name: 'a', type: 'text' }, { name: 'a', type: 'text' },
{ name: 'b', type: 'number' }, { name: 'b', type: 'number' },
] ]
const comparison = { a: 'original', b: 123 } const valueFrom = { a: 'original', b: 123 }
const version = { a: 'changed', b: 123 } const valueTo = { a: 'changed', b: 123 }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(1) expect(result).toBe(1)
}) })
@@ -33,10 +33,10 @@ describe('countChangedFields', () => {
{ name: 'a', type: 'text' }, { name: 'a', type: 'text' },
{ name: 'b', type: 'number' }, { name: 'b', type: 'number' },
] ]
const comparison = {} const valueFrom = {}
const version = { a: 'new', b: 123 } const valueTo = { a: 'new', b: 123 }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -45,10 +45,10 @@ describe('countChangedFields', () => {
{ name: 'id', type: 'text' }, { name: 'id', type: 'text' },
{ name: 'a', type: 'text' }, { name: 'a', type: 'text' },
] ]
const comparison = { id: 'original', a: 'original' } const valueFrom = { id: 'original', a: 'original' }
const version = { id: 'changed', a: 'original' } const valueTo = { id: 'changed', a: 'original' }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(0) expect(result).toBe(0)
}) })
@@ -64,10 +64,10 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { a: 'original', b: 'original', c: 'original' } const valueFrom = { a: 'original', b: 'original', c: 'original' }
const version = { a: 'changed', b: 'changed', c: 'original' } const valueTo = { a: 'changed', b: 'changed', c: 'original' }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -82,10 +82,10 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { a: 'original', b: 'original', c: 'original' } const valueFrom = { a: 'original', b: 'original', c: 'original' }
const version = { a: 'changed', b: 'changed', c: 'original' } const valueTo = { a: 'changed', b: 'changed', c: 'original' }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -101,10 +101,10 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { group: { a: 'original', b: 'original', c: 'original' } } const valueFrom = { group: { a: 'original', b: 'original', c: 'original' } }
const version = { group: { a: 'changed', b: 'changed', c: 'original' } } const valueTo = { group: { a: 'changed', b: 'changed', c: 'original' } }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -124,10 +124,10 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { a: 'original', b: 'original', c: 'original' } const valueFrom = { a: 'original', b: 'original', c: 'original' }
const version = { a: 'changed', b: 'changed', c: 'original' } const valueTo = { a: 'changed', b: 'changed', c: 'original' }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -147,10 +147,10 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { namedTab: { a: 'original', b: 'original', c: 'original' } } const valueFrom = { namedTab: { a: 'original', b: 'original', c: 'original' } }
const version = { namedTab: { a: 'changed', b: 'changed', c: 'original' } } const valueTo = { namedTab: { a: 'changed', b: 'changed', c: 'original' } }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -163,10 +163,10 @@ describe('countChangedFields', () => {
admin: {}, admin: {},
}, },
] ]
const comparison = { a: 'original', b: 'original' } const valueFrom = { a: 'original', b: 'original' }
const version = { a: 'original', b: 'changed' } const valueTo = { a: 'original', b: 'changed' }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(0) expect(result).toBe(0)
}) })
@@ -191,20 +191,20 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
arrayField: [ arrayField: [
{ a: 'original', b: 'original', c: 'original' }, { a: 'original', b: 'original', c: 'original' },
{ a: 'original', b: 'original' }, { a: 'original', b: 'original' },
], ],
} }
const version = { const valueTo = {
arrayField: [ arrayField: [
{ a: 'changed', b: 'changed', c: 'original' }, { a: 'changed', b: 'changed', c: 'original' },
{ a: 'changed', b: 'changed', c: 'changed' }, { a: 'changed', b: 'changed', c: 'changed' },
], ],
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(5) expect(result).toBe(5)
}) })
@@ -235,10 +235,10 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { arrayField: [{ a: 'original', b: 'original', c: 'original' }] } const valueFrom = { arrayField: [{ a: 'original', b: 'original', c: 'original' }] }
const version = { arrayField: [{ a: 'changed', b: 'changed', c: 'original' }] } const valueTo = { arrayField: [{ a: 'changed', b: 'changed', c: 'original' }] }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -259,20 +259,20 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
blocks: [ blocks: [
{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }, { blockType: 'blockA', a: 'original', b: 'original', c: 'original' },
{ blockType: 'blockA', a: 'original', b: 'original' }, { blockType: 'blockA', a: 'original', b: 'original' },
], ],
} }
const version = { const valueTo = {
blocks: [ blocks: [
{ blockType: 'blockA', a: 'changed', b: 'changed', c: 'original' }, { blockType: 'blockA', a: 'changed', b: 'changed', c: 'original' },
{ blockType: 'blockA', a: 'changed', b: 'changed', c: 'changed' }, { blockType: 'blockA', a: 'changed', b: 'changed', c: 'changed' },
], ],
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(5) expect(result).toBe(5)
}) })
@@ -301,14 +301,14 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
blocks: [{ blockType: 'blockA', a: 'removed', b: 'original', c: 'original' }], blocks: [{ blockType: 'blockA', a: 'removed', b: 'original', c: 'original' }],
} }
const version = { const valueTo = {
blocks: [{ blockType: 'blockB', b: 'original', c: 'changed', d: 'new' }], blocks: [{ blockType: 'blockB', b: 'original', c: 'changed', d: 'new' }],
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(3) expect(result).toBe(3)
}) })
@@ -319,15 +319,15 @@ describe('countChangedFields', () => {
{ name: 'a', type: 'text', localized: true }, { name: 'a', type: 'text', localized: true },
{ name: 'b', type: 'text', localized: true }, { name: 'b', type: 'text', localized: true },
] ]
const comparison = { const valueFrom = {
a: { en: 'original', de: 'original' }, a: { en: 'original', de: 'original' },
b: { en: 'original', de: 'original' }, b: { en: 'original', de: 'original' },
} }
const version = { const valueTo = {
a: { en: 'changed', de: 'original' }, a: { en: 'changed', de: 'original' },
b: { en: 'original', de: 'original' }, b: { en: 'original', de: 'original' },
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(1) expect(result).toBe(1)
}) })
@@ -337,15 +337,15 @@ describe('countChangedFields', () => {
{ name: 'a', type: 'text', localized: true }, { name: 'a', type: 'text', localized: true },
{ name: 'b', type: 'text', localized: true }, { name: 'b', type: 'text', localized: true },
] ]
const comparison = { const valueFrom = {
a: { en: 'original', de: 'original' }, a: { en: 'original', de: 'original' },
b: { en: 'original', de: 'original' }, b: { en: 'original', de: 'original' },
} }
const version = { const valueTo = {
a: { en: 'changed', de: 'changed' }, a: { en: 'changed', de: 'changed' },
b: { en: 'original', de: 'original' }, b: { en: 'original', de: 'original' },
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -362,19 +362,19 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
group: { group: {
en: { a: 'original', b: 'original', c: 'original' }, en: { a: 'original', b: 'original', c: 'original' },
de: { a: 'original', b: 'original', c: 'original' }, de: { a: 'original', b: 'original', c: 'original' },
}, },
} }
const version = { const valueTo = {
group: { group: {
en: { a: 'changed', b: 'changed', c: 'original' }, en: { a: 'changed', b: 'changed', c: 'original' },
de: { a: 'original', b: 'changed', c: 'original' }, de: { a: 'original', b: 'changed', c: 'original' },
}, },
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(3) expect(result).toBe(3)
}) })
it('should count changed fields inside localized tabs', () => { it('should count changed fields inside localized tabs', () => {
@@ -394,19 +394,19 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
tab: { tab: {
en: { a: 'original', b: 'original', c: 'original' }, en: { a: 'original', b: 'original', c: 'original' },
de: { a: 'original', b: 'original', c: 'original' }, de: { a: 'original', b: 'original', c: 'original' },
}, },
} }
const version = { const valueTo = {
tab: { tab: {
en: { a: 'changed', b: 'changed', c: 'original' }, en: { a: 'changed', b: 'changed', c: 'original' },
de: { a: 'original', b: 'changed', c: 'original' }, de: { a: 'original', b: 'changed', c: 'original' },
}, },
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(3) expect(result).toBe(3)
}) })
@@ -432,19 +432,19 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
arrayField: { arrayField: {
en: [{ a: 'original', b: 'original', c: 'original' }], en: [{ a: 'original', b: 'original', c: 'original' }],
de: [{ a: 'original', b: 'original', c: 'original' }], de: [{ a: 'original', b: 'original', c: 'original' }],
}, },
} }
const version = { const valueTo = {
arrayField: { arrayField: {
en: [{ a: 'changed', b: 'changed', c: 'original' }], en: [{ a: 'changed', b: 'changed', c: 'original' }],
de: [{ a: 'original', b: 'changed', c: 'original' }], de: [{ a: 'original', b: 'changed', c: 'original' }],
}, },
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(3) expect(result).toBe(3)
}) })
@@ -466,19 +466,19 @@ describe('countChangedFields', () => {
], ],
}, },
] ]
const comparison = { const valueFrom = {
blocks: { blocks: {
en: [{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }], en: [{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }],
de: [{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }], de: [{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }],
}, },
} }
const version = { const valueTo = {
blocks: { blocks: {
en: [{ blockType: 'blockA', a: 'changed', b: 'changed', c: 'original' }], en: [{ blockType: 'blockA', a: 'changed', b: 'changed', c: 'original' }],
de: [{ blockType: 'blockA', a: 'original', b: 'changed', c: 'original' }], de: [{ blockType: 'blockA', a: 'original', b: 'changed', c: 'original' }],
}, },
} }
const result = countChangedFields({ comparison, fields, version, locales }) const result = countChangedFields({ valueFrom, fields, valueTo, locales })
expect(result).toBe(3) expect(result).toBe(3)
}) })
}) })
@@ -496,14 +496,14 @@ describe('countChangedFieldsInRows', () => {
], ],
} }
const comparisonRows = [{ a: 'original', b: 'original', c: 'original' }] const valueFromRows = [{ a: 'original', b: 'original', c: 'original' }]
const versionRows = [{ a: 'changed', b: 'changed', c: 'original' }] const valueToRows = [{ a: 'changed', b: 'changed', c: 'original' }]
const result = countChangedFieldsInRows({ const result = countChangedFieldsInRows({
comparisonRows, valueFromRows,
field, field,
locales: undefined, locales: undefined,
versionRows, valueToRows: valueToRows,
}) })
expect(result).toBe(2) expect(result).toBe(2)
}) })
@@ -524,14 +524,14 @@ describe('countChangedFieldsInRows', () => {
], ],
} }
const comparisonRows = [{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }] const valueFromRows = [{ blockType: 'blockA', a: 'original', b: 'original', c: 'original' }]
const versionRows = [{ blockType: 'blockA', a: 'changed', b: 'changed', c: 'original' }] const valueToRows = [{ blockType: 'blockA', a: 'changed', b: 'changed', c: 'original' }]
const result = countChangedFieldsInRows({ const result = countChangedFieldsInRows({
comparisonRows, valueFromRows,
field, field,
locales: undefined, locales: undefined,
versionRows, valueToRows,
}) })
expect(result).toBe(2) expect(result).toBe(2)
}) })

View File

@@ -6,12 +6,12 @@ import { fieldHasChanges } from './fieldHasChanges.js'
import { getFieldsForRowComparison } from './getFieldsForRowComparison.js' import { getFieldsForRowComparison } from './getFieldsForRowComparison.js'
type Args = { type Args = {
comparison: unknown
config: ClientConfig config: ClientConfig
fields: ClientField[] fields: ClientField[]
locales: string[] | undefined locales: string[] | undefined
parentIsLocalized: boolean parentIsLocalized: boolean
version: unknown valueFrom: unknown
valueTo: unknown
} }
/** /**
@@ -19,12 +19,12 @@ type Args = {
* version data for a given set of fields. * version data for a given set of fields.
*/ */
export function countChangedFields({ export function countChangedFields({
comparison,
config, config,
fields, fields,
locales, locales,
parentIsLocalized, parentIsLocalized,
version, valueFrom,
valueTo,
}: Args) { }: Args) {
let count = 0 let count = 0
@@ -41,27 +41,27 @@ export function countChangedFields({
case 'blocks': { case 'blocks': {
if (locales && fieldShouldBeLocalized({ field, parentIsLocalized })) { if (locales && fieldShouldBeLocalized({ field, parentIsLocalized })) {
locales.forEach((locale) => { locales.forEach((locale) => {
const comparisonRows = comparison?.[field.name]?.[locale] ?? [] const valueFromRows = valueFrom?.[field.name]?.[locale] ?? []
const versionRows = version?.[field.name]?.[locale] ?? [] const valueToRows = valueTo?.[field.name]?.[locale] ?? []
count += countChangedFieldsInRows({ count += countChangedFieldsInRows({
comparisonRows,
config, config,
field, field,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
versionRows, valueFromRows,
valueToRows,
}) })
}) })
} else { } else {
const comparisonRows = comparison?.[field.name] ?? [] const valueFromRows = valueFrom?.[field.name] ?? []
const versionRows = version?.[field.name] ?? [] const valueToRows = valueTo?.[field.name] ?? []
count += countChangedFieldsInRows({ count += countChangedFieldsInRows({
comparisonRows,
config, config,
field, field,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
versionRows, valueFromRows,
valueToRows,
}) })
} }
break break
@@ -87,12 +87,12 @@ export function countChangedFields({
if (locales && fieldShouldBeLocalized({ field, parentIsLocalized })) { if (locales && fieldShouldBeLocalized({ field, parentIsLocalized })) {
locales.forEach((locale) => { locales.forEach((locale) => {
if ( if (
fieldHasChanges(version?.[field.name]?.[locale], comparison?.[field.name]?.[locale]) fieldHasChanges(valueTo?.[field.name]?.[locale], valueFrom?.[field.name]?.[locale])
) { ) {
count++ count++
} }
}) })
} else if (fieldHasChanges(version?.[field.name], comparison?.[field.name])) { } else if (fieldHasChanges(valueTo?.[field.name], valueFrom?.[field.name])) {
count++ count++
} }
break break
@@ -101,12 +101,12 @@ export function countChangedFields({
case 'collapsible': case 'collapsible':
case 'row': { case 'row': {
count += countChangedFields({ count += countChangedFields({
comparison,
config, config,
fields: field.fields, fields: field.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
version, valueFrom,
valueTo,
}) })
break break
@@ -118,33 +118,33 @@ export function countChangedFields({
if (locales && fieldShouldBeLocalized({ field, parentIsLocalized })) { if (locales && fieldShouldBeLocalized({ field, parentIsLocalized })) {
locales.forEach((locale) => { locales.forEach((locale) => {
count += countChangedFields({ count += countChangedFields({
comparison: comparison?.[field.name]?.[locale],
config, config,
fields: field.fields, fields: field.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
version: version?.[field.name]?.[locale], valueFrom: valueFrom?.[field.name]?.[locale],
valueTo: valueTo?.[field.name]?.[locale],
}) })
}) })
} else { } else {
count += countChangedFields({ count += countChangedFields({
comparison: comparison?.[field.name],
config, config,
fields: field.fields, fields: field.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
version: version?.[field.name], valueFrom: valueFrom?.[field.name],
valueTo: valueTo?.[field.name],
}) })
} }
} else { } else {
// Unnamed group field: data is NOT nested under `field.name` // Unnamed group field: data is NOT nested under `field.name`
count += countChangedFields({ count += countChangedFields({
comparison,
config, config,
fields: field.fields, fields: field.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
version, valueFrom,
valueTo,
}) })
} }
break break
@@ -158,33 +158,33 @@ export function countChangedFields({
// Named localized tab // Named localized tab
locales.forEach((locale) => { locales.forEach((locale) => {
count += countChangedFields({ count += countChangedFields({
comparison: comparison?.[tab.name]?.[locale],
config, config,
fields: tab.fields, fields: tab.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || tab.localized, parentIsLocalized: parentIsLocalized || tab.localized,
version: version?.[tab.name]?.[locale], valueFrom: valueFrom?.[tab.name]?.[locale],
valueTo: valueTo?.[tab.name]?.[locale],
}) })
}) })
} else if ('name' in tab) { } else if ('name' in tab) {
// Named tab // Named tab
count += countChangedFields({ count += countChangedFields({
comparison: comparison?.[tab.name],
config, config,
fields: tab.fields, fields: tab.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || tab.localized, parentIsLocalized: parentIsLocalized || tab.localized,
version: version?.[tab.name], valueFrom: valueFrom?.[tab.name],
valueTo: valueTo?.[tab.name],
}) })
} else { } else {
// Unnamed tab // Unnamed tab
count += countChangedFields({ count += countChangedFields({
comparison,
config, config,
fields: tab.fields, fields: tab.fields,
locales, locales,
parentIsLocalized: parentIsLocalized || tab.localized, parentIsLocalized: parentIsLocalized || tab.localized,
version, valueFrom,
valueTo,
}) })
} }
}) })
@@ -208,45 +208,45 @@ export function countChangedFields({
} }
type countChangedFieldsInRowsArgs = { type countChangedFieldsInRowsArgs = {
comparisonRows: unknown[]
config: ClientConfig config: ClientConfig
field: ArrayFieldClient | BlocksFieldClient field: ArrayFieldClient | BlocksFieldClient
locales: string[] | undefined locales: string[] | undefined
parentIsLocalized: boolean parentIsLocalized: boolean
versionRows: unknown[] valueFromRows: unknown[]
valueToRows: unknown[]
} }
export function countChangedFieldsInRows({ export function countChangedFieldsInRows({
comparisonRows = [],
config, config,
field, field,
locales, locales,
parentIsLocalized, parentIsLocalized,
versionRows = [], valueFromRows = [],
valueToRows = [],
}: countChangedFieldsInRowsArgs) { }: countChangedFieldsInRowsArgs) {
let count = 0 let count = 0
let i = 0 let i = 0
while (comparisonRows[i] || versionRows[i]) { while (valueFromRows[i] || valueToRows[i]) {
const comparisonRow = comparisonRows?.[i] || {} const valueFromRow = valueFromRows?.[i] || {}
const versionRow = versionRows?.[i] || {} const valueToRow = valueToRows?.[i] || {}
const { fields: rowFields } = getFieldsForRowComparison({ const { fields: rowFields } = getFieldsForRowComparison({
baseVersionField: { type: 'text', fields: [], path: '', schemaPath: '' }, // Doesn't matter, as we don't need the versionFields output here baseVersionField: { type: 'text', fields: [], path: '', schemaPath: '' }, // Doesn't matter, as we don't need the versionFields output here
comparisonRow,
config, config,
field, field,
row: i, row: i,
versionRow, valueFromRow,
valueToRow,
}) })
count += countChangedFields({ count += countChangedFields({
comparison: comparisonRow,
config, config,
fields: rowFields, fields: rowFields,
locales, locales,
parentIsLocalized: parentIsLocalized || field.localized, parentIsLocalized: parentIsLocalized || field.localized,
version: versionRow, valueFrom: valueFromRow,
valueTo: valueToRow,
}) })
i++ i++

View File

@@ -17,10 +17,11 @@ describe('getFieldsForRowComparison', () => {
const { fields } = getFieldsForRowComparison({ const { fields } = getFieldsForRowComparison({
field, field,
versionRow: {}, valueToRow: {},
comparisonRow: {}, valueFromRow: {},
row: 0, row: 0,
baseVersionField: { fields: [] }, baseVersionField: { fields: [], path: 'items', schemaPath: 'items', type: 'array' },
config: {} as any,
}) })
expect(fields).toEqual(arrayFields) expect(fields).toEqual(arrayFields)
@@ -45,15 +46,16 @@ describe('getFieldsForRowComparison', () => {
], ],
} }
const versionRow = { blockType: 'blockA' } const valueToRow = { blockType: 'blockA' }
const comparisonRow = { blockType: 'blockA' } const valueFromRow = { blockType: 'blockA' }
const { fields } = getFieldsForRowComparison({ const { fields } = getFieldsForRowComparison({
field, field,
versionRow, valueToRow,
comparisonRow, valueFromRow,
row: 0, row: 0,
baseVersionField: { fields: [] }, baseVersionField: { fields: [], path: 'myBlocks', schemaPath: 'myBlocks', type: 'blocks' },
config: {} as any,
}) })
expect(fields).toEqual(blockAFields) expect(fields).toEqual(blockAFields)
@@ -81,15 +83,16 @@ describe('getFieldsForRowComparison', () => {
], ],
} }
const versionRow = { blockType: 'blockA' } const valueToRow = { blockType: 'blockA' }
const comparisonRow = { blockType: 'blockB' } const valueFromRow = { blockType: 'blockB' }
const { fields } = getFieldsForRowComparison({ const { fields } = getFieldsForRowComparison({
field, field,
versionRow, valueToRow,
comparisonRow, valueFromRow,
row: 0, row: 0,
baseVersionField: { fields: [] }, baseVersionField: { fields: [], path: 'myBlocks', schemaPath: 'myBlocks', type: 'blocks' },
config: {} as any,
}) })
// Should contain all unique fields from both blocks // Should contain all unique fields from both blocks

View File

@@ -18,18 +18,18 @@ import { getUniqueListBy } from 'payload/shared'
*/ */
export function getFieldsForRowComparison({ export function getFieldsForRowComparison({
baseVersionField, baseVersionField,
comparisonRow,
config, config,
field, field,
row, row,
versionRow, valueFromRow,
valueToRow,
}: { }: {
baseVersionField: BaseVersionField baseVersionField: BaseVersionField
comparisonRow: any
config: ClientConfig config: ClientConfig
field: ArrayFieldClient | BlocksFieldClient field: ArrayFieldClient | BlocksFieldClient
row: number row: number
versionRow: any valueFromRow: any
valueToRow: any
}): { fields: ClientField[]; versionFields: VersionField[] } { }): { fields: ClientField[]; versionFields: VersionField[] } {
let fields: ClientField[] = [] let fields: ClientField[] = []
let versionFields: VersionField[] = [] let versionFields: VersionField[] = []
@@ -40,12 +40,12 @@ export function getFieldsForRowComparison({
? baseVersionField.rows[row] ? baseVersionField.rows[row]
: baseVersionField.fields : baseVersionField.fields
} else if (field.type === 'blocks') { } else if (field.type === 'blocks') {
if (versionRow?.blockType === comparisonRow?.blockType) { if (valueToRow?.blockType === valueFromRow?.blockType) {
const matchedBlock: ClientBlock = const matchedBlock: ClientBlock =
config?.blocksMap?.[versionRow?.blockType] ?? config?.blocksMap?.[valueToRow?.blockType] ??
(((('blocks' in field || 'blockReferences' in field) && (((('blocks' in field || 'blockReferences' in field) &&
(field.blockReferences ?? field.blocks)?.find( (field.blockReferences ?? field.blocks)?.find(
(block) => typeof block !== 'string' && block.slug === versionRow?.blockType, (block) => typeof block !== 'string' && block.slug === valueToRow?.blockType,
)) || { )) || {
fields: [], fields: [],
}) as ClientBlock) }) as ClientBlock)
@@ -56,19 +56,19 @@ export function getFieldsForRowComparison({
: baseVersionField.fields : baseVersionField.fields
} else { } else {
const matchedVersionBlock = const matchedVersionBlock =
config?.blocksMap?.[versionRow?.blockType] ?? config?.blocksMap?.[valueToRow?.blockType] ??
(((('blocks' in field || 'blockReferences' in field) && (((('blocks' in field || 'blockReferences' in field) &&
(field.blockReferences ?? field.blocks)?.find( (field.blockReferences ?? field.blocks)?.find(
(block) => typeof block !== 'string' && block.slug === versionRow?.blockType, (block) => typeof block !== 'string' && block.slug === valueToRow?.blockType,
)) || { )) || {
fields: [], fields: [],
}) as ClientBlock) }) as ClientBlock)
const matchedComparisonBlock = const matchedComparisonBlock =
config?.blocksMap?.[comparisonRow?.blockType] ?? config?.blocksMap?.[valueFromRow?.blockType] ??
(((('blocks' in field || 'blockReferences' in field) && (((('blocks' in field || 'blockReferences' in field) &&
(field.blockReferences ?? field.blocks)?.find( (field.blockReferences ?? field.blocks)?.find(
(block) => typeof block !== 'string' && block.slug === comparisonRow?.blockType, (block) => typeof block !== 'string' && block.slug === valueFromRow?.blockType,
)) || { )) || {
fields: [], fields: [],
}) as ClientBlock) }) as ClientBlock)

View File

@@ -26,6 +26,7 @@ import type { BrowserContext, Dialog, Page } from '@playwright/test'
import { expect, test } from '@playwright/test' import { expect, test } from '@playwright/test'
import { postsCollectionSlug } from 'admin/slugs.js' import { postsCollectionSlug } from 'admin/slugs.js'
import mongoose from 'mongoose'
import path from 'path' import path from 'path'
import { wait } from 'payload/shared' import { wait } from 'payload/shared'
import { fileURLToPath } from 'url' import { fileURLToPath } from 'url'
@@ -1535,6 +1536,7 @@ describe('Versions', () => {
let versionID: string let versionID: string
let oldVersionID: string let oldVersionID: string
let diffID: string let diffID: string
let diffDoc: Diff
let versionDiffID: string let versionDiffID: string
beforeAll(() => { beforeAll(() => {
@@ -1583,7 +1585,7 @@ describe('Versions', () => {
versionID = versions.docs[0].id versionID = versions.docs[0].id
oldVersionID = versions.docs[1].id oldVersionID = versions.docs[1].id
const diffDoc = ( diffDoc = (
await payload.find({ await payload.find({
collection: diffCollectionSlug, collection: diffCollectionSlug,
depth: 0, depth: 0,
@@ -1613,8 +1615,8 @@ describe('Versions', () => {
await expect(page.locator('.render-field-diffs').first()).toBeVisible() await expect(page.locator('.render-field-diffs').first()).toBeVisible()
} }
async function navigateToDiffVersionView() { async function navigateToDiffVersionView(versionID?: string) {
const versionURL = `${serverURL}/admin/collections/${diffCollectionSlug}/${diffID}/versions/${versionDiffID}` const versionURL = `${serverURL}/admin/collections/${diffCollectionSlug}/${diffID}/versions/${versionID ?? versionDiffID}`
await page.goto(versionURL) await page.goto(versionURL)
await expect(page.locator('.render-field-diffs').first()).toBeVisible() await expect(page.locator('.render-field-diffs').first()).toBeVisible()
} }
@@ -2084,6 +2086,118 @@ describe('Versions', () => {
}) })
await Promise.all(checkPromises) await Promise.all(checkPromises)
}) })
test('diff is displayed correctly when editing 2nd block in a blocks field with 3 blocks', async () => {
await payload.update({
collection: 'diff',
data: {
blocks: [
...diffDoc!.blocks!.map((block, i) => {
if (i === 1) {
return {
...block,
textInRowInCollapsibleBlock: 'textInRowInCollapsibleBlock3',
}
}
return block
}),
],
},
id: diffID,
})
const latestVersionDiff = (
await payload.findVersions({
collection: diffCollectionSlug,
depth: 0,
limit: 1,
where: {
parent: { equals: diffID },
},
})
).docs[0] as Diff
await navigateToDiffVersionView(latestVersionDiff.id)
const blocks = page.locator('[data-field-path="blocks"]')
await expect(blocks.locator('.iterable-diff__label')).toHaveCount(1)
await expect(blocks.locator('.iterable-diff__label')).toHaveText('Block 02')
const blockDiff = page.locator('[data-field-path="blocks.1.textInRowInCollapsibleBlock"]')
await expect(blockDiff.locator('.html-diff__diff-old')).toHaveText(
'textInRowInCollapsibleBlock2',
)
await expect(blockDiff.locator('.html-diff__diff-new')).toHaveText(
'textInRowInCollapsibleBlock3',
)
})
test('diff is displayed correctly when editing 2nd array in a arrays field with 3 arrays', async () => {
const newArray = [
{
id: new mongoose.Types.ObjectId().toHexString(),
textInArray: 'textInArray1',
},
{
id: new mongoose.Types.ObjectId().toHexString(),
textInArray: 'textInArray2',
},
{
id: new mongoose.Types.ObjectId().toHexString(),
textInArray: 'textInArray3',
},
]
await payload.update({
collection: 'diff',
data: {
array: newArray,
},
id: diffID,
})
await payload.update({
collection: 'diff',
data: {
array: newArray.map((arrayItem, i) => {
if (i === 1) {
return {
...arrayItem,
textInArray: 'textInArray2Modified',
}
}
return arrayItem
}),
},
id: diffID,
})
const latestVersionDiff = (
await payload.findVersions({
collection: diffCollectionSlug,
depth: 0,
limit: 1,
where: {
parent: { equals: diffID },
},
})
).docs[0] as Diff
await navigateToDiffVersionView(latestVersionDiff.id)
const blocks = page.locator('[data-field-path="array"]')
await expect(blocks.locator('.iterable-diff__label')).toHaveCount(1)
await expect(blocks.locator('.iterable-diff__label')).toHaveText('Item 02')
const blockDiff = page.locator('[data-field-path="array.1.textInArray"]')
await expect(blockDiff.locator('.html-diff__diff-old')).toHaveText('textInArray2')
await expect(blockDiff.locator('.html-diff__diff-new')).toHaveText('textInArray2Modified')
})
}) })
describe('Scheduled publish', () => { describe('Scheduled publish', () => {