Skip to content

Commit c5de3b8

Browse files
bphbph
committed
Address review feedback batch (correctness, i18n, safety)
Bugs / correctness: - addColor: derive next index from the max existing `new-color-N` suffix instead of `value.length + 1`. Add → Remove → Add no longer produces duplicate slugs (P1). - Palette rows: assign a stable client-only `__cbtRowId` at creation time and use it as the React key, so removing a middle row no longer leaks Dropdown/ColorPicker state to subsequent rows. The ID is stripped from the payload before posting. - Update button: disable when any palette row has an empty slug; show a tooltip explaining why. Prevents the modal from POSTing an entry with `slug: ""`. Concurrent edits / lost updates (Codex P2, reviewer #12, #13): - Build a minimal patch on save: only the color-settings keys that differ from the snapshot, and the palette only if the user touched it. Stops unrelated toggles from clobbering a concurrent palette edit (RFC 7396 list-replacement) made elsewhere. - Skip the post-refresh reseed effect while the user has unsaved edits, so a mid-flight cache invalidation doesn't silently wipe in-progress work. - changeCount now reports `dirtyColorKeys.length + (paletteDirty ? 1 : 0)` — toggles count individually, palette counts as one unit. Documented in the PR description. i18n: - Update label uses `_n( 'Update (%d change)', 'Update (%d changes)', count )` so single-change pluralization is correct. - Section list in the warning notice uses `Intl.ListFormat` for locale-aware joining instead of a hard-coded English comma, with a plain `, `.join() fallback for environments without it. SCSS / observability: - Replace hard-coded `#757575`, `#ddd`, `#fff` with `$gray-700`, `$gray-300`, `$white` from `@wordpress/base-styles/_colors`. - json-editor-modal: log fetch failures via `console.error` instead of swallowing them silently. Refs #838
1 parent 910d472 commit c5de3b8

3 files changed

Lines changed: 166 additions & 48 deletions

File tree

‎src/editor-sidebar/edit-theme-settings-modal.js‎

Lines changed: 153 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* WordPress dependencies
33
*/
4-
import { __, sprintf } from '@wordpress/i18n';
4+
import { __, _n, sprintf } from '@wordpress/i18n';
55
import {
66
useState,
77
useEffect,
@@ -63,6 +63,54 @@ const COLOR_SETTINGS_DEFAULTS = {
6363
link: false,
6464
};
6565

66+
// Local-only stable ID for React keys on palette rows. Entries that come
67+
// from the server (initial load, save response) get a `__cbtRowId` added
68+
// when we seed local state; entries the user adds get one at creation.
69+
// Stripped from the payload before sending to the server.
70+
let nextRowIdCounter = 0;
71+
const newRowId = () => `cbt-row-${ ++nextRowIdCounter }`;
72+
73+
const augmentPaletteWithIds = ( entries ) =>
74+
entries.map( ( entry ) => ( {
75+
...entry,
76+
__cbtRowId: entry.__cbtRowId || newRowId(),
77+
} ) );
78+
79+
const stripPaletteIds = ( entries ) =>
80+
entries.map( ( { __cbtRowId: _id, ...rest } ) => rest );
81+
82+
const palettesEqual = ( a, b ) => {
83+
if ( a.length !== b.length ) {
84+
return false;
85+
}
86+
for ( let i = 0; i < a.length; i++ ) {
87+
if (
88+
a[ i ].slug !== b[ i ].slug ||
89+
a[ i ].name !== b[ i ].name ||
90+
a[ i ].color !== b[ i ].color
91+
) {
92+
return false;
93+
}
94+
}
95+
return true;
96+
};
97+
98+
// Find the next free index for an auto-generated `new-color-N` slug so
99+
// Add → Remove → Add doesn't produce duplicate slugs.
100+
const nextNewColorIndex = ( entries ) => {
101+
let max = 0;
102+
for ( const entry of entries ) {
103+
const match = /^new-color-(\d+)$/.exec( entry.slug || '' );
104+
if ( match ) {
105+
const n = parseInt( match[ 1 ], 10 );
106+
if ( n > max ) {
107+
max = n;
108+
}
109+
}
110+
}
111+
return max + 1;
112+
};
113+
66114
const ColorSettingsPanel = ( { value, onChange } ) => {
67115
const update = ( key ) => ( next ) =>
68116
onChange( { ...value, [ key ]: next } );
@@ -250,7 +298,8 @@ const PalettePanel = ( { value, onChange } ) => {
250298
onChange( [
251299
...value,
252300
{
253-
slug: `new-color-${ value.length + 1 }`,
301+
__cbtRowId: newRowId(),
302+
slug: `new-color-${ nextNewColorIndex( value ) }`,
254303
name: __( 'New color', 'create-block-theme' ),
255304
color: '#000000',
256305
},
@@ -296,7 +345,7 @@ const PalettePanel = ( { value, onChange } ) => {
296345
<ItemGroup isBordered isSeparated>
297346
{ value.map( ( entry, index ) => (
298347
<div
299-
key={ index }
348+
key={ entry.__cbtRowId }
300349
ref={
301350
index === value.length - 1
302351
? lastRowRef
@@ -501,22 +550,17 @@ const getCustomizedSections = ( userGlobalStyles, edits ) => {
501550
);
502551
};
503552

504-
// Per-field dirty diff between the modal's working state and the last-saved
505-
// snapshot from the server. Returns the number of fields that differ —
506-
// surfaced in the Update button label.
507-
const countChanges = ( current, snapshot ) => {
508-
let count = 0;
553+
// Returns the set of color-settings keys whose current value differs from
554+
// the last-saved snapshot. Used to drive both the Update-button counter
555+
// and the minimal-patch payload sent to the server.
556+
const getDirtyColorKeys = ( current, snapshot ) => {
557+
const keys = [];
509558
for ( const key of COLOR_SETTINGS_KEYS ) {
510-
if ( current.colorSettings[ key ] !== snapshot.colorSettings[ key ] ) {
511-
count += 1;
559+
if ( current[ key ] !== snapshot[ key ] ) {
560+
keys.push( key );
512561
}
513562
}
514-
if (
515-
JSON.stringify( current.palette ) !== JSON.stringify( snapshot.palette )
516-
) {
517-
count += 1;
518-
}
519-
return count;
563+
return keys;
520564
};
521565

522566
export const EditThemeSettingsModal = ( { onRequestClose } ) => {
@@ -549,58 +593,101 @@ export const EditThemeSettingsModal = ( { onRequestClose } ) => {
549593

550594
const themeColor = themeData?.theme_json?.settings?.color;
551595

596+
// `snapshot` is the server-canonical state — palette entries here have
597+
// NO `__cbtRowId` since the server never sees that key. Working state
598+
// (`palette`) is augmented with row IDs for stable React keys.
552599
const initialState = useMemo(
553600
() => ( {
554601
colorSettings: pickColorSettings( themeColor ),
555-
palette: themeColor?.palette ? [ ...themeColor.palette ] : [],
602+
palette: Array.isArray( themeColor?.palette )
603+
? themeColor.palette
604+
: [],
556605
} ),
557606
[ themeColor ]
558607
);
559608

560609
const [ colorSettings, setColorSettings ] = useState(
561610
initialState.colorSettings
562611
);
563-
const [ palette, setPalette ] = useState( initialState.palette );
612+
const [ palette, setPalette ] = useState( () =>
613+
augmentPaletteWithIds( initialState.palette )
614+
);
564615
const [ snapshot, setSnapshot ] = useState( initialState );
565616
const [ isSaving, setIsSaving ] = useState( false );
566617

567-
// Reseed local state when the server-side theme data refreshes (initial
568-
// load, and after a successful save invalidates the resolver).
618+
const dirtyColorKeys = useMemo(
619+
() => getDirtyColorKeys( colorSettings, snapshot.colorSettings ),
620+
[ colorSettings, snapshot.colorSettings ]
621+
);
622+
const strippedPalette = useMemo(
623+
() => stripPaletteIds( palette ),
624+
[ palette ]
625+
);
626+
const paletteDirty = useMemo(
627+
() => ! palettesEqual( strippedPalette, snapshot.palette ),
628+
[ strippedPalette, snapshot.palette ]
629+
);
630+
const changeCount = dirtyColorKeys.length + ( paletteDirty ? 1 : 0 );
631+
const isDirty = changeCount > 0;
632+
633+
const hasEmptySlug = palette.some(
634+
( entry ) => ! entry.slug || ! entry.slug.trim()
635+
);
636+
637+
// Reseed local state from refreshed server data — but only if the user
638+
// hasn't started editing in this session. Otherwise a mid-flight cache
639+
// invalidation (e.g. another resolver triggers it) would silently wipe
640+
// pending edits.
569641
useEffect( () => {
642+
if ( isDirty ) {
643+
return;
644+
}
570645
setColorSettings( initialState.colorSettings );
571-
setPalette( initialState.palette );
646+
setPalette( augmentPaletteWithIds( initialState.palette ) );
572647
setSnapshot( initialState );
648+
// `isDirty` is intentionally excluded from deps: we only want to
649+
// reseed when the server-side data changes, not when the user's
650+
// edits transition the dirty flag.
651+
// eslint-disable-next-line react-hooks/exhaustive-deps
573652
}, [ initialState ] );
574653

575-
const changeCount = countChanges( { colorSettings, palette }, snapshot );
576-
const isDirty = changeCount > 0;
577-
578654
const handleUpdateClick = async () => {
655+
// Build a minimal patch: only the keys the user touched in this
656+
// session. Avoids overwriting fields another tab/CLI may have
657+
// edited concurrently (server-side merge is RFC 7396 last-writes-
658+
// wins on lists, so sending an unchanged palette would still
659+
// clobber concurrent palette edits).
660+
const colorPayload = {};
661+
for ( const key of dirtyColorKeys ) {
662+
colorPayload[ key ] = colorSettings[ key ];
663+
}
664+
if ( paletteDirty ) {
665+
colorPayload.palette = strippedPalette;
666+
}
667+
if ( Object.keys( colorPayload ).length === 0 ) {
668+
return;
669+
}
670+
579671
setIsSaving( true );
580672
try {
581673
// The endpoint returns `{ status, theme_json: <merged> }` on
582-
// success. Reseed from the merged theme.json directly instead of
583-
// relying on a refetch: `getCurrentTheme` is entity-record-backed
584-
// and `invalidateResolution` doesn't reliably re-fetch it before
585-
// the user sees the (now-stale) dirty count.
674+
// success. Reseed snapshot from the merged theme.json directly
675+
// instead of relying on a refetch: `getCurrentTheme` is
676+
// entity-record-backed and `invalidateResolution` doesn't
677+
// reliably re-fetch it before the user sees the dirty count.
586678
const response = await postUpdateThemeSettings( {
587-
settings: {
588-
color: {
589-
...colorSettings,
590-
palette,
591-
},
592-
},
679+
settings: { color: colorPayload },
593680
} );
594681
const savedColor = response?.theme_json?.settings?.color || {};
595682
const nextColorSettings = pickColorSettings( savedColor );
596-
const nextPalette = Array.isArray( savedColor.palette )
597-
? [ ...savedColor.palette ]
683+
const nextPaletteRaw = Array.isArray( savedColor.palette )
684+
? savedColor.palette
598685
: [];
599686
setColorSettings( nextColorSettings );
600-
setPalette( nextPalette );
687+
setPalette( augmentPaletteWithIds( nextPaletteRaw ) );
601688
setSnapshot( {
602689
colorSettings: nextColorSettings,
603-
palette: nextPalette,
690+
palette: nextPaletteRaw,
604691
} );
605692
createSuccessNotice(
606693
__( 'Theme settings saved.', 'create-block-theme' ),
@@ -646,11 +733,27 @@ export const EditThemeSettingsModal = ( { onRequestClose } ) => {
646733
const updateLabel = isDirty
647734
? sprintf(
648735
/* translators: %d: number of pending changes */
649-
__( 'Update (%d changes)', 'create-block-theme' ),
736+
_n(
737+
'Update (%d change)',
738+
'Update (%d changes)',
739+
changeCount,
740+
'create-block-theme'
741+
),
650742
changeCount
651743
)
652744
: __( 'Update', 'create-block-theme' );
653745

746+
// Use Intl.ListFormat so the separator (and final "and") match the
747+
// user's locale instead of being hard-coded English punctuation.
748+
// Falls back to a plain ", " join in environments without it.
749+
const sectionList =
750+
typeof Intl !== 'undefined' && Intl.ListFormat
751+
? new Intl.ListFormat( undefined, {
752+
style: 'long',
753+
type: 'conjunction',
754+
} ).format( customizedSections )
755+
: customizedSections.join( ', ' );
756+
654757
return (
655758
<Modal
656759
size="large"
@@ -683,7 +786,7 @@ export const EditThemeSettingsModal = ( { onRequestClose } ) => {
683786
'You have changes in the Site Editor that haven’t been written to theme.json: <list>%s</list>.',
684787
'create-block-theme'
685788
),
686-
customizedSections.join( ', ' )
789+
sectionList
687790
),
688791
{ list: <strong /> }
689792
) }
@@ -713,8 +816,17 @@ export const EditThemeSettingsModal = ( { onRequestClose } ) => {
713816
<Button
714817
variant="primary"
715818
onClick={ handleUpdateClick }
716-
disabled={ ! isDirty || isSaving }
819+
disabled={ ! isDirty || isSaving || hasEmptySlug }
717820
isBusy={ isSaving }
821+
label={
822+
hasEmptySlug
823+
? __(
824+
'Every palette entry needs a slug.',
825+
'create-block-theme'
826+
)
827+
: undefined
828+
}
829+
showTooltip={ hasEmptySlug }
718830
>
719831
{ updateLabel }
720832
</Button>

‎src/editor-sidebar/json-editor-modal.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,14 @@ const ThemeJsonEditorModal = ( { onRequestClose } ) => {
3838
setThemeName( active?.name?.raw ?? '' );
3939
setThemeData( JSON.stringify( active?.theme_json, null, 2 ) );
4040
} )
41-
.catch( () => {
42-
// Swallow — leave the modal showing whatever was last rendered.
41+
.catch( ( err ) => {
42+
// Leave the modal showing whatever was last rendered, but
43+
// surface the failure to the console so it isn't invisible.
44+
// eslint-disable-next-line no-console
45+
console.error(
46+
'View theme.json: failed to fetch active theme',
47+
err
48+
);
4349
} );
4450
return () => {
4551
cancelled = true;

‎src/plugin-styles.scss‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ $modal-footer-height: 70px;
1212
}
1313

1414
.create-block-theme__metadata-editor-modal__footer {
15-
border-top: 1px solid #ddd;
16-
background-color: #fff;
15+
border-top: 1px solid $gray-300;
16+
background-color: $white;
1717
position: absolute;
1818
bottom: 0;
1919
margin: 0 -32px;
@@ -40,8 +40,8 @@ $modal-footer-height: 70px;
4040
}
4141

4242
.create-block-theme__edit-theme-settings-modal__footer {
43-
border-top: 1px solid #ddd;
44-
background-color: #fff;
43+
border-top: 1px solid $gray-300;
44+
background-color: $white;
4545
position: absolute;
4646
bottom: 0;
4747
margin: 0 -32px;
@@ -116,7 +116,7 @@ $modal-footer-height: 70px;
116116
line-height: 1.4;
117117
text-transform: uppercase;
118118
letter-spacing: 0.5px;
119-
color: #757575;
119+
color: $gray-700;
120120
}
121121
}
122122

0 commit comments

Comments
 (0)