diff --git a/docs/explanations/architecture/suggestions.md b/docs/explanations/architecture/suggestions.md index eb818174685928..16ae83737c0b08 100644 --- a/docs/explanations/architecture/suggestions.md +++ b/docs/explanations/architecture/suggestions.md @@ -2,9 +2,9 @@ ## Overview -Suggestions extend the Notes feature (block-level comments) to support proposed content changes. A reviewer switches to **Suggest** intent, edits a block, and the change is captured as a versioned suggestion payload stored on a note comment. The post author can then **Apply** (merge the change) or **Reject** (dismiss it) from the notes sidebar. +Suggestions extend the Notes feature (block-level comments) to support proposed content changes. A reviewer switches to **Suggest** intent and edits a block; the change is captured as a versioned suggestion payload on a note comment, auto-saved in the background after a short idle window. The post author then **Accepts** (merges the change) or **Rejects** (dismisses it) from the notes sidebar. -The feature is designed around a swappable provider interface so the storage backend can evolve from comment-meta (v1, current) to Yjs `AttributionManager` (v2, future) without changing the UI or apply/reject logic. +The feature is designed around a swappable provider interface so the storage backend can evolve from comment-meta (v1, current) to Yjs `AttributionManager` (v2, future) without changing the UI or accept/reject logic. ## End-to-end lifecycle @@ -14,21 +14,22 @@ sequenceDiagram participant U as Reviewer participant B as Block participant O as Overlay store + participant AS as AutoSave (debounced) participant P as SuggestionsProvider participant R as REST (/wp/v2/comments) participant A as Post author U->>B: Switch to Suggest intent, edit block - B->>O: setAttributes → overlay (store baseline on first edit) + B->>O: setAttributes → overlay (capture baseline on first edit) Note right of O: Block-editor store is NEVER written - U->>B: Click "Submit suggestion" - B->>P: createSuggestion({ clientId, operations }) - P->>R: POST note + _wp_suggestion meta + O->>AS: Overlay changed (debounce ~1.5s) + AS->>P: createSuggestion or updateSuggestion + P->>R: POST / PUT note + _wp_suggestion meta R-->>P: Saved comment - P->>B: updateBlockAttributes(metadata.noteId) + P->>B: updateBlockAttributes(metadata.noteId) (on create) A->>A: Open notes sidebar - A->>P: Apply (or Reject) + A->>P: Accept (or Reject) alt baseRevision stale P-->>A: Confirm dialog ("Apply anyway?") end @@ -56,7 +57,13 @@ When the intent is `suggest`, an `editor.BlockEdit` filter (`withSuggestionOverl 2. **Diversion** — `setAttributes` writes to a React-context-backed overlay (`SuggestionOverlayProvider`) keyed by `clientId`, not the block-editor store. 3. **Merge for render** — the block receives `{ ...realAttributes, ...overlayAttributes }` so the user sees their in-progress change live. -Because the store is never touched, autosave, undo/redo, and RTC sync stay at the real baseline. On commit, the overlay is serialized into a suggestion payload and sent to the server as comment meta; on discard, the overlay is cleared. +A companion `editor.BlockListBlock` filter tags each block that has a pending overlay with an `is-suggestion-pending` class, which renders the green bracket/outline treatment so edited blocks are discoverable without relying on the selected-block toolbar. + +Because the store is never touched, autosave, undo/redo, and RTC sync stay at the real baseline. + +### Auto-save + +There is no manual "Submit" step — `SuggestionAutoSave` watches the overlay and, after ~1.5 s of idle time on a given block, persists the current operations as a note comment. The overlay entry tracks the resulting `commentId` and a fingerprint of the last synced operations, so subsequent edits update the same note rather than creating new ones. If an edit is undone back to baseline the auto-saver trashes the note instead. ### Store interceptor @@ -90,8 +97,9 @@ The Suggest-mode subsystem lives in `packages/editor/src/components/suggestion-m | `with-suggestion-overlay.js`| `editor.BlockEdit` HOC that diverts `setAttributes` into the overlay. | | `store-interceptor.js` | Snapshot/diff/revert subscriber for store-level mutations; multi-peer accept logic. | | `provider.js` | `useSuggestionsProvider` — the `createSuggestion` / `applySuggestion` / `rejectSuggestion` API. Owns `operationsFromOverlay`, `applyOperations`, `parseSuggestionPayload`, and the wrapper-aware equality check. | -| `suggestion-diff.js` | Sidebar diff preview (word-level for text attributes, label fallback otherwise). | -| `commit-bar.js` | "Submit suggestion" toolbar shown while editing in Suggest intent. | +| `suggestion-diff.js` | Inline diff preview rendered in a comment thread (word-level for text attributes, label fallback otherwise). | +| `suggestion-summary.js` | Compact sidebar summary ("Add: …", "Delete: …", "Format: …") used in collapsed thread lists. | +| `auto-save.js` | Debounced background persistence of pending overlays as note comments (replaces the explicit "Submit" affordance from earlier phases). | REST/PHP surface lives in `lib/compat/wordpress-6.9/`: @@ -145,25 +153,29 @@ When bumping the version, add a migration step in `parseSuggestionPayload` that ``` useSuggestionsProvider() → { - createSuggestion({ clientId, blockName, operations }) → Promise - applySuggestion({ commentId, clientId, payload }) → Promise - rejectSuggestion({ commentId }) → Promise + createSuggestion({ clientId, blockName, operations }) → Promise + updateSuggestion({ commentId, blockName, operations }) → Promise + deleteSuggestion({ commentId }) → Promise + applySuggestion({ commentId, clientId, payload }) → Promise + rejectSuggestion({ commentId }) → Promise } ``` -The current implementation (`provider.js`) uses comment meta. A future Yjs-backed implementation would read from `AttributionManager` and write changes through the CRDT document, exposing the same three methods. +The current implementation (`provider.js`) uses comment meta. A future Yjs-backed implementation would read from `AttributionManager` and write changes through the CRDT document, exposing the same methods. -## Apply / Reject +## Accept / Reject -- **Apply**: runs `applyOperations(currentAttributes, payload.operations)` to produce new attributes, dispatches `updateBlockAttributes`, marks the note as resolved with `_wp_suggestion_status = 'applied'`. +- **Accept**: runs `applyOperations(currentAttributes, payload.operations)` to produce new attributes, dispatches `updateBlockAttributes`, marks the note as resolved with `_wp_suggestion_status = 'applied'`. - **Reject**: marks the note as resolved with `_wp_suggestion_status = 'rejected'`. No content change. -- **Staleness**: if `baseRevision` differs from the current `post_modified_gmt`, a warning snackbar is shown but apply is not blocked (conservative approach — the user reviews and decides). +- **Conflict detection**: accept-time staleness is checked at the attribute level, not the post level. `hasAttributeConflict(currentAttributes, operations)` compares each operation's captured `before` to the block's current value; only a real divergence on a targeted attribute prompts the "apply anyway" confirmation. Post-level `baseRevision` is still stamped into the payload for provenance, but does not drive the prompt — every auto-save bumps `post_modified_gmt`, so a post-level compare would flag nearly every suggestion as stale. + +## Review UI -## Diff Preview +In the notes sidebar, a suggestion thread renders: -The `SuggestionDiff` component renders operations in the notes sidebar: -- **Text attributes**: word-level LCS diff with green underlined insertions and red strikethrough deletions. -- **Non-text attributes**: `attribute: before → after` label. +- **`SuggestionSummary`** — a Docs-style "Add: …", "Delete: …", "Format: …" summary derived from the operations. +- **Accept / Reject icon buttons** — checkmark and close icons that trigger the provider's apply/reject flows. +- **`SuggestionDiff`** (still available) — the full word-level diff preview for when a more detailed view is needed. ## Yjs v2 Migration Path @@ -189,5 +201,6 @@ These are non-obvious quirks reviewers should keep in mind when reading the code - **Structural suggestions** (block insert, remove, move) are not yet supported. The `operations` array is designed to accept `block-insert-after` and `block-remove` types in the future. - **Inline text selections** are not anchored — suggestions apply to the entire attribute, not a sub-range. Fragment-level suggestions depend on inline annotation infrastructure tracked separately. -- **Permissions**: the Gutenberg REST comment controller overrides `update_item_permissions_check` so users with `edit_post` on the parent can update note comments (and their suggestion meta). This lets post editors apply or reject suggestions authored by other users. The `_wp_suggestion` and `_wp_suggestion_status` meta `auth_callback`s follow the same pattern. +- **Permissions**: the Gutenberg REST comment controller overrides `update_item_permissions_check` so users with `edit_post` on the parent can update note comments — **but only for suggestion-lifecycle fields** (`status` limited to `approved`/`hold`, plus `meta._wp_suggestion_status`). Any other field in the update body falls back to core's `edit_comment` check, preventing post editors from rewriting another user's note content. The `_wp_suggestion` and `_wp_suggestion_status` meta `auth_callback`s follow the same `edit_post`-on-parent pattern. +- **Payload size**: `_wp_suggestion` meta is capped at 64 KB via a `sanitize_callback`. Requests exceeding that limit are truncated to prevent arbitrarily large JSON blobs from flooding comment meta storage. - **Rich-text format fidelity**: the word-level diff operates on the serialized HTML string, which may produce noisy diffs when formatting (bold, links) changes. Progressive enhancement planned. diff --git a/docs/reference-guides/data/data-core-editor.md b/docs/reference-guides/data/data-core-editor.md index aed396b95bf789..c23ecf6407b822 100644 --- a/docs/reference-guides/data/data-core-editor.md +++ b/docs/reference-guides/data/data-core-editor.md @@ -1525,7 +1525,9 @@ Sets the current editor intent. The intent represents the user's editing purpose: directly editing content (`edit`), suggesting changes that the author can apply or reject (`suggest`), or viewing the post in a read-only mode (`view`). It is orthogonal to the `editorMode` preference (visual vs. code). -The value persists via the preferences store so the intent survives reloads. Unknown intents are silently rejected (no dispatch, no announcement) to keep typos or stale values from corrupting the preference; valid values are listed in `EDITOR_INTENTS`. +The intent is _session-scoped_ — held in the editor reducer (not the preferences store), so reloading the editor always returns to `edit`. Persisting suggest/view across reloads surprises users who don't realize they left the editor in a non-default state. + +Unknown intents are silently rejected (no dispatch, no announcement) so typos from a bookmarklet, browser extension, or third-party plugin can't poison the editor state; valid values are listed in `EDITOR_INTENTS`. _Parameters_ diff --git a/lib/compat/wordpress-6.9/block-comments.php b/lib/compat/wordpress-6.9/block-comments.php index 4b24a4edd9331b..31e50c67a9724d 100644 --- a/lib/compat/wordpress-6.9/block-comments.php +++ b/lib/compat/wordpress-6.9/block-comments.php @@ -120,13 +120,22 @@ function gutenberg_register_block_comment_metadata() { return $value; }, 'auth_callback' => function ( $allowed, $meta_key, $object_id ) { + // During comment creation the comment does not yet exist, so + // `object_id` is 0. Defer to the comment controller's own + // create permission — if the request can create the + // comment at all, it can set the suggestion meta on it. + if ( ! $object_id ) { + return current_user_can( 'edit_posts' ); + } + $comment = get_comment( $object_id ); + if ( $comment && 'note' === $comment->comment_type ) { + return current_user_can( 'edit_post', $comment->comment_post_ID ); + } return current_user_can( 'edit_comment', $object_id ); }, ) ); - // Lifecycle status for a suggestion. `pending` on creation; moved to - // `applied` or `rejected` by the apply/reject actions. register_meta( 'comment', '_wp_suggestion_status', @@ -141,6 +150,10 @@ function gutenberg_register_block_comment_metadata() { ), ), 'auth_callback' => function ( $allowed, $meta_key, $object_id ) { + $comment = get_comment( $object_id ); + if ( $comment && 'note' === $comment->comment_type ) { + return current_user_can( 'edit_post', $comment->comment_post_ID ); + } return current_user_can( 'edit_comment', $object_id ); }, ) diff --git a/lib/compat/wordpress-6.9/class-gutenberg-rest-comment-controller-6-9.php b/lib/compat/wordpress-6.9/class-gutenberg-rest-comment-controller-6-9.php index 45aab3bee54cf3..a4ce7d5360bdda 100644 --- a/lib/compat/wordpress-6.9/class-gutenberg-rest-comment-controller-6-9.php +++ b/lib/compat/wordpress-6.9/class-gutenberg-rest-comment-controller-6-9.php @@ -156,6 +156,100 @@ public function get_item_permissions_check( $request ) { return true; } + /** + * Checks if a given request has access to update a comment. + * + * Extends core's check so that users who can `edit_post` on the parent + * post are also allowed to update note-type comments — but only for + * suggestion-lifecycle fields (status and `_wp_suggestion_status` meta). + * This unblocks the suggestion workflow where a post editor applies or + * rejects a suggestion authored by someone else, without granting them + * the ability to rewrite the note's content, reassign authorship, or + * otherwise modify another user's comment. + * + * @param WP_REST_Request $request Full details about the request. + * @return true|WP_Error True if the request has access, WP_Error otherwise. + */ + public function update_item_permissions_check( $request ) { + $comment = $this->get_comment( $request['id'] ); + if ( is_wp_error( $comment ) ) { + return $comment; + } + + // For note comments, allow users who can edit the parent post to + // update suggestion-lifecycle fields only. + if ( + 'note' === $comment->comment_type && + self::is_suggestion_lifecycle_update( $request ) + ) { + $post = get_post( $comment->comment_post_ID ); + if ( $post && current_user_can( 'edit_post', $post->ID ) ) { + return true; + } + } + + // Fall back to core's default check (moderate_comments or edit_comment). + return parent::update_item_permissions_check( $request ); + } + + /** + * Determines whether a note-update request touches only the fields used + * by the suggestion apply/reject lifecycle. + * + * Allowed fields: + * - `status` (limited to `approved` or `hold`) + * - `meta._wp_suggestion_status` + * + * Any other field present in the request body disqualifies the request + * from the `edit_post` shortcut, forcing it through core's edit_comment + * check instead. + * + * @param WP_REST_Request $request + * @return bool + */ + private static function is_suggestion_lifecycle_update( $request ) { + // Accept either a JSON body (the block editor client) or a form- + // encoded body (custom integrations / curl scripts). Either way the + // shortcut is gated by the same allowlist below — query/URL params + // are intentionally excluded so the body is the source of truth for + // what's being written. + $params = $request->get_json_params(); + if ( ! is_array( $params ) ) { + $params = $request->get_body_params(); + } + if ( ! is_array( $params ) || empty( $params ) ) { + return false; + } + + $allowed_keys = array( 'id', 'status', 'meta' ); + foreach ( array_keys( $params ) as $key ) { + if ( ! in_array( $key, $allowed_keys, true ) ) { + return false; + } + } + + if ( + isset( $params['status'] ) && + ! in_array( $params['status'], array( 'approved', 'hold' ), true ) + ) { + return false; + } + + if ( isset( $params['meta'] ) ) { + if ( ! is_array( $params['meta'] ) ) { + return false; + } + $allowed_meta = array( '_wp_suggestion_status' ); + foreach ( array_keys( $params['meta'] ) as $meta_key ) { + if ( ! in_array( $meta_key, $allowed_meta, true ) ) { + return false; + } + } + } + + return true; + } + public function create_item_permissions_check( $request ) { $is_note = ! empty( $request['type'] ) && 'note' === $request['type']; @@ -296,6 +390,55 @@ public function create_item_permissions_check( $request ) { return true; } + /** + * Validates that an incoming request's `_wp_suggestion` meta is within the + * allowed byte budget. Truncating arbitrary JSON corrupts the payload, so + * we reject before any storage happens. + * + * @param WP_REST_Request $request Full details about the request. + * @return true|WP_Error True if no payload or within bounds, WP_Error otherwise. + */ + protected static function validate_suggestion_payload_size( $request ) { + $meta = $request['meta'] ?? null; + if ( ! is_array( $meta ) || ! isset( $meta['_wp_suggestion'] ) ) { + return true; + } + $value = $meta['_wp_suggestion']; + if ( ! is_string( $value ) ) { + return true; + } + if ( strlen( $value ) > GUTENBERG_SUGGESTION_PAYLOAD_MAX_BYTES ) { + return new WP_Error( + 'rest_suggestion_too_large', + sprintf( + /* translators: %d: maximum allowed byte length. */ + __( 'Suggestion payload exceeds the %d-byte limit.', 'gutenberg' ), + GUTENBERG_SUGGESTION_PAYLOAD_MAX_BYTES + ), + array( 'status' => 413 ) + ); + } + return true; + } + + /** + * Updates a comment. + * + * Wraps core's update path with a pre-flight size check on the suggestion + * payload so oversized values are rejected with a clean 413 instead of + * silently dropped by the meta sanitize_callback. + * + * @param WP_REST_Request $request Full details about the request. + * @return WP_REST_Response|WP_Error + */ + public function update_item( $request ) { + $size_check = self::validate_suggestion_payload_size( $request ); + if ( is_wp_error( $size_check ) ) { + return $size_check; + } + return parent::update_item( $request ); + } + /** * Creates a comment. * @@ -325,6 +468,14 @@ public function create_item( $request ) { ); } + // Reject oversized suggestion payloads with a 413 so the client knows. + // Without this, the meta sanitize_callback would silently reject the + // value and the suggestion would disappear server-side. + $size_check = self::validate_suggestion_payload_size( $request ); + if ( is_wp_error( $size_check ) ) { + return $size_check; + } + $prepared_comment = $this->prepare_item_for_database( $request ); if ( is_wp_error( $prepared_comment ) ) { return $prepared_comment; @@ -655,5 +806,6 @@ protected function check_is_comment_content_allowed( $prepared_comment ) { function () { $controller = new Gutenberg_REST_Comment_Controller_6_9(); $controller->register_routes(); - } + }, + 11 ); diff --git a/packages/editor/CHANGELOG.md b/packages/editor/CHANGELOG.md index 959e9c764c2384..87242739025f1e 100644 --- a/packages/editor/CHANGELOG.md +++ b/packages/editor/CHANGELOG.md @@ -6,6 +6,7 @@ - Added an `editorIntent` preference (`edit`, `suggest`, `view`) with a matching `setEditorIntent` action and `getEditorIntent` selector. Surfaced as an Edit / Suggest / View menu in the editor options for post types that support notes. The `view` intent puts the block editor into a read-only preview. Keyboard shortcuts follow the Google Docs convention: Ctrl+Alt+Shift+Z (Edit), +X (Suggest), +C (View) on Windows / ⌘⌥⇧Z/X/C on macOS. - Added a suggestion-overlay subsystem that powers the `suggest` intent. When active, an `editor.BlockEdit` filter diverts `setAttributes` into an in-memory overlay keyed by `clientId`; the block renders with the pending change merged on top of its real attributes, but the block-editor store stays at the baseline. A toolbar button on the selected block submits the overlay as a note comment with a `_wp_suggestion` meta payload (`schemaVersion`, `blockName`, `baseRevision`, `operations`) or discards it. Phase 2: capture only; Apply/Reject and diff rendering follow. +- `setEditorIntent` now surfaces mode transitions with a snackbar ('You're suggesting' / 'You're editing' / 'You're viewing') alongside the existing a11y announcement. ## 14.45.0 (2026-04-29) diff --git a/packages/editor/src/components/collab-sidebar/add-note.js b/packages/editor/src/components/collab-sidebar/add-note.js index 2fceb67e20fbc2..b3c7aea629b2d7 100644 --- a/packages/editor/src/components/collab-sidebar/add-note.js +++ b/packages/editor/src/components/collab-sidebar/add-note.js @@ -2,6 +2,7 @@ * WordPress dependencies */ import { __ } from '@wordpress/i18n'; +import { useRef } from '@wordpress/element'; import { useSelect, useDispatch } from '@wordpress/data'; import { store as blockEditorStore, @@ -34,6 +35,7 @@ export function AddNote( { onSubmit, sidebarRef, floating } ) { const blockElement = useBlockElement( clientId ); const { toggleBlockSpotlight } = unlock( useDispatch( blockEditorStore ) ); const { selectNote } = unlock( useDispatch( editorStore ) ); + const isSubmittingRef = useRef( false ); const unselectNote = () => { selectNote( undefined ); @@ -61,6 +63,12 @@ export function AddNote( { onSubmit, sidebarRef, floating } ) { if ( ! document.hasFocus() ) { return; } + // Prevent blur from closing the form while the async submit + // is in progress. Clicking "Add note" moves focus away, + // triggering blur before onSubmit completes. + if ( isSubmittingRef.current ) { + return; + } if ( event.currentTarget.contains( event.relatedTarget ) ) { return; } @@ -71,6 +79,7 @@ export function AddNote( { onSubmit, sidebarRef, floating } ) { { + isSubmittingRef.current = true; const { id } = await onSubmit( { content: inputComment, } ); diff --git a/packages/editor/src/components/collab-sidebar/hooks.js b/packages/editor/src/components/collab-sidebar/hooks.js index 4553cd6b7320de..c23741b4bc820f 100644 --- a/packages/editor/src/components/collab-sidebar/hooks.js +++ b/packages/editor/src/components/collab-sidebar/hooks.js @@ -26,7 +26,12 @@ import { store as editorStore } from '../../store'; import { FLOATING_NOTES_SIDEBAR } from './constants'; import { unlock } from '../../lock-unlock'; import { createBoardStore } from './board-store'; -import { calculateNotePositions } from './utils'; +import { + calculateNotePositions, + getNoteIdsFromMetadata, + addNoteIdToMetadata, + removeNoteIdFromMetadata, +} from './utils'; const { cleanEmptyObject } = unlock( blockEditorPrivateApis ); @@ -59,15 +64,20 @@ export function useNoteThreads( postId ) { return { notes: [], unresolvedNotes: [] }; } - // Single pass over clientIds: build clientId->noteId map AND reverse lookup. + // Single pass over clientIds: build clientId->noteIds map AND reverse lookup. + // Blocks can carry multiple note IDs in their metadata, so the forward map + // stores arrays and the reverse map links each note back to its block. const blocksWithNotes = {}; const clientIdByNoteId = new Map(); for ( const clientId of clientIds ) { - const noteId = getBlockAttributes( clientId )?.metadata?.noteId; - if ( noteId ) { - const key = String( noteId ); - blocksWithNotes[ clientId ] = key; - clientIdByNoteId.set( key, clientId ); + const metadata = getBlockAttributes( clientId )?.metadata; + const noteIds = getNoteIdsFromMetadata( metadata ); + if ( noteIds.length > 0 ) { + const keys = noteIds.map( ( id ) => String( id ) ); + blocksWithNotes[ clientId ] = keys; + for ( const key of keys ) { + clientIdByNoteId.set( key, clientId ); + } } } @@ -101,20 +111,23 @@ export function useNoteThreads( postId ) { return { notes: [], unresolvedNotes: [] }; } - // Single partition over notes-in-block-order. + // Single partition over notes-in-block-order. Each block can have + // multiple note IDs, so iterate the flattened list. const unresolved = []; const resolved = []; - for ( const noteId of Object.values( blocksWithNotes ) ) { - const thread = - threadsById.get( Number( noteId ) ) ?? - threadsById.get( noteId ); - if ( ! thread ) { - continue; - } - if ( thread.status === 'hold' ) { - unresolved.push( thread ); - } else if ( thread.status === 'approved' ) { - resolved.push( thread ); + for ( const noteIds of Object.values( blocksWithNotes ) ) { + for ( const noteId of noteIds ) { + const thread = + threadsById.get( Number( noteId ) ) ?? + threadsById.get( noteId ); + if ( ! thread ) { + continue; + } + if ( thread.status === 'hold' ) { + unresolved.push( thread ); + } else if ( thread.status === 'approved' ) { + resolved.push( thread ); + } } } @@ -170,14 +183,22 @@ export function useNoteActions() { ); // If it's a top-level note, update the block attributes with the note id. + // Read-modify-write on metadata is racy under concurrent edits: + // two near-simultaneous adds against the same base will each write + // a 2-element array and the later write wins, dropping the other + // id. Tracking issue: https://github.com/WordPress/gutenberg/issues/74751. if ( ! parent && savedRecord?.id ) { const clientId = getSelectedBlockClientId(); + if ( ! clientId ) { + return savedRecord; + } const metadata = getBlockAttributes( clientId )?.metadata; + const updatedMetadata = addNoteIdToMetadata( + metadata, + savedRecord.id + ); updateBlockAttributes( clientId, { - metadata: { - ...metadata, - noteId: savedRecord.id, - }, + metadata: cleanEmptyObject( updatedMetadata ), } ); } @@ -267,13 +288,19 @@ export function useNoteActions() { } ); if ( ! note.parent ) { - const clientId = getSelectedBlockClientId(); + // Use blockClientId if available, otherwise fall back to selected block. + const clientId = + note.blockClientId || getSelectedBlockClientId(); + if ( ! clientId ) { + return; + } const metadata = getBlockAttributes( clientId )?.metadata; + const updatedMetadata = removeNoteIdFromMetadata( + metadata, + note.id + ); updateBlockAttributes( clientId, { - metadata: cleanEmptyObject( { - ...metadata, - noteId: undefined, - } ), + metadata: cleanEmptyObject( updatedMetadata ), } ); } diff --git a/packages/editor/src/components/collab-sidebar/index.js b/packages/editor/src/components/collab-sidebar/index.js index d2ad4a717906fc..91b8f71119bb37 100644 --- a/packages/editor/src/components/collab-sidebar/index.js +++ b/packages/editor/src/components/collab-sidebar/index.js @@ -26,6 +26,7 @@ import { AddNoteMenuItem } from './add-note-menu-item'; import { NoteAvatarIndicator } from './note-indicator-toolbar'; import { useGlobalStylesContext } from '../global-styles-provider'; import { useNoteThreads, useEnableFloatingSidebar } from './hooks'; +import { getNoteIdsFromMetadata } from './utils'; import PostTypeSupportCheck from '../post-type-support-check'; import { unlock } from '../../lock-unlock'; @@ -53,6 +54,8 @@ function NotesSidebar( { postId } ) { : false, }; }, [] ); + + const blockNoteIds = getNoteIdsFromMetadata( { noteId } ); const { isDistractionFree } = useSelect( ( select ) => { const { get } = select( preferencesStore ); return { @@ -82,10 +85,7 @@ function NotesSidebar( { postId } ) { openTheSidebar(); }, { - // When multiple notes per block are supported. Remove note ID check. - // See: https://github.com/WordPress/gutenberg/pull/75147. - isDisabled: - isDistractionFree || isClassicBlock || ! clientId || !! noteId, + isDisabled: isDistractionFree || isClassicBlock || ! clientId, } ); @@ -93,23 +93,46 @@ function NotesSidebar( { postId } ) { const { merged: GlobalStyles } = useGlobalStylesContext(); const backgroundColor = GlobalStyles?.styles?.color?.background; - // Find the current thread for the selected block. - const currentThread = noteId - ? notes.find( ( thread ) => thread.id === noteId ) - : null; + // Find threads for the selected block. Blocks can carry multiple note IDs, + // so gather all matching threads and surface the most relevant one + // (first unresolved, else first) for UI interactions like avatars. + const currentThreads = + blockNoteIds.length > 0 + ? notes.filter( ( thread ) => blockNoteIds.includes( thread.id ) ) + : []; + const currentThread = + currentThreads.find( ( thread ) => thread.status === 'hold' ) ?? + currentThreads[ 0 ] ?? + null; + + async function openTheSidebar( { + addNewNote = false, + clientId: explicitClientId, + } = {} ) { + // `AddNoteMenuItem` (a slot fill rendered per block in the List + // View row menus) passes the row's clientId, which may differ + // from the canvas selection. Fall back to the canvas selection + // for the keyboard shortcut and avatar indicator paths. + const targetClientId = explicitClientId ?? clientId; + if ( ! targetClientId ) { + return; + } + + // Look up threads for the target block directly so the List View + // path resolves the right notes even when the canvas selection + // is somewhere else. + const targetThreads = notes.filter( + ( thread ) => thread.blockClientId === targetClientId + ); + const targetThread = + targetThreads.find( ( thread ) => thread.status === 'hold' ) ?? + targetThreads[ 0 ] ?? + null; - async function openTheSidebar( selectedClientId ) { const prevArea = await getActiveComplementaryArea( 'core' ); const activeNotesArea = SIDEBARS.find( ( name ) => name === prevArea ); - const targetClientId = - selectedClientId && selectedClientId !== clientId - ? selectedClientId - : clientId; - const targetNote = notes.find( - ( note ) => note.blockClientId === targetClientId - ); - if ( targetNote?.status === 'approved' ) { + if ( targetThread?.status === 'approved' && ! addNewNote ) { enableComplementaryArea( 'core', ALL_NOTES_SIDEBAR ); } else if ( ! activeNotesArea || ! showAllNotesSidebar ) { enableComplementaryArea( @@ -124,11 +147,17 @@ function NotesSidebar( { postId } ) { return; } - // A special case for the List View, where block selection isn't required to trigger an action. - // The action won't do anything if the block is already selected. + // When addNewNote is true, always open the new note form. + // Otherwise, select the existing thread or open new. + const shouldAddNew = addNewNote || ! targetThread; + // A special case for the List View, where block selection isn't + // required to trigger the action. The action is a no-op when the + // block is already selected. selectBlock( targetClientId, null ); toggleBlockSpotlight( targetClientId, true ); - selectNote( targetNote ? targetNote.id : 'new', { focus: true } ); + selectNote( shouldAddNew ? 'new' : targetThread.id, { + focus: true, + } ); } if ( isDistractionFree ) { @@ -140,10 +169,17 @@ function NotesSidebar( { postId } ) { { !! currentThread && ( openTheSidebar() } /> ) } - + + openTheSidebar( { + addNewNote: true, + clientId: menuClientId, + } ) + } + /> { showAllNotesSidebar && ( + { hasSuggestionPayload && ( + + ) } { isSelected && canResolve && onResolve && ( - - - ) } - { showStaleDialog && ( - { - setShowStaleDialog( false ); - runApply(); - } } - onCancel={ () => setShowStaleDialog( false ) } - confirmButtonText={ __( 'Apply anyway' ) } - > - { __( - 'The post has changed since this suggestion was made. Applying it may produce unexpected results. Continue?' - ) } - + { ! isResolved && applyDisabledReason && ( + + { applyDisabledReason } + ) } ); diff --git a/packages/editor/src/components/collab-sidebar/test/utils.js b/packages/editor/src/components/collab-sidebar/test/utils.js index f4b4f92d02607d..71efc43124b1fc 100644 --- a/packages/editor/src/components/collab-sidebar/test/utils.js +++ b/packages/editor/src/components/collab-sidebar/test/utils.js @@ -1,12 +1,200 @@ /** * Internal dependencies */ -import { calculateNotePositions } from '../utils'; +import { + getNoteIdsFromMetadata, + addNoteIdToMetadata, + removeNoteIdFromMetadata, + calculateNotePositions, +} from '../utils'; function makeRect( top ) { return { top }; } +describe( 'getNoteIdsFromMetadata', () => { + it( 'returns empty array for null metadata', () => { + expect( getNoteIdsFromMetadata( null ) ).toEqual( [] ); + } ); + + it( 'returns empty array for undefined metadata', () => { + expect( getNoteIdsFromMetadata( undefined ) ).toEqual( [] ); + } ); + + it( 'returns empty array for metadata without noteId', () => { + expect( getNoteIdsFromMetadata( {} ) ).toEqual( [] ); + expect( getNoteIdsFromMetadata( { name: 'test' } ) ).toEqual( [] ); + } ); + + it( 'returns empty array for noteId of 0', () => { + expect( getNoteIdsFromMetadata( { noteId: 0 } ) ).toEqual( [] ); + } ); + + it( 'returns empty array for noteId of empty string', () => { + expect( getNoteIdsFromMetadata( { noteId: '' } ) ).toEqual( [] ); + } ); + + it( 'returns empty array for noteId of false', () => { + expect( getNoteIdsFromMetadata( { noteId: false } ) ).toEqual( [] ); + } ); + + it( 'returns array from scalar noteId (legacy format)', () => { + expect( getNoteIdsFromMetadata( { noteId: 42 } ) ).toEqual( [ 42 ] ); + } ); + + it( 'handles string noteId (legacy format)', () => { + expect( getNoteIdsFromMetadata( { noteId: '42' } ) ).toEqual( [ + '42', + ] ); + } ); + + it( 'returns array from array noteId', () => { + expect( getNoteIdsFromMetadata( { noteId: [ 1, 2, 3 ] } ) ).toEqual( [ + 1, 2, 3, + ] ); + } ); + + it( 'filters out falsy values from array', () => { + expect( + getNoteIdsFromMetadata( { noteId: [ 1, null, 2, undefined, 3 ] } ) + ).toEqual( [ 1, 2, 3 ] ); + } ); + + it( 'filters out zero and empty string from array', () => { + expect( + getNoteIdsFromMetadata( { noteId: [ 0, '', 1, false, 2 ] } ) + ).toEqual( [ 1, 2 ] ); + } ); + + it( 'returns empty array when all array values are falsy', () => { + expect( + getNoteIdsFromMetadata( { noteId: [ null, undefined, 0, '' ] } ) + ).toEqual( [] ); + } ); +} ); + +describe( 'addNoteIdToMetadata', () => { + it( 'creates array for first note on empty metadata', () => { + const result = addNoteIdToMetadata( {}, 42 ); + expect( result.noteId ).toEqual( [ 42 ] ); + } ); + + it( 'creates array for first note on null metadata', () => { + const result = addNoteIdToMetadata( null, 42 ); + expect( result.noteId ).toEqual( [ 42 ] ); + } ); + + it( 'creates array for first note on undefined metadata', () => { + const result = addNoteIdToMetadata( undefined, 42 ); + expect( result.noteId ).toEqual( [ 42 ] ); + } ); + + it( 'converts scalar noteId to array and appends new id', () => { + const result = addNoteIdToMetadata( { noteId: 1 }, 2 ); + expect( result.noteId ).toEqual( [ 1, 2 ] ); + } ); + + it( 'appends to existing array', () => { + const result = addNoteIdToMetadata( { noteId: [ 1, 2 ] }, 3 ); + expect( result.noteId ).toEqual( [ 1, 2, 3 ] ); + } ); + + it( 'prevents duplicates', () => { + const result = addNoteIdToMetadata( { noteId: [ 1, 2 ] }, 1 ); + expect( result ).toEqual( { noteId: [ 1, 2 ] } ); + } ); + + it( 'preserves other metadata properties', () => { + const result = addNoteIdToMetadata( { noteId: 1, name: 'test' }, 2 ); + expect( result ).toEqual( { noteId: [ 1, 2 ], name: 'test' } ); + } ); + + it( 'returns original metadata object when duplicate is added', () => { + const metadata = { noteId: [ 1, 2 ] }; + const result = addNoteIdToMetadata( metadata, 1 ); + expect( result ).toBe( metadata ); + } ); + + it( 'handles adding to metadata with other properties but no noteId', () => { + const result = addNoteIdToMetadata( { name: 'test' }, 5 ); + expect( result ).toEqual( { name: 'test', noteId: [ 5 ] } ); + } ); + + it( 'dedupes a numeric id against a string-typed legacy id', () => { + const metadata = { noteId: '5' }; + const result = addNoteIdToMetadata( metadata, 5 ); + expect( result ).toBe( metadata ); + } ); + + it( 'dedupes a string id against a numeric id already in the array', () => { + const result = addNoteIdToMetadata( { noteId: [ 1, 2 ] }, '2' ); + expect( result ).toEqual( { noteId: [ 1, 2 ] } ); + } ); +} ); + +describe( 'removeNoteIdFromMetadata', () => { + it( 'removes noteId from array', () => { + const result = removeNoteIdFromMetadata( { noteId: [ 1, 2, 3 ] }, 2 ); + expect( result.noteId ).toEqual( [ 1, 3 ] ); + } ); + + it( 'returns undefined noteId when array becomes empty', () => { + const result = removeNoteIdFromMetadata( { noteId: [ 1 ] }, 1 ); + expect( result.noteId ).toBeUndefined(); + } ); + + it( 'handles removing from scalar noteId (legacy format)', () => { + const result = removeNoteIdFromMetadata( { noteId: 42 }, 42 ); + expect( result.noteId ).toBeUndefined(); + } ); + + it( 'handles removing non-existent id', () => { + const result = removeNoteIdFromMetadata( { noteId: [ 1, 2 ] }, 99 ); + expect( result.noteId ).toEqual( [ 1, 2 ] ); + } ); + + it( 'handles empty metadata', () => { + const result = removeNoteIdFromMetadata( {}, 1 ); + expect( result.noteId ).toBeUndefined(); + } ); + + it( 'preserves other metadata properties', () => { + const result = removeNoteIdFromMetadata( + { noteId: [ 1, 2 ], name: 'test' }, + 1 + ); + expect( result ).toEqual( { noteId: [ 2 ], name: 'test' } ); + } ); + + it( 'handles null metadata', () => { + const result = removeNoteIdFromMetadata( null, 1 ); + expect( result.noteId ).toBeUndefined(); + } ); + + it( 'handles undefined metadata', () => { + const result = removeNoteIdFromMetadata( undefined, 1 ); + expect( result.noteId ).toBeUndefined(); + } ); + + it( 'removes last note and cleans up noteId to undefined', () => { + const result = removeNoteIdFromMetadata( + { noteId: [ 42 ], name: 'test' }, + 42 + ); + expect( result ).toEqual( { name: 'test', noteId: undefined } ); + } ); + + it( 'removes a numeric id when stored as a string-typed legacy scalar', () => { + const result = removeNoteIdFromMetadata( { noteId: '42' }, 42 ); + expect( result.noteId ).toBeUndefined(); + } ); + + it( 'removes a string id when stored as a number in the array', () => { + const result = removeNoteIdFromMetadata( { noteId: [ 1, 2, 3 ] }, '2' ); + expect( result.noteId ).toEqual( [ 1, 3 ] ); + } ); +} ); + describe( 'calculateNotePositions', () => { it( 'returns empty positions when the anchor thread has no blockRect', () => { const { positions } = calculateNotePositions( { @@ -152,4 +340,69 @@ describe( 'calculateNotePositions', () => { // 2: 300 + 500 - 16 = 784 expect( positions ).toEqual( { 1: 584, 2: 784 } ); } ); + + it( 'stacks two threads that share a block with the first as anchor', () => { + const threads = [ { id: 1 }, { id: 2 } ]; + const blockRects = { + 1: makeRect( 200 ), + 2: makeRect( 200 ), + }; + const heights = { 1: 50, 2: 50 }; + + const { positions } = calculateNotePositions( { + threads, + selectedNoteId: 1, + blockRects, + heights, + scrollTop: 0, + } ); + + // 1 (anchor): 200 - 16 = 184 + // 2 (downward): (184 + 50) - 200 + 20 = 54 → 200 + 54 = 254 + expect( positions ).toEqual( { 1: 184, 2: 254 } ); + } ); + + it( 'stacks two threads that share a block with the second as anchor', () => { + const threads = [ { id: 1 }, { id: 2 } ]; + const blockRects = { + 1: makeRect( 200 ), + 2: makeRect( 200 ), + }; + const heights = { 1: 50, 2: 50 }; + + const { positions } = calculateNotePositions( { + threads, + selectedNoteId: 2, + blockRects, + heights, + scrollTop: 0, + } ); + + // 2 (anchor): 200 - 16 = 184 + // 1 (upward): 184 - 200 - 50 - 20 = -86 → 200 + (-86) = 114 + expect( positions ).toEqual( { 1: 114, 2: 184 } ); + } ); + + it( 'stacks three threads sharing a block with middle as anchor', () => { + const threads = [ { id: 1 }, { id: 2 }, { id: 3 } ]; + const blockRects = { + 1: makeRect( 200 ), + 2: makeRect( 200 ), + 3: makeRect( 200 ), + }; + const heights = { 1: 50, 2: 50, 3: 50 }; + + const { positions } = calculateNotePositions( { + threads, + selectedNoteId: 2, + blockRects, + heights, + scrollTop: 0, + } ); + + // 2 (anchor): 200 - 16 = 184 + // 3 (downward): (184 + 50) - 200 + 20 = 54 → 254 + // 1 (upward): 184 - 200 - 50 - 20 = -86 → 114 + expect( positions ).toEqual( { 1: 114, 2: 184, 3: 254 } ); + } ); } ); diff --git a/packages/editor/src/components/collab-sidebar/utils.js b/packages/editor/src/components/collab-sidebar/utils.js index 0b49bfcfb0b498..a3eafb102b8aa9 100644 --- a/packages/editor/src/components/collab-sidebar/utils.js +++ b/packages/editor/src/components/collab-sidebar/utils.js @@ -90,6 +90,80 @@ export function getNoteExcerpt( text, excerptLength = 10 ) { return isTrimmed ? trimmedExcerpt + '…' : trimmedExcerpt; } +/* + * Multi-noteId helpers + * + * Block notes were originally a single linkage: `metadata.noteId` was a + * scalar comment id. PR #75147 widened that to an array so a block can + * carry multiple coexisting notes (e.g. a resolved suggestion plus a + * pending discussion thread). The helpers below normalize both shapes — + * existing posts written before the migration still have scalar values — + * so callers never have to branch on the storage format. Add new accessors + * here rather than reading `metadata.noteId` directly, both to preserve + * compatibility and to handle the string/numeric id mismatch that legacy + * data sometimes contains. + */ + +/** + * Normalizes noteId metadata to always return an array. + * Handles both scalar (legacy) and array (new) noteId values. + * + * @param {Object} metadata Block metadata object + * @return {number[]} Array of note IDs (may be empty) + */ +export function getNoteIdsFromMetadata( metadata ) { + if ( ! metadata || metadata.noteId === null ) { + return []; + } + // New format: noteId is an array. + if ( Array.isArray( metadata.noteId ) ) { + return metadata.noteId.filter( Boolean ); + } + // Legacy format: noteId is a scalar. + return metadata.noteId ? [ metadata.noteId ] : []; +} + +/** + * Adds a note ID to the metadata. + * Converts scalar to array if needed, otherwise appends. + * + * @param {Object} metadata Existing block metadata + * @param {number} noteId Note ID to add + * @return {Object} Updated metadata object + */ +export function addNoteIdToMetadata( metadata, noteId ) { + const existingIds = getNoteIdsFromMetadata( metadata ); + // Compare as strings so a string-typed legacy id (e.g. '5') and a numeric + // id (5) are treated as duplicates. + const noteIdKey = String( noteId ); + if ( existingIds.some( ( id ) => String( id ) === noteIdKey ) ) { + return metadata; + } + return { + ...metadata, + noteId: [ ...existingIds, noteId ], + }; +} + +/** + * Removes a note ID from the metadata. + * + * @param {Object} metadata Existing block metadata + * @param {number} noteId Note ID to remove + * @return {Object} Updated metadata object + */ +export function removeNoteIdFromMetadata( metadata, noteId ) { + const existingIds = getNoteIdsFromMetadata( metadata ); + // Compare as strings so a string-typed legacy id (e.g. '5') matches a + // numeric id (5) when removing. + const noteIdKey = String( noteId ); + const newIds = existingIds.filter( ( id ) => String( id ) !== noteIdKey ); + return { + ...metadata, + noteId: newIds.length > 0 ? newIds : undefined, + }; +} + /** * Calculate final top positions for all floating note threads in the * editor's content coordinate space. Adjusts positions to prevent overlapping diff --git a/packages/editor/src/components/provider/use-block-editor-settings.js b/packages/editor/src/components/provider/use-block-editor-settings.js index a77ef4000c4c9a..18a9fdb656cf6b 100644 --- a/packages/editor/src/components/provider/use-block-editor-settings.js +++ b/packages/editor/src/components/provider/use-block-editor-settings.js @@ -158,9 +158,11 @@ function useBlockEditorSettings( settings, postType, postId, renderingMode ) { } = select( coreStore ); const { get } = select( preferencesStore ); const { getBlockTypes } = select( blocksStore ); - const { getDeviceType, isRevisionsMode: _isRevisionsMode } = unlock( - select( editorStore ) - ); + const { + getDeviceType, + isRevisionsMode: _isRevisionsMode, + getEditorIntent, + } = unlock( select( editorStore ) ); const { getBlocksByName, getBlockAttributes } = select( blockEditorStore ); const siteSettings = canUser( 'read', { @@ -204,7 +206,7 @@ function useBlockEditorSettings( settings, postType, postId, renderingMode ) { get( 'core', 'fixedToolbar' ) || ! isLargeViewport, hiddenBlockTypes: get( 'core', 'hiddenBlockTypes' ), isDistractionFree: get( 'core', 'distractionFree' ), - isViewIntent: get( 'core', 'editorIntent' ) === 'view', + isViewIntent: getEditorIntent() === 'view', keepCaretInsideBlock: get( 'core', 'keepCaretInsideBlock' ), hasUploadPermissions: canUser( 'create', { diff --git a/packages/editor/src/components/suggestion-mode/index.js b/packages/editor/src/components/suggestion-mode/index.js index f204900159a6d9..52e389d570d6e4 100644 --- a/packages/editor/src/components/suggestion-mode/index.js +++ b/packages/editor/src/components/suggestion-mode/index.js @@ -13,9 +13,14 @@ export { useSuggestionsProvider, operationsFromOverlay, applyOperations, + hasAttributeConflict, parseSuggestionPayload, payloadByteLength, PAYLOAD_MAX_BYTES, SCHEMA_VERSION, } from './provider'; export { default as SuggestionDiff, wordDiff } from './suggestion-diff'; +export { + default as SuggestionSummary, + summarizeOperations, +} from './suggestion-summary'; diff --git a/packages/editor/src/components/suggestion-mode/provider.js b/packages/editor/src/components/suggestion-mode/provider.js index c386ce4ab345d6..4ea2cc50c8666b 100644 --- a/packages/editor/src/components/suggestion-mode/provider.js +++ b/packages/editor/src/components/suggestion-mode/provider.js @@ -13,6 +13,10 @@ import { __ } from '@wordpress/i18n'; */ import { EDITOR_STORE_NAME } from './constants'; import { useSuggestionOverlay } from './overlay-context'; +import { + addNoteIdToMetadata, + getNoteIdsFromMetadata, +} from '../collab-sidebar/utils'; /** * @typedef {Object} SuggestionOperation @@ -112,7 +116,9 @@ function isAttributeEqual( a, b ) { // suggestion payload) and the other is a wrapper object (typically a // `RichTextData` instance from the live block-editor store). Compare // their string representations so the same logical content reads as - // equal across the serialization boundary. + // equal across the serialization boundary — otherwise `hasAttributeConflict` + // flags every content suggestion as stale and the apply flow short- + // circuits to a never-visible "stale" dialog. const aIsObject = typeof a === 'object'; const bIsObject = typeof b === 'object'; if ( aIsObject !== bIsObject ) { @@ -178,6 +184,37 @@ export function applyOperations( currentAttributes, operations ) { return result; } +/** + * Report whether applying the suggestion's operations over the block's + * current attributes would overwrite concurrent changes made by someone + * else. A suggestion is considered conflicting only when the baseline + * captured at suggest-time differs from the attribute's current value — + * simply reopening the post after any auto-save doesn't qualify. + * + * @param {Object} currentAttributes Block's current attributes. + * @param {SuggestionOperation[]} operations Operations from the payload. + * @return {boolean} True if at least one targeted attribute has diverged. + */ +export function hasAttributeConflict( currentAttributes, operations ) { + if ( ! Array.isArray( operations ) ) { + return false; + } + for ( const op of operations ) { + if ( op.type !== 'attribute-set' ) { + continue; + } + if ( + ! isAttributeEqual( + op.before ?? null, + currentAttributes?.[ op.attribute ] ?? null + ) + ) { + return true; + } + } + return false; +} + /** * Parse a `_wp_suggestion` meta value into a typed payload. * @@ -239,8 +276,10 @@ export function useSuggestionsProvider() { const { saveEntityRecord } = useDispatch( coreStore ); const { createNotice } = useDispatch( noticesStore ); const { updateBlockAttributes } = useDispatch( blockEditorStore ); - const { getBlockAttributes: selectBlockAttributes } = - useSelect( blockEditorStore ); + const { + getBlockAttributes: selectBlockAttributes, + getClientIdsWithDescendants: selectClientIdsWithDescendants, + } = useSelect( blockEditorStore ); const { requestInterceptorBypass, clearOverlay } = useSuggestionOverlay(); const createSuggestion = useCallback( @@ -288,23 +327,21 @@ export function useSuggestionsProvider() { ); if ( savedRecord?.id ) { - // Merge into existing metadata rather than replacing so - // other fields like bindings, name, and block identifiers - // are preserved. + // Append to the noteId array so a fresh suggestion on a + // block whose previous note(s) have been applied or + // rejected coexists with them rather than overwriting + // the link. Other metadata fields like bindings and name + // are preserved by `addNoteIdToMetadata`. const existingMeta = selectBlockAttributes( clientId )?.metadata ?? {}; updateBlockAttributes( clientId, { - metadata: { - ...existingMeta, - noteId: savedRecord.id, - }, + metadata: addNoteIdToMetadata( + existingMeta, + savedRecord.id + ), } ); } - createNotice( 'success', __( 'Suggestion submitted.' ), { - type: 'snackbar', - isDismissible: true, - } ); return savedRecord; } catch ( error ) { createNotice( @@ -335,7 +372,12 @@ export function useSuggestionsProvider() { * suggestion (`_wp_suggestion` * meta). * @param {string} args.clientId Block client id of the apply - * target. + * target. May be undefined if + * the acting user opened the + * post fresh and the metadata + * linkage was never persisted — + * the apply path then scans the + * live tree by `metadata.noteId`. * @param {SuggestionPayload} args.payload Parsed payload (from * `parseSuggestionPayload`). * @return {Promise} @@ -350,21 +392,42 @@ export function useSuggestionsProvider() { return; } - if ( - payload.baseRevision && - postModified && - payload.baseRevision !== postModified - ) { + // `thread.blockClientId` is derived by matching `metadata.noteId` + // on blocks currently in the editor. If the Suggest author never + // auto-saved the post after the comment was created — or the + // author reloaded before the save landed — the metadata linkage + // won't exist yet and the caller will pass `clientId: undefined`. + // Fall back to scanning the live block tree for a block whose + // `metadata.noteId` includes the comment id (the field is an + // array post-#75147 to support multiple notes per block, so use + // the shared normalization helper instead of strict equality). + let targetClientId = clientId; + if ( ! targetClientId ) { + const liveIds = selectClientIdsWithDescendants?.() ?? []; + const commentIdKey = String( commentId ); + for ( const id of liveIds ) { + const ids = getNoteIdsFromMetadata( + selectBlockAttributes( id )?.metadata + ); + if ( ids.some( ( n ) => String( n ) === commentIdKey ) ) { + targetClientId = id; + break; + } + } + } + + if ( ! targetClientId ) { createNotice( - 'warning', + 'error', __( - 'Post content has changed since this suggestion. Review carefully.' + 'Could not find the block this suggestion applies to.' ), { type: 'snackbar', isDismissible: true } ); + return; } - const currentAttributes = selectBlockAttributes( clientId ); + const currentAttributes = selectBlockAttributes( targetClientId ); const newAttributes = applyOperations( currentAttributes, payload.operations @@ -400,9 +463,9 @@ export function useSuggestionsProvider() { // so any subsequent user edit captures a fresh baseline // from the post-apply attributes. Outside Suggest mode the // interceptor isn't running and these calls are no-ops. - requestInterceptorBypass( clientId ); - clearOverlay( clientId ); - updateBlockAttributes( clientId, newAttributes ); + requestInterceptorBypass( targetClientId ); + clearOverlay( targetClientId ); + updateBlockAttributes( targetClientId, newAttributes ); await saveEntityRecord( 'root', @@ -422,8 +485,8 @@ export function useSuggestionsProvider() { } catch ( error ) { // Roll back the block change so the UI isn't left in a // half-applied state if the server rejected the update. - requestInterceptorBypass( clientId ); - updateBlockAttributes( clientId, rollbackPayload ); + requestInterceptorBypass( targetClientId ); + updateBlockAttributes( targetClientId, rollbackPayload ); createNotice( 'error', error?.message || __( 'Failed to save suggestion status.' ), @@ -432,10 +495,10 @@ export function useSuggestionsProvider() { } }, [ - postModified, saveEntityRecord, updateBlockAttributes, selectBlockAttributes, + selectClientIdsWithDescendants, createNotice, requestInterceptorBypass, clearOverlay, @@ -481,7 +544,11 @@ export function useSuggestionsProvider() { [ saveEntityRecord, createNotice ] ); - return { createSuggestion, applySuggestion, rejectSuggestion }; + return { + createSuggestion, + applySuggestion, + rejectSuggestion, + }; } export { SCHEMA_VERSION, PAYLOAD_MAX_BYTES, payloadByteLength }; diff --git a/packages/editor/src/components/suggestion-mode/style.scss b/packages/editor/src/components/suggestion-mode/style.scss new file mode 100644 index 00000000000000..198ea4b0941a25 --- /dev/null +++ b/packages/editor/src/components/suggestion-mode/style.scss @@ -0,0 +1,92 @@ +@use "@wordpress/base-styles/colors" as *; +@use "@wordpress/base-styles/variables" as *; + +// Green "bracket" styling applied to any block with a pending suggestion +// overlay while the editor is in Suggest mode. Drawn with an outline so it +// layers above block content without affecting layout. Matches Docs' review +// affordance: the block is visibly edited without hiding its normal UI. +$suggestion-color: #007017; + +.block-editor-block-list__block.is-suggestion-pending { + position: relative; + outline: 2px solid $suggestion-color; + outline-offset: 2px; + border-radius: $radius-small; + + &::before, + &::after { + content: ""; + position: absolute; + width: $grid-unit-10; + height: $grid-unit-10; + border: 2px solid $suggestion-color; + pointer-events: none; + } + + &::before { + top: -$grid-unit-05; + left: -$grid-unit-05; + border-right: 0; + border-bottom: 0; + } + + &::after { + bottom: -$grid-unit-05; + right: -$grid-unit-05; + border-left: 0; + border-top: 0; + } +} + +.editor-collab-sidebar-panel__suggestion-summary { + em { + font-style: italic; + color: $gray-700; + } +} + +// Inline diff colors for the suggestion preview. Pulled from the design +// system fallbacks so the rendering still degrades gracefully on themes +// without `--wp-admin-theme-color` defined. +.editor-collab-sidebar-panel__suggestion-text-diff { + del { + color: var(--wp-block-synced-color, #cc1818); + text-decoration: line-through; + } + + ins { + color: var(--wp-admin-theme-color, #007017); + text-decoration: underline; + } +} + +// Inline accept/reject buttons live in the note header, right-aligned next +// to the author info. Force both icons to render at the same box so the +// checkmark (a thin glyph) visually matches the close glyph next to it, +// and thicken the check stroke so it reads at a similar visual weight to +// the denser close glyph. +.editor-collab-sidebar-panel__suggestion-header-actions { + margin-left: auto; + + .components-button.has-icon { + width: $grid-unit-40; + height: $grid-unit-40; + min-width: $grid-unit-40; + padding: 0; + + svg { + width: $grid-unit-40; + height: $grid-unit-40; + } + + // The check path in @wordpress/icons is drawn with a lighter weight + // than the close glyph, so pair the filled path with a 1px stroke to + // lift it to the same visual density. + &[aria-label="Accept suggestion"] svg path { + stroke: currentColor; + stroke-width: 1; + stroke-linecap: round; + stroke-linejoin: round; + } + } +} diff --git a/packages/editor/src/components/suggestion-mode/suggestion-diff.js b/packages/editor/src/components/suggestion-mode/suggestion-diff.js index 87aadb9654e9bb..5a5189b1dd2c2d 100644 --- a/packages/editor/src/components/suggestion-mode/suggestion-diff.js +++ b/packages/editor/src/components/suggestion-mode/suggestion-diff.js @@ -181,13 +181,7 @@ function TextDiff( { before, after } ) { { segments.map( ( seg, i ) => { if ( seg.type === 'delete' ) { return ( - + { __( 'Deleted:' ) } @@ -197,13 +191,7 @@ function TextDiff( { before, after } ) { } if ( seg.type === 'insert' ) { return ( - + { __( 'Inserted:' ) } diff --git a/packages/editor/src/components/suggestion-mode/suggestion-summary.js b/packages/editor/src/components/suggestion-mode/suggestion-summary.js new file mode 100644 index 00000000000000..9bc070b5b65c44 --- /dev/null +++ b/packages/editor/src/components/suggestion-mode/suggestion-summary.js @@ -0,0 +1,318 @@ +/** + * Sidebar summary of a suggestion's operations. + * + * Companion to `suggestion-diff.js` — where SuggestionDiff is a full inline + * diff (used in the comment thread body), SuggestionSummary is a one-or-two + * line precis suitable for the collapsed sidebar list. It produces three + * line categories from the operations array: + * + * - **Add: …** — new text inserted by the suggestion. For text-valued + * attributes the inserted words are extracted from the + * word-level diff; long insertions are truncated to + * `SUMMARY_MAX_CHARS` with an ellipsis. + * - **Delete: …** — text removed by the suggestion, again derived from + * the word diff. + * - **Format: …** — non-text attribute changes. Uses + * `FORMAT_ATTRIBUTE_LABELS` to surface friendly names + * (e.g. `level` → "heading level") with a fallback to + * the raw attribute name so a brand-new attribute + * isn't silently swallowed. + * - **Formatting:** — pure inline-format changes (bold, italic, links). + * Detected by tag-level diff of the serialized HTML + * and de-duplicated by `joinLabels` so multiple span + * edits don't list the same format twice. + */ +/** + * WordPress dependencies + */ +import { __ } from '@wordpress/i18n'; +import { __experimentalText as WCText } from '@wordpress/components'; +import { Stack } from '@wordpress/ui'; +import { useMemo } from '@wordpress/element'; +import { __unstableStripHTML as wpStripHTML } from '@wordpress/dom'; + +/** + * Internal dependencies + */ +import { wordDiff } from './suggestion-diff'; + +/** + * Cap on how much text we'll render inline in a summary. Longer insertions + * or deletions are ellipsized so the comment thread stays readable. + */ +const SUMMARY_MAX_CHARS = 120; + +/** + * Friendlier labels for common block attributes so `Format:` lines read like + * human categories rather than internal names. Anything not in this map + * falls through to the raw attribute name. + */ +const FORMAT_ATTRIBUTE_LABELS = { + level: __( 'heading level' ), + align: __( 'alignment' ), + textAlign: __( 'text alignment' ), + fontSize: __( 'font size' ), + style: __( 'style' ), + url: __( 'link' ), + href: __( 'link' ), + backgroundColor: __( 'background color' ), + textColor: __( 'text color' ), +}; + +/** + * Mapping of inline HTML tags — as emitted by RichText serialization — to + * human-readable format names. The key is the lower-cased tag name; the + * value is what appears in a "Formatting:" line. Tags not in this map are + * reported by their raw name (```` → "mark") so a future rich-text + * format isn't silently swallowed. + */ +const INLINE_FORMAT_TAG_LABELS = { + strong: __( 'bold' ), + b: __( 'bold' ), + em: __( 'italic' ), + i: __( 'italic' ), + u: __( 'underline' ), + s: __( 'strikethrough' ), + del: __( 'strikethrough' ), + strike: __( 'strikethrough' ), + code: __( 'code' ), + mark: __( 'highlight' ), + a: __( 'link' ), + sub: __( 'subscript' ), + sup: __( 'superscript' ), + kbd: __( 'keyboard' ), +}; + +const TAG_REGEX = /<\s*([a-zA-Z][a-zA-Z0-9]*)\b[^>]*>/g; + +/** + * Strip HTML tags AND decode entities, leaving only the visible text. Used + * to decide whether a content change is purely a formatting change (same + * visible text wrapped in different markup) or a real text edit. + * + * Wraps `__unstableStripHTML` so HTML entities such as `&` and ` ` + * are decoded by the DOM parser rather than via a hand-rolled regex — a + * regex-only strip would mis-classify text edits where one side encoded an + * ampersand and the other didn't. + * + * @param {string} html Possibly-HTML content. + * @return {string} The visible text, with whitespace collapsed. + */ +function stripTags( html ) { + if ( typeof html !== 'string' || html === '' ) { + return ''; + } + return wpStripHTML( html ).replace( /\s+/g, ' ' ).trim(); +} + +/** + * Count occurrences of each opening tag in an HTML string. Self-closing + * and void tags (e.g. `
`) are counted the same way as paired tags + * since we only care about presence, not balance. + * + * @param {string} html Possibly-HTML content. + * @return {Map} Tag name → count. + */ +function countTags( html ) { + const counts = new Map(); + let match; + TAG_REGEX.lastIndex = 0; + while ( ( match = TAG_REGEX.exec( html ) ) !== null ) { + const name = match[ 1 ].toLowerCase(); + counts.set( name, ( counts.get( name ) ?? 0 ) + 1 ); + } + return counts; +} + +/** + * Diff the tag usage between two HTML strings. Returns a set of format + * names (mapped via `INLINE_FORMAT_TAG_LABELS`) whose count differs, which + * indicates an inline format was added or removed regardless of direction. + * + * @param {string} before HTML before the edit. + * @param {string} after HTML after the edit. + * @return {string[]} Ordered, deduplicated list of changed format labels. + */ +function diffInlineFormats( before, after ) { + const beforeCounts = countTags( before ); + const afterCounts = countTags( after ); + const changed = new Set(); + const tags = new Set( [ ...beforeCounts.keys(), ...afterCounts.keys() ] ); + for ( const tag of tags ) { + if ( + ( beforeCounts.get( tag ) ?? 0 ) === ( afterCounts.get( tag ) ?? 0 ) + ) { + continue; + } + const label = INLINE_FORMAT_TAG_LABELS[ tag ] ?? tag; + changed.add( label ); + } + return Array.from( changed ); +} + +/** + * Join an array of label strings with a comma, using `__()`-friendly + * punctuation. Deduplicated and lowercased for display. + * + * @param {string[]} labels Raw labels. + * @return {string} Comma-joined list. + */ +function joinLabels( labels ) { + const unique = Array.from( + new Set( labels.filter( Boolean ).map( ( l ) => l.toLowerCase() ) ) + ); + return unique.join( ', ' ); +} + +function ellipsize( text ) { + const trimmed = text.replace( /\s+/g, ' ' ).trim(); + if ( trimmed.length <= SUMMARY_MAX_CHARS ) { + return trimmed; + } + return `${ trimmed.slice( 0, SUMMARY_MAX_CHARS - 1 ).trimEnd() }…`; +} + +/** + * Derive the inserted and deleted text spans from a pair of before/after + * strings by running the shared word-level diff and concatenating matching + * segments. Whitespace-only runs are excluded from the counts so a pure + * format change doesn't surface as "Add: ' '". + * + * @param {string} before Original text. + * @param {string} after Proposed text. + * @return {{inserted: string, deleted: string}} Aggregated insertions and + * deletions, already trimmed and ellipsized. + */ +function textDelta( before, after ) { + const segments = wordDiff( before, after ); + let inserted = ''; + let deleted = ''; + for ( const seg of segments ) { + if ( seg.type === 'insert' ) { + inserted += seg.value; + } else if ( seg.type === 'delete' ) { + deleted += seg.value; + } + } + return { + inserted: inserted.trim() ? ellipsize( inserted ) : '', + deleted: deleted.trim() ? ellipsize( deleted ) : '', + }; +} + +function isTextLike( value ) { + return typeof value === 'string'; +} + +/** + * Build a list of `{ label, value }` lines summarizing a suggestion. The + * content attribute is reported with `Add:` / `Delete:` quotes; other + * attribute changes are collapsed into a single `Format:` line listing the + * touched attributes. + * + * @param {import('./provider').SuggestionOperation[]} operations Operations. + * @return {Array<{label: string, value: string}>} Rendered lines. + */ +export function summarizeOperations( operations ) { + if ( ! Array.isArray( operations ) || operations.length === 0 ) { + return []; + } + + const lines = []; + const attributeLabels = []; + const formattingLabels = []; + + for ( const op of operations ) { + if ( op.type !== 'attribute-set' ) { + attributeLabels.push( op.attribute ); + continue; + } + + const isContent = op.attribute === 'content'; + const canTextDiff = + isContent && isTextLike( op.before ) && isTextLike( op.after ); + + if ( ! canTextDiff ) { + attributeLabels.push( op.attribute ); + continue; + } + + const before = op.before ?? ''; + const after = op.after ?? ''; + + // A pure inline-format change produces identical visible text with + // different markup — surface it as "Formatting: bold" rather than + // leaking raw `` into an Add/Delete quote. + if ( stripTags( before ) === stripTags( after ) && before !== after ) { + const changedFormats = diffInlineFormats( before, after ); + if ( changedFormats.length > 0 ) { + formattingLabels.push( ...changedFormats ); + } else { + attributeLabels.push( op.attribute ); + } + continue; + } + + const { inserted, deleted } = textDelta( before, after ); + if ( inserted ) { + lines.push( { label: __( 'Add:' ), value: `“${ inserted }”` } ); + } + if ( deleted ) { + lines.push( { + label: __( 'Delete:' ), + value: `“${ deleted }”`, + } ); + } + if ( ! inserted && ! deleted ) { + attributeLabels.push( op.attribute ); + } + } + + if ( formattingLabels.length > 0 ) { + lines.push( { + label: __( 'Formatting:' ), + value: joinLabels( formattingLabels ), + } ); + } + + if ( attributeLabels.length > 0 ) { + const labels = attributeLabels.map( + ( key ) => FORMAT_ATTRIBUTE_LABELS[ key ] ?? key + ); + lines.push( { label: __( 'Format:' ), value: joinLabels( labels ) } ); + } + + return lines; +} + +/** + * Compact sidebar summary of a suggestion — "Add: …", "Delete: …", + * "Format: …". Designed to mirror a Google Docs-style review note. + * + * @param {Object} props + * @param {import('./provider').SuggestionOperation[]} props.operations + */ +export default function SuggestionSummary( { operations } ) { + const lines = useMemo( + () => summarizeOperations( operations ), + [ operations ] + ); + + if ( lines.length === 0 ) { + return null; + } + + return ( + + { lines.map( ( line, index ) => ( + + { line.label } { line.value } + + ) ) } + + ); +} diff --git a/packages/editor/src/components/suggestion-mode/test/provider.js b/packages/editor/src/components/suggestion-mode/test/provider.js index f9d6ccbe9cdb5c..5c73c4aef56a39 100644 --- a/packages/editor/src/components/suggestion-mode/test/provider.js +++ b/packages/editor/src/components/suggestion-mode/test/provider.js @@ -4,6 +4,7 @@ import { operationsFromOverlay, applyOperations, + hasAttributeConflict, parseSuggestionPayload, payloadByteLength, PAYLOAD_MAX_BYTES, @@ -148,6 +149,105 @@ describe( 'payloadByteLength', () => { } ); } ); +describe( 'hasAttributeConflict', () => { + const CONTENT_OP = { + type: 'attribute-set', + attribute: 'content', + before: 'Hello', + after: 'Hi', + }; + + it( 'returns false when the targeted attribute still matches the baseline', () => { + expect( + hasAttributeConflict( { content: 'Hello', level: 2 }, [ + CONTENT_OP, + ] ) + ).toBe( false ); + } ); + + it( 'returns true when the targeted attribute has diverged', () => { + expect( + hasAttributeConflict( { content: 'Hola' }, [ CONTENT_OP ] ) + ).toBe( true ); + } ); + + it( 'ignores unrelated attribute changes on the block', () => { + // Post modified bumps often because an unrelated attribute (or another + // block entirely) changed — those should never count as a conflict + // for this suggestion. + expect( + hasAttributeConflict( + { content: 'Hello', level: 3, align: 'center' }, + [ CONTENT_OP ] + ) + ).toBe( false ); + } ); + + it( 'deep-compares object-valued attributes', () => { + const op = { + type: 'attribute-set', + attribute: 'style', + before: { typography: { fontSize: '16px' } }, + after: { typography: { fontSize: '20px' } }, + }; + expect( + hasAttributeConflict( + { style: { typography: { fontSize: '16px' } } }, + [ op ] + ) + ).toBe( false ); + expect( + hasAttributeConflict( + { style: { typography: { fontSize: '18px' } } }, + [ op ] + ) + ).toBe( true ); + } ); + + it( 'treats a null baseline as equal to a missing current attribute', () => { + const op = { + type: 'attribute-set', + attribute: 'url', + before: null, + after: 'https://x.test', + }; + expect( hasAttributeConflict( {}, [ op ] ) ).toBe( false ); + expect( + hasAttributeConflict( { url: 'https://other.test' }, [ op ] ) + ).toBe( true ); + } ); + + it( 'returns false for malformed input', () => { + expect( hasAttributeConflict( {}, undefined ) ).toBe( false ); + expect( hasAttributeConflict( {}, [] ) ).toBe( false ); + } ); + + it( 'compares string baselines against wrapper-object live values via toString', () => { + // Regression: rich-text attributes are stored on a block as + // `RichTextData` instances but serialize into the suggestion payload + // as plain strings. Without a string-vs-wrapper fallback, + // `hasAttributeConflict` flagged every content suggestion as stale + // because `typeof string` !== `typeof object`, which short-circuited + // the apply flow into a never-visible "Apply anyway" dialog. + const wrapper = { + toString() { + return 'Hello'; + }, + }; + const wrapperOther = { + toString() { + return 'Hola'; + }, + }; + expect( + hasAttributeConflict( { content: wrapper }, [ CONTENT_OP ] ) + ).toBe( false ); + expect( + hasAttributeConflict( { content: wrapperOther }, [ CONTENT_OP ] ) + ).toBe( true ); + } ); +} ); + describe( 'parseSuggestionPayload', () => { it( 'parses a valid JSON payload', () => { const raw = JSON.stringify( { diff --git a/packages/editor/src/components/suggestion-mode/test/suggestion-summary.js b/packages/editor/src/components/suggestion-mode/test/suggestion-summary.js new file mode 100644 index 00000000000000..4961eb9b610a2c --- /dev/null +++ b/packages/editor/src/components/suggestion-mode/test/suggestion-summary.js @@ -0,0 +1,202 @@ +/** + * Internal dependencies + */ +import { summarizeOperations } from '../suggestion-summary'; + +describe( 'summarizeOperations', () => { + it( 'returns an empty list for missing or empty operations', () => { + expect( summarizeOperations( undefined ) ).toEqual( [] ); + expect( summarizeOperations( [] ) ).toEqual( [] ); + } ); + + it( 'quotes a purely-inserted content span as Add:', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'Hello world', + after: 'Hello brave new world', + }, + ] ); + expect( lines ).toEqual( [ { label: 'Add:', value: '“brave new”' } ] ); + } ); + + it( 'quotes a purely-removed content span as Delete:', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'Hello brave new world', + after: 'Hello world', + }, + ] ); + expect( lines ).toEqual( [ + { label: 'Delete:', value: '“brave new”' }, + ] ); + } ); + + it( 'reports both Add: and Delete: lines for a mixed content edit', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'The quick fox jumps', + after: 'The slow fox leaps', + }, + ] ); + expect( lines ).toEqual( + expect.arrayContaining( [ + { label: 'Add:', value: expect.stringContaining( 'slow' ) }, + { label: 'Delete:', value: expect.stringContaining( 'quick' ) }, + ] ) + ); + } ); + + it( 'collapses non-content attribute changes into a Format: line', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'level', + before: 2, + after: 3, + }, + { + type: 'attribute-set', + attribute: 'align', + before: null, + after: 'center', + }, + ] ); + expect( lines ).toEqual( [ + { + label: 'Format:', + value: expect.stringMatching( /heading level.*alignment/i ), + }, + ] ); + } ); + + it( 'combines content and format changes in a single summary', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'Hi', + after: 'Hi there', + }, + { + type: 'attribute-set', + attribute: 'level', + before: 2, + after: 3, + }, + ] ); + expect( lines ).toEqual( + expect.arrayContaining( [ + { label: 'Add:', value: expect.stringContaining( 'there' ) }, + { + label: 'Format:', + value: expect.stringContaining( 'heading level' ), + }, + ] ) + ); + } ); + + it( 'ellipsizes very long insertions and deletions', () => { + const long = 'lorem ipsum '.repeat( 50 ); + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: '', + after: long, + }, + ] ); + expect( lines ).toHaveLength( 1 ); + expect( lines[ 0 ].label ).toBe( 'Add:' ); + expect( lines[ 0 ].value.endsWith( '…”' ) ).toBe( true ); + } ); + + it( 'reports bold toggled on as Formatting: bold', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 's magna elementum platea neque.', + after: 's magna elementum platea neque.', + }, + ] ); + expect( lines ).toEqual( [ { label: 'Formatting:', value: 'bold' } ] ); + } ); + + it( 'reports bold toggled off as Formatting: bold', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'Hello brave world', + after: 'Hello brave world', + }, + ] ); + expect( lines ).toEqual( [ { label: 'Formatting:', value: 'bold' } ] ); + } ); + + it( 'collapses multiple inline format changes into one Formatting: line', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'hello world', + after: 'hello world', + }, + ] ); + expect( lines ).toHaveLength( 1 ); + expect( lines[ 0 ].label ).toBe( 'Formatting:' ); + expect( lines[ 0 ].value ).toMatch( /bold/ ); + expect( lines[ 0 ].value ).toMatch( /italic/ ); + } ); + + it( 'still reports Add/Delete when text also changed alongside formatting', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'Hello world', + after: 'Hello brave world', + }, + ] ); + // Visible text differs ("brave" inserted), so this takes the text-diff + // path rather than the Formatting: path. + expect( lines ).toEqual( + expect.arrayContaining( [ + { label: 'Add:', value: expect.stringContaining( 'brave' ) }, + ] ) + ); + } ); + + it( 'treats non-text content attributes as a format change', () => { + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: { not: 'a string' }, + after: { also: 'object' }, + }, + ] ); + expect( lines ).toEqual( [ { label: 'Format:', value: 'content' } ] ); + } ); + + it( 'decodes HTML entities when comparing visible text', () => { + // `&` and `&` render to the same visible text. The summary + // should treat this as a pure formatting change (bold toggled), + // not as an Add/Delete of an ampersand. + const lines = summarizeOperations( [ + { + type: 'attribute-set', + attribute: 'content', + before: 'Tom & Jerry', + after: 'Tom & Jerry', + }, + ] ); + expect( lines ).toEqual( [ { label: 'Formatting:', value: 'bold' } ] ); + } ); +} ); diff --git a/packages/editor/src/components/suggestion-mode/test/with-suggestion-overlay.js b/packages/editor/src/components/suggestion-mode/test/with-suggestion-overlay.js index 70e9683f36ea86..afacd69920caf1 100644 --- a/packages/editor/src/components/suggestion-mode/test/with-suggestion-overlay.js +++ b/packages/editor/src/components/suggestion-mode/test/with-suggestion-overlay.js @@ -7,7 +7,7 @@ import { render, screen, act, fireEvent } from '@testing-library/react'; * WordPress dependencies */ import { createRegistry, RegistryProvider } from '@wordpress/data'; -import { store as preferencesStore } from '@wordpress/preferences'; +import { store as noticesStore } from '@wordpress/notices'; /** * Internal dependencies @@ -23,9 +23,12 @@ import { store as editorStore } from '../../../store'; function renderWithProviders( ui, { intent = 'edit' } = {} ) { const registry = createRegistry(); - registry.register( preferencesStore ); + // `setEditorIntent` dispatches a snackbar via the notices store when + // the intent actually changes, so the store needs to be registered even + // in tests that only care about the overlay HOC. + registry.register( noticesStore ); registry.register( editorStore ); - registry.dispatch( preferencesStore ).set( 'core', 'editorIntent', intent ); + registry.dispatch( editorStore ).setEditorIntent( intent ); const wrapper = ( { children } ) => ( diff --git a/packages/editor/src/components/suggestion-mode/with-suggestion-overlay.js b/packages/editor/src/components/suggestion-mode/with-suggestion-overlay.js index a1a217a62f43d8..7a5a4981783286 100644 --- a/packages/editor/src/components/suggestion-mode/with-suggestion-overlay.js +++ b/packages/editor/src/components/suggestion-mode/with-suggestion-overlay.js @@ -1,3 +1,8 @@ +/** + * External dependencies + */ +import clsx from 'clsx'; + /** * WordPress dependencies */ @@ -73,7 +78,7 @@ function SuggestingBlockEdit( { BlockEdit, props } ) { // source of truth; `captureBaseline` only creates an entry when there // isn't one, so we can skip the dispatch when we already know there is. // Relying on a local ref was fragile — it didn't reset after the entry - // was cleared (orphan prune, intent-switch). + // was cleared (auto-save trash, orphan prune, intent-switch). const entryExists = !! entries[ clientId ]; const wrappedSetAttributes = useCallback( @@ -131,10 +136,49 @@ const withSuggestionOverlay = createHigherOrderComponent( 'withSuggestionOverlay' ); +/** + * HOC that tags the rendered block list item with a class whenever it has a + * pending suggestion overlay. The class is the hook for the "bracket" + * styling that makes edited blocks discoverable without relying on the + * block toolbar being visible. + */ +const withSuggestionBlockClassName = createHigherOrderComponent( + ( BlockListBlock ) => + function BlockListBlockWithSuggestionClass( props ) { + const { clientId } = props; + const { entries } = useSuggestionOverlay(); + const isSuggestMode = useSelect( + ( select ) => + select( EDITOR_STORE_NAME ).getEditorIntent() === + SUGGEST_INTENT, + [] + ); + const entry = entries[ clientId ]; + const hasPendingOverlay = + !! entry && + Object.keys( entry.overlayAttributes ?? {} ).length > 0; + + if ( ! isSuggestMode || ! hasPendingOverlay ) { + return ; + } + + return ( + + ); + }, + 'withSuggestionBlockClassName' +); + let filterRegistered = false; /** - * Register the overlay filter. Idempotent — safe to call multiple times + * Register the overlay filters. Idempotent — safe to call multiple times * (hot reload, dynamic imports). */ export function registerSuggestionOverlayFilter() { @@ -147,6 +191,11 @@ export function registerSuggestionOverlayFilter() { 'core/editor/suggestion-mode-overlay', withSuggestionOverlay ); + addFilter( + 'editor.BlockListBlock', + 'core/editor/suggestion-mode-block-class', + withSuggestionBlockClassName + ); } export { mergeOverlayAttributes }; diff --git a/packages/editor/src/store/actions.js b/packages/editor/src/store/actions.js index c04e8909da0b0c..669348d7773b0e 100644 --- a/packages/editor/src/store/actions.js +++ b/packages/editor/src/store/actions.js @@ -1078,33 +1078,54 @@ export const switchEditorMode = * (`suggest`), or viewing the post in a read-only mode (`view`). It is * orthogonal to the `editorMode` preference (visual vs. code). * - * The value persists via the preferences store so the intent survives - * reloads. Unknown intents are silently rejected (no dispatch, no - * announcement) to keep typos or stale values from corrupting the - * preference; valid values are listed in `EDITOR_INTENTS`. + * The intent is *session-scoped* — held in the editor reducer (not the + * preferences store), so reloading the editor always returns to `edit`. + * Persisting suggest/view across reloads surprises users who don't realize + * they left the editor in a non-default state. + * + * Unknown intents are silently rejected (no dispatch, no announcement) so + * typos from a bookmarklet, browser extension, or third-party plugin can't + * poison the editor state; valid values are listed in `EDITOR_INTENTS`. * * @param {'edit'|'suggest'|'view'} intent The editor intent to set. */ export const setEditorIntent = ( intent ) => - ( { registry } ) => { - // Reject unknown intents instead of throwing so a typo from a - // bookmarklet, browser extension, or third-party plugin can't poison - // the persisted preference. + ( { select, dispatch, registry } ) => { if ( ! EDITOR_INTENTS.includes( intent ) ) { return; } - registry - .dispatch( preferencesStore ) - .set( 'core', 'editorIntent', intent ); + const previousIntent = select.getEditorIntent(); + + dispatch( { type: 'SET_EDITOR_INTENT', intent } ); + // Skip the snackbar/announcement on the initial set (when there is no + // previous intent, e.g. during editor boot) so the user isn't greeted + // with a mode notice they didn't trigger. + if ( previousIntent === intent || previousIntent === undefined ) { + return; + } + + let label; if ( intent === EDITOR_INTENT_EDIT ) { - speak( __( 'Edit mode selected' ), 'assertive' ); + label = __( "You're editing" ); } else if ( intent === EDITOR_INTENT_SUGGEST ) { - speak( __( 'Suggest mode selected' ), 'assertive' ); + label = __( "You're suggesting" ); } else if ( intent === EDITOR_INTENT_VIEW ) { - speak( __( 'View mode selected' ), 'assertive' ); + label = __( "You're viewing" ); + } + + if ( label ) { + speak( label, 'assertive' ); + // Reuse the same notice id across mode changes so rapid keyboard + // cycling doesn't pile up multiple snackbars — the new notice + // replaces the old one. + registry.dispatch( noticesStore ).createNotice( 'info', label, { + id: 'editor-intent-mode', + type: 'snackbar', + isDismissible: true, + } ); } }; diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index eb6756642269eb..f09db7c472c5fd 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -468,6 +468,26 @@ export function selectedNote( state = {}, action ) { return state; } +/** + * Reducer returning the current editor intent (`edit`, `suggest`, `view`). + * The intent is intentionally session-scoped: it lives in the editor + * store, not in the preferences store, so reloading the editor always + * returns to the default `edit` intent. Persisting suggest/view across + * reloads is undesirable — it surprises the user and is the wrong default + * for most sessions. + * + * @param {'edit'|'suggest'|'view'} state Current state. + * @param {Object} action Dispatched action. + * @return {'edit'|'suggest'|'view'} Updated state. + */ +export function editorIntent( state = 'edit', action ) { + switch ( action.type ) { + case 'SET_EDITOR_INTENT': + return action.intent; + } + return state; +} + export default combineReducers( { postId, postType, @@ -492,5 +512,6 @@ export default combineReducers( { revisionId, showRevisionDiff, selectedNote, + editorIntent, dataviews: dataviewsReducer, } ); diff --git a/packages/editor/src/store/selectors.js b/packages/editor/src/store/selectors.js index 28d1a1aece9848..c0cb2ebe4c976f 100644 --- a/packages/editor/src/store/selectors.js +++ b/packages/editor/src/store/selectors.js @@ -1411,11 +1411,9 @@ export const getEditorMode = createRegistrySelector( * * @return {string} The current editor intent. One of `edit`, `suggest`, `view`. */ -export const getEditorIntent = createRegistrySelector( - ( select ) => () => - select( preferencesStore ).get( 'core', 'editorIntent' ) ?? - EDITOR_INTENT_EDIT -); +export function getEditorIntent( state ) { + return state.editorIntent ?? EDITOR_INTENT_EDIT; +} /* * Backward compatibility diff --git a/packages/editor/src/store/test/actions.js b/packages/editor/src/store/test/actions.js index d83ca6ee763527..71fb189dd28213 100644 --- a/packages/editor/src/store/test/actions.js +++ b/packages/editor/src/store/test/actions.js @@ -585,13 +585,13 @@ describe( 'Editor actions', () => { ); } ); - it( 'persists to the preferences store', () => { + it( 'does not write to the preferences store (session-scoped only)', () => { registry.dispatch( editorStore ).setEditorIntent( 'view' ); expect( registry .select( preferencesStore ) .get( 'core', 'editorIntent' ) - ).toEqual( 'view' ); + ).toBeUndefined(); } ); } ); diff --git a/packages/editor/src/style.scss b/packages/editor/src/style.scss index 56e86da7323bf3..29135c2ddac664 100644 --- a/packages/editor/src/style.scss +++ b/packages/editor/src/style.scss @@ -47,6 +47,7 @@ @use "./components/post-sticky/style.scss" as *; @use "./components/post-sync-status/style.scss" as *; @use "./components/post-taxonomies/style.scss" as *; +@use "./components/suggestion-mode/style.scss" as *; @use "./components/sync-connection-error-modal/style.scss" as *; @use "./components/post-template/style.scss" as *; @use "./components/template-actions-panel/style.scss" as *; diff --git a/phpunit/experimental/class-wp-rest-comments-controller-gutenberg-test.php b/phpunit/experimental/class-wp-rest-comments-controller-gutenberg-test.php index 02d2ea8ed39a56..f866348e8cdb72 100644 --- a/phpunit/experimental/class-wp-rest-comments-controller-gutenberg-test.php +++ b/phpunit/experimental/class-wp-rest-comments-controller-gutenberg-test.php @@ -1,6 +1,27 @@ array( 'reopen' ), ); } + + /** + * Test that a suggestion payload can be stored and retrieved via meta. + */ + public function test_create_note_with_suggestion_meta() { + wp_set_current_user( self::$editor_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + $payload = wp_json_encode( + array( + 'schemaVersion' => 1, + 'blockName' => 'core/paragraph', + 'baseRevision' => '2026-04-15T00:00:00', + 'operations' => array( + array( + 'type' => 'attribute-set', + 'attribute' => 'content', + 'before' => 'Hello', + 'after' => 'Hello world', + ), + ), + ) + ); + + $params = array( + 'post' => $post_id, + 'content' => '', + 'type' => 'note', + 'author' => self::$editor_id, + 'meta' => array( + '_wp_suggestion' => $payload, + ), + ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/comments' ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( wp_json_encode( $params ) ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 201, $response->get_status() ); + + $data = $response->get_data(); + $comment_id = $data['id'] ?? null; + $this->assertIsInt( $comment_id ); + + // Bypass REST schema variability across WP versions and check the + // stored meta directly. The sanitize_callback is in scope of this + // test; the REST layer assembles schemas at runtime in a way that + // isn't always available in the experimental phpunit harness. + $stored = get_comment_meta( $comment_id, '_wp_suggestion', true ); + $this->assertNotEmpty( $stored, 'Suggestion meta should round-trip into storage.' ); + $decoded = json_decode( $stored, true ); + $this->assertSame( 'core/paragraph', $decoded['blockName'] ?? null ); + $this->assertSame( 1, $decoded['schemaVersion'] ?? null ); + $this->assertCount( 1, $decoded['operations'] ?? array() ); + } + + /** + * Test that an editor can update a note they did not author (edit_post check). + */ + public function test_editor_can_update_note_on_own_post() { + wp_set_current_user( self::$admin_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + // Admin creates a note on editor's post. + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_type' => 'note', + 'comment_approved' => 1, + 'user_id' => self::$admin_id, + 'comment_content' => 'suggestion note', + ) + ); + + // Editor (post author) updates the note they did not author. + wp_set_current_user( self::$editor_id ); + + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/' . $comment_id ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( + wp_json_encode( + array( + 'status' => 'approved', + 'meta' => array( + '_wp_suggestion_status' => 'applied', + ), + ) + ) + ); + + $response = rest_get_server()->dispatch( $request ); + // The suggestion-lifecycle override passed because the editor owns + // the parent post; the update succeeds with a 200 status. + $this->assertSame( 200, $response->get_status() ); + } + + /** + * Test that a subscriber cannot update a note on someone else's post. + */ + public function test_subscriber_cannot_update_note() { + wp_set_current_user( self::$editor_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_type' => 'note', + 'user_id' => self::$editor_id, + 'comment_content' => 'a suggestion', + ) + ); + + // Subscriber tries to update the note. + wp_set_current_user( self::$subscriber_id ); + + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/' . $comment_id ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( + wp_json_encode( + array( + 'meta' => array( + '_wp_suggestion_status' => 'rejected', + ), + ) + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertErrorResponse( 'rest_cannot_edit', $response, 403 ); + } + + /** + * Test that _wp_suggestion_status does not persist invalid enum values. + */ + public function test_suggestion_status_ignores_invalid_value() { + wp_set_current_user( self::$editor_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_type' => 'note', + 'comment_approved' => 1, + 'user_id' => self::$editor_id, + ) + ); + + // First set a valid value. + update_comment_meta( $comment_id, '_wp_suggestion_status', 'pending' ); + + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/' . $comment_id ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( + wp_json_encode( + array( + 'meta' => array( + '_wp_suggestion_status' => 'invalid_value', + ), + ) + ) + ); + + rest_get_server()->dispatch( $request ); + // Even if the request succeeds, the invalid value should not + // overwrite the existing valid value. + $stored = get_comment_meta( $comment_id, '_wp_suggestion_status', true ); + $this->assertSame( 'pending', $stored ); + } + + /** + * Test that a subscriber cannot apply a suggestion even if the request + * only touches the suggestion-lifecycle fields. + */ + public function test_subscriber_cannot_apply_suggestion() { + wp_set_current_user( self::$editor_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_type' => 'note', + 'comment_approved' => 1, + 'user_id' => self::$editor_id, + 'comment_content' => '', + ) + ); + + wp_set_current_user( self::$subscriber_id ); + + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/' . $comment_id ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( + wp_json_encode( + array( + 'status' => 'approved', + 'meta' => array( + '_wp_suggestion_status' => 'applied', + ), + ) + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertErrorResponse( 'rest_cannot_edit', $response, 403 ); + } + + /** + * Test that creating a note with an oversized suggestion payload is + * rejected with a clear 413 error rather than silently truncated. + */ + public function test_create_rejects_oversized_suggestion_payload() { + wp_set_current_user( self::$editor_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + $oversized = str_repeat( 'a', GUTENBERG_SUGGESTION_PAYLOAD_MAX_BYTES + 1 ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/comments' ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( + wp_json_encode( + array( + 'post' => $post_id, + 'content' => '', + 'type' => 'note', + 'meta' => array( + '_wp_suggestion' => $oversized, + ), + ) + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertErrorResponse( 'rest_suggestion_too_large', $response, 413 ); + } + + /** + * Test that updating a note with an oversized suggestion payload is + * rejected with a clear 413 error. + */ + public function test_update_rejects_oversized_suggestion_payload() { + wp_set_current_user( self::$editor_id ); + $post_id = self::factory()->post->create( array( 'post_author' => self::$editor_id ) ); + + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_type' => 'note', + 'comment_approved' => 1, + 'user_id' => self::$editor_id, + 'comment_content' => 'a suggestion', + ) + ); + + $oversized = str_repeat( 'a', GUTENBERG_SUGGESTION_PAYLOAD_MAX_BYTES + 1 ); + + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/' . $comment_id ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( + wp_json_encode( + array( + 'meta' => array( + '_wp_suggestion' => $oversized, + ), + ) + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertErrorResponse( 'rest_suggestion_too_large', $response, 413 ); + } + + /** + * Test that the sanitize_callback rejects rather than truncates an + * oversized payload reaching the meta layer through a non-REST path. + * Truncating mid-string would corrupt the JSON. + */ + public function test_sanitize_callback_rejects_oversized_value() { + $post_id = self::factory()->post->create(); + $comment_id = self::factory()->comment->create( + array( + 'comment_post_ID' => $post_id, + 'comment_type' => 'note', + ) + ); + + $oversized = str_repeat( 'a', GUTENBERG_SUGGESTION_PAYLOAD_MAX_BYTES + 1 ); + update_comment_meta( $comment_id, '_wp_suggestion', $oversized ); + + $stored = get_comment_meta( $comment_id, '_wp_suggestion', true ); + $this->assertSame( '', $stored, 'Oversized payload should be rejected, not truncated.' ); + } + + /** + * Test that `is_suggestion_lifecycle_update` correctly rejects + * request bodies that touch fields outside the suggestion-lifecycle + * allowlist. We assert against the private helper via a request + * probe rather than through the full REST dispatch because actual + * permission behavior for `edit_comment` on a foreign note on a + * post the current user authored is governed by core's + * `map_meta_cap` for `edit_comment` (which delegates to `edit_post` + * on the comment's parent post) — outside the scope of this override. + */ + public function test_lifecycle_update_rejects_non_allowlisted_fields() { + $cases = array( + 'content field blocks shortcut' => array( + 'body' => array( + 'status' => 'approved', + 'content' => 'rewritten', + ), + 'expected' => false, + ), + 'only id/status/meta passes shortcut' => array( + 'body' => array( + 'status' => 'approved', + 'meta' => array( + '_wp_suggestion_status' => 'applied', + ), + ), + 'expected' => true, + ), + 'non-approved status blocks shortcut' => array( + 'body' => array( + 'status' => 'spam', + ), + 'expected' => false, + ), + 'non-allowlisted meta blocks shortcut' => array( + 'body' => array( + 'meta' => array( + '_wp_note_status' => 'resolved', + ), + ), + 'expected' => false, + ), + ); + + foreach ( $cases as $label => $case ) { + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/1' ); + $request->add_header( 'Content-Type', 'application/json' ); + $request->set_body( wp_json_encode( $case['body'] ) ); + + $reflection = new ReflectionMethod( + 'Gutenberg_REST_Comment_Controller_6_9', + 'is_suggestion_lifecycle_update' + ); + $reflection->setAccessible( true ); + + $this->assertSame( + $case['expected'], + $reflection->invoke( null, $request ), + "Lifecycle shortcut expectation mismatched for: {$label}" + ); + } + } + + /** + * Test that the lifecycle helper also accepts form-encoded request + * bodies, not only JSON. Custom integrations may issue updates with + * `application/x-www-form-urlencoded` and should benefit from the + * same `edit_post` shortcut as the JSON path. + */ + public function test_lifecycle_update_accepts_form_encoded_bodies() { + $request = new WP_REST_Request( 'PUT', '/wp/v2/comments/1' ); + $request->add_header( 'Content-Type', 'application/x-www-form-urlencoded' ); + $request->set_body_params( + array( + 'status' => 'approved', + 'meta' => array( + '_wp_suggestion_status' => 'applied', + ), + ) + ); + + $reflection = new ReflectionMethod( + 'Gutenberg_REST_Comment_Controller_6_9', + 'is_suggestion_lifecycle_update' + ); + $reflection->setAccessible( true ); + $this->assertTrue( $reflection->invoke( null, $request ) ); + } } diff --git a/test/e2e/specs/editor/various/block-notes.spec.js b/test/e2e/specs/editor/various/block-notes.spec.js index 4ab7003a1974a4..73115f3dd3a573 100644 --- a/test/e2e/specs/editor/various/block-notes.spec.js +++ b/test/e2e/specs/editor/various/block-notes.spec.js @@ -848,6 +848,419 @@ test.describe( 'Block Notes', () => { await expect( thread ).toBeFocused(); } ); } ); + + test.describe( 'Multiple notes per block', () => { + test( 'can add multiple notes to the same block', async ( { + editor, + page, + blockNoteUtils, + } ) => { + // Add a block with the first note. + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block with multiple notes' }, + comment: 'First note on block', + } ); + + // Dismiss the first "Note added." snackbar so it won't interfere + // with the second note's snackbar check. + await page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + .click(); + + // Click "Add note" again to add a second note. + await editor.clickBlockOptionsMenuItem( 'Add note' ); + + // Verify the new note form is shown (not the reply form). + const newNoteForm = page.getByRole( 'textbox', { + name: 'New note', + exact: true, + } ); + await expect( newNoteForm ).toBeFocused(); + + // Add the second note. + await newNoteForm.fill( 'Second note on block' ); + await page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Add note', exact: true } ) + .click(); + + // Verify both notes are visible as separate threads. + const firstThread = page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'treeitem', { name: 'Note: First note on block' } ); + const secondThread = page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'treeitem', { + name: 'Note: Second note on block', + } ); + + await expect( firstThread ).toBeVisible(); + await expect( secondThread ).toBeVisible(); + + // Verify "Note added." (not "Reply added."). + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + } ); + + test( 'multiple notes on same block are stored as array in metadata', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block with multiple notes' }, + comment: 'First note', + } ); + // Dismiss the first "Note added." so the second toast can be asserted. + await blockNoteUtils.dismissSnackbar( 'Note added.' ); + + await blockNoteUtils.addAnotherNoteToCurrentBlock( 'Second note' ); + + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + + // Verify noteId is an array with 2 elements. + const blocks = await editor.getBlocks(); + const paragraphBlock = blocks.find( + ( b ) => b.name === 'core/paragraph' + ); + const noteIds = paragraphBlock?.attributes?.metadata?.noteId; + + expect( Array.isArray( noteIds ) ).toBe( true ); + expect( noteIds ).toHaveLength( 2 ); + } ); + + test( 'deleting one note preserves the other notes on the same block', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block with notes to delete' }, + comment: 'Note to keep', + } ); + await blockNoteUtils.dismissSnackbar( 'Note added.' ); + + await blockNoteUtils.addAnotherNoteToCurrentBlock( + 'Note to delete' + ); + + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + + // Both notes should be visible. + const settings = page.getByRole( 'region', { + name: 'Editor settings', + } ); + await expect( + settings.getByRole( 'treeitem', { name: 'Note: Note to keep' } ) + ).toBeVisible(); + await expect( + settings.getByRole( 'treeitem', { + name: 'Note: Note to delete', + } ) + ).toBeVisible(); + + // Delete the second note. + const secondThread = settings.getByRole( 'treeitem', { + name: 'Note: Note to delete', + } ); + await secondThread.click(); + await blockNoteUtils.clickBlockNoteActionMenuItem( 'Delete' ); + await page + .getByRole( 'dialog' ) + .getByRole( 'button', { name: 'Delete' } ) + .click(); + + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note deleted.' } ) + ).toBeVisible(); + + // First note should still be visible; second should be gone. + await expect( + settings.getByRole( 'treeitem', { name: 'Note: Note to keep' } ) + ).toBeVisible(); + await expect( + settings.getByRole( 'treeitem', { + name: 'Note: Note to delete', + } ) + ).toBeHidden(); + + // Metadata should still have one noteId remaining. + const blocks = await editor.getBlocks(); + const paragraphBlock = blocks.find( + ( b ) => b.name === 'core/paragraph' + ); + const noteIds = paragraphBlock?.attributes?.metadata?.noteId; + expect( noteIds ).toHaveLength( 1 ); + } ); + + test( 'resolving one note does not affect sibling notes on the same block', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block with notes to resolve' }, + comment: 'Note A', + } ); + await blockNoteUtils.dismissSnackbar( 'Note added.' ); + + await blockNoteUtils.addAnotherNoteToCurrentBlock( 'Note B' ); + + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + + const settings = page.getByRole( 'region', { + name: 'Editor settings', + } ); + + // Resolve Note A. + const threadA = settings.getByRole( 'treeitem', { + name: 'Note: Note A', + } ); + await threadA.click(); + await page.getByRole( 'button', { name: 'Resolve' } ).click(); + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note marked as resolved.' } ) + ).toBeVisible(); + + // Note B should still be visible and unresolved (expanded). + const threadB = settings.getByRole( 'treeitem', { + name: 'Note: Note B', + } ); + await expect( threadB ).toBeVisible(); + + // Both notes should still exist in metadata. + const blocks = await editor.getBlocks(); + const paragraphBlock = blocks.find( + ( b ) => b.name === 'core/paragraph' + ); + const noteIds = paragraphBlock?.attributes?.metadata?.noteId; + expect( noteIds ).toHaveLength( 2 ); + } ); + + test( 'auto-selects first unresolved note when clicking a block with multiple notes', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block for auto-select' }, + comment: 'First note', + } ); + await blockNoteUtils.dismissSnackbar( 'Note added.' ); + + await blockNoteUtils.addAnotherNoteToCurrentBlock( 'Second note' ); + + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + + const settings = page.getByRole( 'region', { + name: 'Editor settings', + } ); + + // Resolve the first note. + const firstThread = settings.getByRole( 'treeitem', { + name: 'Note: First note', + } ); + await firstThread.click(); + await page.getByRole( 'button', { name: 'Resolve' } ).click(); + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note marked as resolved.' } ) + ).toBeVisible(); + + // Click the title to deselect the block and its comment. + await editor.canvas + .getByRole( 'textbox', { name: 'Add title' } ) + .focus(); + + // Click back on the original block. + await editor.canvas + .getByRole( 'document', { name: 'Block: Paragraph' } ) + .filter( { hasText: 'Block for auto-select' } ) + .click(); + + // The second (unresolved) note should be the active one. + const secondThread = settings.getByRole( 'treeitem', { + name: 'Note: Second note', + } ); + await expect( secondThread ).toHaveAttribute( + 'aria-expanded', + 'true' + ); + } ); + + test( 'metadata noteId is cleaned up when all notes are deleted from a block', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block to clear notes' }, + comment: 'Only note', + } ); + + const settings = page.getByRole( 'region', { + name: 'Editor settings', + } ); + const thread = settings.getByRole( 'treeitem', { + name: 'Note: Only note', + } ); + await thread.click(); + await blockNoteUtils.clickBlockNoteActionMenuItem( 'Delete' ); + await page + .getByRole( 'dialog' ) + .getByRole( 'button', { name: 'Delete' } ) + .click(); + + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note deleted.' } ) + ).toBeVisible(); + + // noteId should be cleaned up from metadata entirely. + const blocks = await editor.getBlocks(); + const paragraphBlock = blocks.find( + ( b ) => b.name === 'core/paragraph' + ); + expect( + paragraphBlock?.attributes?.metadata?.noteId + ).toBeUndefined(); + } ); + + test( 'lazy-migrates a legacy scalar noteId to an array when a second note is added', async ( { + editor, + page, + } ) => { + // Insert a block whose metadata already carries a scalar noteId, the + // shape produced by older code before multi-note support shipped. The + // orphan id (999) does not correspond to a real thread; the test + // asserts that the lazy migration in addNoteIdToMetadata preserves it + // rather than silently dropping it on first write. + await editor.insertBlock( { + name: 'core/paragraph', + attributes: { + content: 'Block with legacy scalar noteId', + metadata: { noteId: 999 }, + }, + } ); + + await editor.clickBlockOptionsMenuItem( 'Add note' ); + await page + .getByRole( 'textbox', { name: 'New note', exact: true } ) + .fill( 'New multi-note era note' ); + await page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Add note', exact: true } ) + .click(); + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + + const blocks = await editor.getBlocks(); + const paragraphBlock = blocks.find( + ( b ) => b.name === 'core/paragraph' + ); + const noteIds = paragraphBlock?.attributes?.metadata?.noteId; + + expect( Array.isArray( noteIds ) ).toBe( true ); + expect( noteIds ).toHaveLength( 2 ); + // Legacy id is preserved as the first entry; the new note id is appended. + expect( noteIds[ 0 ] ).toBe( 999 ); + expect( typeof noteIds[ 1 ] ).toBe( 'number' ); + expect( noteIds[ 1 ] ).not.toBe( 999 ); + } ); + + test( 'multi-note metadata persists across save and reload', async ( { + editor, + page, + blockNoteUtils, + } ) => { + await blockNoteUtils.addBlockWithNote( { + type: 'core/paragraph', + attributes: { content: 'Block with persisted notes' }, + comment: 'Persisted note A', + } ); + await blockNoteUtils.dismissSnackbar( 'Note added.' ); + + await blockNoteUtils.addAnotherNoteToCurrentBlock( + 'Persisted note B' + ); + await expect( + page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: 'Note added.' } ) + ).toBeVisible(); + + const beforeIds = ( await editor.getBlocks() ).find( + ( b ) => b.name === 'core/paragraph' + )?.attributes?.metadata?.noteId; + expect( beforeIds ).toHaveLength( 2 ); + + await editor.saveDraft(); + await page.reload(); + + // The pinned notes sidebar isn't restored on reload, so open it + // explicitly before asserting the threads are visible. + await blockNoteUtils.openBlockNoteSidebar(); + + // After reload, both threads should reattach to the block. + const settings = page.getByRole( 'region', { + name: 'Editor settings', + } ); + await expect( + settings.getByRole( 'treeitem', { + name: 'Note: Persisted note A', + } ) + ).toBeVisible(); + await expect( + settings.getByRole( 'treeitem', { + name: 'Note: Persisted note B', + } ) + ).toBeVisible(); + + // Metadata should still be in array form, with the same ids in the + // same order — the round-trip through serialize/parse must not lose + // or reorder the array. + const afterIds = ( await editor.getBlocks() ).find( + ( b ) => b.name === 'core/paragraph' + )?.attributes?.metadata?.noteId; + expect( Array.isArray( afterIds ) ).toBe( true ); + expect( afterIds ).toEqual( beforeIds ); + } ); + } ); } ); class BlockNoteUtils { @@ -921,4 +1334,22 @@ class BlockNoteUtils { .click(); await this.#page.getByRole( 'menuitem', { name: actionName } ).click(); } + + async dismissSnackbar( text ) { + await this.#page + .getByRole( 'button', { name: 'Dismiss this notice' } ) + .filter( { hasText: text } ) + .click(); + } + + async addAnotherNoteToCurrentBlock( content ) { + await this.#editor.clickBlockOptionsMenuItem( 'Add note' ); + await this.#page + .getByRole( 'textbox', { name: 'New note', exact: true } ) + .fill( content ); + await this.#page + .getByRole( 'region', { name: 'Editor settings' } ) + .getByRole( 'button', { name: 'Add note', exact: true } ) + .click(); + } } diff --git a/test/e2e/specs/editor/various/editor-intent-switcher.spec.js b/test/e2e/specs/editor/various/editor-intent-switcher.spec.js index b4171b3577046b..5ba03cd6fc0b7a 100644 --- a/test/e2e/specs/editor/various/editor-intent-switcher.spec.js +++ b/test/e2e/specs/editor/various/editor-intent-switcher.spec.js @@ -14,7 +14,7 @@ test.describe( 'Editor intent switcher', () => { await admin.createNewPost(); } ); - test( 'defaults to Edit intent and persists across reload', async ( { + test( 'defaults to Edit intent on every editor load', async ( { page, editor, } ) => { @@ -38,13 +38,16 @@ test.describe( 'Editor intent switcher', () => { await expect( viewChoice ).toBeVisible(); await expect( editChoice ).toHaveAttribute( 'aria-checked', 'true' ); - // Select Suggest and confirm selection persists across reload. + // The intent is session-scoped: switching to Suggest and reloading + // must fall back to Edit rather than persist the previous choice. await suggestChoice.click(); await page.reload(); await editor.canvas.locator( 'body' ).waitFor(); await openIntentSwitcher( page ); await expect( - page.getByRole( 'menuitemradio', { name: /^Suggest/ } ) + page.getByRole( 'menuitemradio', { + name: /^Edit\s+Edit content directly/, + } ) ).toHaveAttribute( 'aria-checked', 'true' ); } ); diff --git a/test/e2e/specs/editor/various/suggestion-mode.spec.js b/test/e2e/specs/editor/various/suggestion-mode.spec.js index d69fdee8aad20a..14e0fff32ed876 100644 --- a/test/e2e/specs/editor/various/suggestion-mode.spec.js +++ b/test/e2e/specs/editor/various/suggestion-mode.spec.js @@ -19,12 +19,43 @@ async function switchIntent( page, intentLabel ) { await page.keyboard.press( 'Escape' ); } +async function waitForSuggestionSaved( page ) { + // Auto-save is debounced; wait for the REST call to land. + await page.waitForResponse( + ( response ) => + /\/wp\/v2\/comments(\?|$|\/)/.test( response.url() ) && + [ 'POST', 'PUT' ].includes( response.request().method() ) && + response.ok() + ); +} + test.describe( 'Suggestion mode', () => { test.beforeEach( async ( { admin } ) => { await admin.createNewPost(); } ); - test( 'captures edits as an overlay without mutating the block', async ( { + test.afterAll( async ( { requestUtils } ) => { + await requestUtils.deleteAllComments( 'note' ); + } ); + + test( 'announces the mode change with a snackbar', async ( { page } ) => { + // The mode change also fires an a11y live-region announcement + // carrying the same text, so scope to the snackbar list to avoid a + // strict-mode match on both the snackbar and the live region. + const snackbarList = page.locator( '.components-snackbar-list' ); + + await switchIntent( page, 'Suggest' ); + await expect( + snackbarList.getByText( "You're suggesting" ) + ).toBeVisible(); + + await switchIntent( page, 'Edit' ); + await expect( + snackbarList.getByText( "You're editing" ) + ).toBeVisible(); + } ); + + test( 'auto-saves a content edit as a suggestion', async ( { editor, page, } ) => { @@ -42,16 +73,19 @@ test.describe( 'Suggestion mode', () => { await page.keyboard.press( 'End' ); await page.keyboard.type( ' plus suggested' ); - // The rendered block reflects the overlayed suggestion. + // Overlay reflects the proposed content, block store does not. await expect( paragraph ).toContainText( 'Original content plus suggested' ); - - // The serialized post content stays at the baseline — the overlay - // never touched the block-editor store. const serialized = await editor.getEditedPostContent(); expect( serialized ).toContain( 'Original content' ); expect( serialized ).not.toContain( 'plus suggested' ); + + // Auto-save fires after the debounce window. + await waitForSuggestionSaved( page ); + + // Edited block picks up the pending-suggestion outline. + await expect( paragraph ).toHaveClass( /is-suggestion-pending/ ); } ); test( 'captures a heading-level change made via the block-switcher variation picker', async ( { @@ -94,31 +128,9 @@ test.describe( 'Suggestion mode', () => { const serialized = await editor.getEditedPostContent(); expect( serialized ).toContain( '