Skip to content

Commit 11b382c

Browse files
Merge remote-tracking branch 'origin/suggest/inline-wiring' into fix/f27-summary-context
# Conflicts: # packages/editor/src/components/suggestion-mode/suggestion-summary.js # packages/editor/src/components/suggestion-mode/test/suggestion-summary.js
2 parents b21bea3 + 848b2b6 commit 11b382c

11 files changed

Lines changed: 894 additions & 32 deletions

File tree

docs/explanations/architecture/suggestions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ The current implementation (`provider.js`) uses comment meta. A future Yjs-backe
273273

274274
In the notes sidebar, a suggestion thread renders:
275275

276-
- **`SuggestionSummary`** — a Docs-style "Add: …", "Delete: …", "Format: …" summary derived from the operations. It is the sidebar's sole suggestion renderer; its `wordDiff` engine lives in `word-diff.js`, capped by `MAX_DIFF_LENGTH`/`MAX_DIFF_TOKENS` so a large payload can't freeze the sidebar.
276+
- **`SuggestionSummary`** — a Docs-style "Add: …", "Delete: …", "Change: …" summary derived from the operations. Inline formatting reads "Formatting: bold" and block attributes read "Change: heading level", so the two families of suggestion stay tellable apart in a mixed list. It is the sidebar's sole suggestion renderer; its `wordDiff` engine lives in `word-diff.js`, capped by `MAX_DIFF_LENGTH`/`MAX_DIFF_TOKENS` so a large payload can't freeze the sidebar.
277277
- **Accept / Reject icon buttons** — checkmark and close icons that trigger the provider's apply/reject flows.
278278

279279
## Yjs v2 Migration Path

packages/editor/CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
- Suggest mode: keep a pasted run's bold, italics and links when it becomes a suggestion. A rich-text paste was diffed as plain text, so the marker held the words and none of their formatting and accepting the suggestion destroyed a pasted link. The pasted run is now carried into the marker as rich text, whether it opens a new addition or grows the author's own pending one, with the marker stamped as the outermost format so the proposal stays a single `<mark>` ([#81672](https://github.com/WordPress/gutenberg/pull/81672)).
1313
- Suggest mode: keep the code editor closed while the Suggest intent is active and while the post still carries unresolved inline suggestion markers, and identify a marker by its element rather than by a substring of the document ([#81662](https://github.com/WordPress/gutenberg/pull/81662)).
1414
- Suggest mode: Announce inline suggestion markers to screen readers. A marker's state was carried entirely by color and text decoration, so a run proposed for deletion was read aloud as ordinary prose. Each marker is now bracketed by an announcement naming the kind of change and the person who proposed it, and add and delete markers carry `role="insertion"` and `role="deletion"`; a formatting suggestion no longer claims to be a deletion. Suggestions over overlapping runs nest, and each is announced and attributed to the person who made it ([#81663](https://github.com/WordPress/gutenberg/pull/81663), [#81957](https://github.com/WordPress/gutenberg/pull/81957)).
15+
- Suggest mode: keep a review decision and its note together through undo. Accepting or rejecting a suggestion changes block content and resolves the note, but only the content half is in the undo stack, so undo put the marker back on a note that stayed resolved - a marked-up run with no Accept/Reject on it and no way to clear it. Undoing a decision made in this session now reopens its note along with the marker ([#81669](https://github.com/WordPress/gutenberg/pull/81669)).
1516
- Suggest mode: extend a format suggestion on a second toggle instead of opening a second one. Toggling a further format over a run that already carries the suggester's own pending `format` marker recorded a suggestion whose before and after were both empty - unreviewable and unapplyable - and made every marker in the block disappear. The existing suggestion is now revised in place, a toggle that restores the original run retracts it rather than storing a note that proposes nothing, and a note that has replies is revised rather than withdrawn ([#81665](https://github.com/WordPress/gutenberg/pull/81665)).
1617
- Suggest mode: refuse post status changes while suggesting. `editPost` drops the `status` field in the `suggest` intent, the status control and the summary panel show the status without offering to change it, and the publish button is disabled there rather than dropping the status edit and saving the post anyway. A status edit that travels with a companion field - the `password` that visibility changes carry, the `date` that scheduling carries - is refused whole rather than half-applied, a status repeated at the value it already holds is not announced as a refusal, and a status staged before the intent changed is discarded on the way in. The refusal is announced and shown in a snackbar ([#81664](https://github.com/WordPress/gutenberg/pull/81664)).
1718
- Register the editor and block editor keyboard shortcuts from the editor provider, so shortcuts work for consumers that mount the editor without rendering `EditorKeyboardShortcutsRegister` themselves ([#81580](https://github.com/WordPress/gutenberg/pull/81580)).

packages/editor/src/components/suggestion-mode/provider.js

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -426,10 +426,56 @@ function withDecisionInFlight( decide ) {
426426
return await decide( args );
427427
} finally {
428428
decisionsInFlight.delete( key );
429+
resolvedThisSession.add( key );
429430
}
430431
};
431432
}
432433

434+
/*
435+
* Suggestions this session has applied or rejected.
436+
*
437+
* A decision has two halves: the block change, which the undo stack holds, and
438+
* the comment's lifecycle status, which lives on the server and no keystroke
439+
* here can walk back. Undo therefore puts a marker back while its note stays
440+
* resolved, leaving a marked-up run with no Accept/Reject on it and no way to
441+
* clear it through the UI (issue #73411, F-18). The note collector watches this
442+
* set and reopens a note whose marker reappears, so the two halves travel
443+
* together again.
444+
*
445+
* Deliberately scoped to decisions made HERE rather than to every resolved note
446+
* that has a live marker: a peer's decision arriving through sync before this
447+
* session's content catches up looks identical from the outside, and reopening
448+
* that would undo their review.
449+
*/
450+
const resolvedThisSession = new Set();
451+
452+
/**
453+
* Comment ids this session applied or rejected and has not yet reopened.
454+
*
455+
* @return {Set<string>} Comment id keys.
456+
*/
457+
export function getSuggestionsResolvedThisSession() {
458+
return resolvedThisSession;
459+
}
460+
461+
/**
462+
* Forget a decision, once its note has been reopened or is past reopening.
463+
*
464+
* @param {number|string} commentId Comment id.
465+
*/
466+
export function forgetResolvedSuggestion( commentId ) {
467+
resolvedThisSession.delete( String( commentId ) );
468+
}
469+
470+
/**
471+
* Record a decision again, so a failed reopen is retried on a later pass.
472+
*
473+
* @param {number|string} commentId Comment id.
474+
*/
475+
export function rememberResolvedSuggestion( commentId ) {
476+
resolvedThisSession.add( String( commentId ) );
477+
}
478+
433479
/**
434480
* Comment-meta backed suggestions provider. The provider shape is stable so
435481
* a future Yjs-backed provider can swap in without touching the UI.

packages/editor/src/components/suggestion-mode/suggestion-note-gc.js

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,11 @@ import { useSuggestionOverlay } from './overlay-context';
3636
import {
3737
findInlineOp,
3838
findStructuralOp,
39+
forgetResolvedSuggestion,
40+
getSuggestionsResolvedThisSession,
3941
isSuggestionDecisionInFlight,
4042
parseSuggestionPayload,
43+
rememberResolvedSuggestion,
4144
} from './provider';
4245
import { findSuggestionRange } from '../inline-suggestions';
4346
import { getNoteIdsFromMetadata } from '../collab-sidebar/utils';
@@ -156,13 +159,25 @@ export default function SuggestionNoteGC() {
156159
// Pending suggestion root notes only; replies and resolved notes have no
157160
// anchor contract.
158161
const suggestionNotes = [];
162+
/*
163+
* Notes this session applied or rejected. Their marker should be gone; if
164+
* it is back, an undo walked the block half of the decision back while the
165+
* note stayed resolved, and the note has to follow (#73411, F-18).
166+
*/
167+
const resolvedNotes = [];
168+
const resolvedIds = getSuggestionsResolvedThisSession();
159169
for ( const note of notes ?? [] ) {
160-
if ( note.parent !== 0 || note.status !== 'hold' ) {
170+
if ( note.parent !== 0 ) {
161171
continue;
162172
}
163173
const anchor = describeAnchor( note );
164-
if ( anchor ) {
174+
if ( ! anchor ) {
175+
continue;
176+
}
177+
if ( note.status === 'hold' ) {
165178
suggestionNotes.push( { note, anchor } );
179+
} else if ( resolvedIds.has( String( note.id ) ) ) {
180+
resolvedNotes.push( { note, anchor } );
166181
}
167182
}
168183

@@ -174,6 +189,8 @@ export default function SuggestionNoteGC() {
174189

175190
const entriesRef = useRef( entries );
176191
entriesRef.current = entries;
192+
const resolvedNotesRef = useRef( resolvedNotes );
193+
resolvedNotesRef.current = resolvedNotes;
177194
const clearOverlayRef = useRef( clearOverlay );
178195
clearOverlayRef.current = clearOverlay;
179196

@@ -210,6 +227,15 @@ export default function SuggestionNoteGC() {
210227
}`
211228
);
212229
}
230+
for ( const { note, anchor } of resolvedNotes ) {
231+
parts.push(
232+
`r${ note.id }:${
233+
isAnchorPresent( note, anchor, blockEditor, entries )
234+
? 1
235+
: 0
236+
}`
237+
);
238+
}
213239
return parts.join( '|' );
214240
},
215241
// eslint-disable-next-line react-hooks/exhaustive-deps
@@ -338,6 +364,42 @@ export default function SuggestionNoteGC() {
338364
trashedRef.current.set( idKey, info );
339365
} );
340366
}
367+
/*
368+
* Undo: a decision this session made has had its marker put back. The
369+
* comment's status is the half undo cannot reach, so reopen it here -
370+
* otherwise the run stays marked with no Accept/Reject on it and no way
371+
* to clear it through the UI (#73411, F-18). The in-flight guard keeps
372+
* this off the decision's own window, where the status can land before
373+
* the tree has been mutated.
374+
*/
375+
for ( const { note, anchor } of resolvedNotesRef.current ) {
376+
if (
377+
isSuggestionDecisionInFlight( note.id ) ||
378+
! isAnchorPresent(
379+
note,
380+
anchor,
381+
blockEditor,
382+
entriesRef.current
383+
)
384+
) {
385+
continue;
386+
}
387+
forgetResolvedSuggestion( note.id );
388+
saveEntityRecord(
389+
'root',
390+
'comment',
391+
{
392+
id: note.id,
393+
status: 'hold',
394+
meta: { _wp_suggestion_status: '' },
395+
},
396+
{ throwOnError: true }
397+
).catch( () => {
398+
// Reopen failed; leave it recorded so a later pass retries.
399+
rememberResolvedSuggestion( note.id );
400+
} );
401+
}
402+
341403
// `presenceSignature` fully determines the work; the other values are
342404
// read through refs or stable.
343405
// eslint-disable-next-line react-hooks/exhaustive-deps

packages/editor/src/components/suggestion-mode/suggestion-summary.js

Lines changed: 114 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@
1515
* halves are one change, so they are reported on one
1616
* line ("old" → "new") rather than as an unrelated
1717
* delete plus add.
18-
* - **Format: …** — non-text attribute changes. Uses
19-
* `FORMAT_ATTRIBUTE_LABELS` to surface friendly names
20-
* (e.g. `level` → "heading level") with a fallback to
21-
* the raw attribute name so a brand-new attribute
22-
* isn't silently swallowed.
18+
* - **Change: …** — non-text attribute changes. Uses
19+
* `ATTRIBUTE_LABELS` to surface friendly names
20+
* (e.g. `level` → "heading level") and humanizes any
21+
* attribute not in that map so a brand-new attribute
22+
* isn't silently swallowed or shown as `camelCase`.
23+
* - **Rename block:** — the one attribute change worth its own line: a
24+
* block renamed through `metadata.name`, reported
25+
* with the name being proposed.
2326
* - **Add formatting: / Remove formatting:**
2427
* — pure inline-format changes (bold, italic, links).
2528
* Detected by tag-level diff of the serialized HTML
@@ -36,6 +39,10 @@
3639
* kind and count ("3 spaces") rather than quoted into invisibility, and
3740
* structural lines name the parent block when there is one.
3841
*
42+
* "Change:" and the two "… formatting:" labels name different families of
43+
* suggestion, so they have to be readable as different things in a mixed list.
44+
* The attribute family was once "Format:", one word from "Formatting:".
45+
*
3946
* Quoted lines report what a reviewer would read on screen: the diff behind
4047
* them runs on visible text, never on the raw content attribute, so no markup
4148
* reaches the sidebar. Changed words keep the spaces that separated them, and
@@ -63,11 +70,14 @@ const SUMMARY_MAX_CHARS = 120;
6370
const REPLACE_SIDE_MAX_CHARS = 60;
6471

6572
/**
66-
* Friendlier labels for common block attributes so `Format:` lines read like
67-
* human categories rather than internal names. Anything not in this map
68-
* falls through to the raw attribute name.
73+
* Friendlier labels for common block attributes so `Change:` lines read like
74+
* human categories rather than internal names. Anything not in this map is
75+
* humanized by `humanizeAttributeName`.
76+
*
77+
* The names here match what the editor's own controls call these settings, so
78+
* a reviewer reading "additional CSS class" can go and find the field.
6979
*/
70-
const FORMAT_ATTRIBUTE_LABELS = {
80+
const ATTRIBUTE_LABELS = {
7181
level: __( 'heading level' ),
7282
align: __( 'alignment' ),
7383
textAlign: __( 'text alignment' ),
@@ -77,8 +87,46 @@ const FORMAT_ATTRIBUTE_LABELS = {
7787
href: __( 'link' ),
7888
backgroundColor: __( 'background color' ),
7989
textColor: __( 'text color' ),
90+
className: __( 'additional CSS class' ),
91+
anchor: __( 'HTML anchor' ),
92+
content: __( 'text' ),
93+
metadata: __( 'block settings' ),
8094
};
8195

96+
/**
97+
* Turn an attribute key the summary has no friendly label for into something
98+
* readable: `fontFamily` becomes "font family", `layout_type` becomes "layout
99+
* type". Better than surfacing the raw key, which used to be lowercased whole
100+
* and rendered `className` as the non-word "classname".
101+
*
102+
* @param {string} key Attribute key.
103+
* @return {string} Humanized name.
104+
*/
105+
function humanizeAttributeName( key ) {
106+
if ( typeof key !== 'string' || key === '' ) {
107+
return __( 'setting' );
108+
}
109+
return key
110+
.replace( /([a-z0-9])([A-Z])/g, '$1 $2' )
111+
.replace( /[_-]+/g, ' ' )
112+
.replace( /\s+/g, ' ' )
113+
.trim()
114+
.toLowerCase();
115+
}
116+
117+
/**
118+
* Read the custom block name out of a `metadata` attribute value, which is
119+
* where the "Rename" command stores it. Returns null for anything that isn't
120+
* a usable name so the caller can fall back to the generic label.
121+
*
122+
* @param {*} metadata Attribute value.
123+
* @return {?string} The block name, or null.
124+
*/
125+
function readBlockName( metadata ) {
126+
const name = metadata?.name;
127+
return typeof name === 'string' && name.trim() !== '' ? name.trim() : null;
128+
}
129+
82130
/**
83131
* Convert a block name like `core/paragraph` to a friendlier label used in
84132
* structural-suggestion summaries ("Remove block: paragraph"). Strips the
@@ -343,6 +391,19 @@ function joinLabels( labels ) {
343391
return unique.join( ', ' );
344392
}
345393

394+
/**
395+
* Join attribute labels without touching their case. Unlike `joinLabels`,
396+
* which lowercases the inline-format names it is given, these labels already
397+
* read as the editor writes them and carry acronyms - "HTML anchor",
398+
* "additional CSS class" - that lowercasing would turn into noise.
399+
*
400+
* @param {string[]} labels Attribute labels.
401+
* @return {string} Comma-joined list.
402+
*/
403+
function joinAttributeLabels( labels ) {
404+
return Array.from( new Set( labels.filter( Boolean ) ) ).join( ', ' );
405+
}
406+
346407
function ellipsize( text, max = SUMMARY_MAX_CHARS ) {
347408
const trimmed = text.replace( /\s+/g, ' ' ).trim();
348409
if ( trimmed.length <= max ) {
@@ -473,8 +534,8 @@ function isTextLike( value ) {
473534
/**
474535
* Build a list of `{ label, value }` lines summarizing a suggestion. The
475536
* content attribute is reported with `Add:` / `Delete:` quotes; other
476-
* attribute changes are collapsed into a single `Format:` line listing the
477-
* touched attributes.
537+
* attribute changes are collapsed into a single `Change:` line listing the
538+
* touched settings.
478539
*
479540
* @param {import('./provider').SuggestionOperation[]} operations Operations.
480541
* @return {Array<{label: string, value: string}>} Rendered lines.
@@ -564,11 +625,41 @@ export function summarizeOperations( operations ) {
564625
continue;
565626
}
566627

628+
/*
629+
* A rename is stored as a `metadata` attribute change, so it would
630+
* otherwise arrive in the sidebar as the word "metadata" - a reviewer
631+
* can't tell whether a block was renamed, bound to a field, or turned
632+
* into a pattern override. Report the proposed name instead, and only
633+
* when the name is what actually changed.
634+
*/
635+
if ( op.attribute === 'metadata' ) {
636+
const beforeName = readBlockName( op.before );
637+
const afterName = readBlockName( op.after );
638+
if ( afterName && afterName !== beforeName ) {
639+
lines.push( {
640+
label: __( 'Rename block:' ),
641+
value: `“${ ellipsize( afterName ) }”`,
642+
} );
643+
continue;
644+
}
645+
if ( beforeName && ! afterName ) {
646+
lines.push( {
647+
label: __( 'Rename block:' ),
648+
value: sprintf(
649+
/* translators: %s: the block's current custom name. */
650+
__( 'reset “%s” to the default name' ),
651+
ellipsize( beforeName )
652+
),
653+
} );
654+
continue;
655+
}
656+
}
657+
567658
const isContent = op.attribute === 'content';
568659
/*
569660
* The word diff below is O(m*n); cap the input length so a payload
570661
* approaching the 64KB limit can't freeze the sidebar. Oversized
571-
* content changes fall back to the attribute-level "Format: content"
662+
* content changes fall back to the attribute-level "Change: text"
572663
* line. This character cap composes with `wordDiff`'s own
573664
* MAX_DIFF_TOKENS guard, which bounds the LCS table itself for any
574665
* input that passes here but tokenizes pathologically.
@@ -670,18 +761,26 @@ export function summarizeOperations( operations ) {
670761
}
671762

672763
if ( attributeLabels.length > 0 ) {
764+
/*
765+
* Attribute changes and inline formatting are different families of
766+
* suggestion, so their labels have to be tellable apart at a glance in
767+
* a mixed list. "Format:" next to "Formatting:" was not.
768+
*/
673769
const labels = attributeLabels.map(
674-
( key ) => FORMAT_ATTRIBUTE_LABELS[ key ] ?? key
770+
( key ) => ATTRIBUTE_LABELS[ key ] ?? humanizeAttributeName( key )
675771
);
676-
lines.push( { label: __( 'Format:' ), value: joinLabels( labels ) } );
772+
lines.push( {
773+
label: __( 'Change:' ),
774+
value: joinAttributeLabels( labels ),
775+
} );
677776
}
678777

679778
return lines;
680779
}
681780

682781
/**
683782
* Compact sidebar summary of a suggestion — "Add: …", "Delete: …",
684-
* "Format: …". Designed to mirror a Google Docs-style review note.
783+
* "Change: …". Designed to mirror a Google Docs-style review note.
685784
*
686785
* @param {Object} props
687786
* @param {import('./provider').SuggestionOperation[]} props.operations

0 commit comments

Comments
 (0)