Suggest mode 2/9: suggestion storage, REST controller, and provider - #80428
adamsilverstein wants to merge 60 commits into
Conversation
Adds the comment-meta storage backbone: _wp_suggestion payload meta and sanitization (64KB cap, KSES on block snapshots), the 7.1 REST comment controller subclass scoped to suggestion lifecycle updates, the comment-meta SuggestionsProvider (create/update/delete, apply/reject for attribute and structural operations, schema versioning + migration), the pending-suggestion overlay store with its debounced write queue, and the auto-save loop that persists overlay entries as note comments. Inline (marker-based) operations are added by a later layer. Nothing captures suggestions yet - that starts in the next layer.
|
Size Change: +4.24 kB (+0.05%) Total Size: 7.92 MB 📦 View Changed
|
# Conflicts: # tools/eslint/suppressions.json
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Trunk switched the @wordpress/dependency-group rule to 'never' mode, so the import group headers in these files now fail lint.
The auto-save scheduler never cleared its debounce timers when the intent changed. Because the component is mounted on the experiment flag rather than the intent, switching from Suggest to Edit or View left any armed timer running, and it went on to POST a note for edits the user had just walked away from - visible to collaborators as a deliberate suggestion. Cancelling is not lossy: the overlay entry keeps its unsynced fingerprint, so returning to Suggest reschedules the save.
… tree Applying or rejecting a block-remove, block-insert-after, or block-move suggestion dispatched the tree mutation first and only surfaced an error notice if the save failed. Nothing rolled the tree back, so a failed save left the editor diverged from the server: the block was gone (or moved) while the note still read as pending, and for block-remove the marker went with the block, leaving no way to act on the note again. The attribute-set path solves this by snapshotting attributes and restoring them. A structural rollback cannot be that faithful - position, nested children, selection, and the overlay entry would all need restoring - so await the lifecycle write first instead. The mutation only runs once the decision is persisted, which is the same invariant for less machinery.
|
Flaky tests detected in c05f47a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/33117670591 refuses the drop and uploads nothing in
|
Rename the files this PR adds from .js to .ts/.tsx and add types so the package's strict type check covers them, per the repo convention that new files are authored in TypeScript. No behavior changes: the overlay entry, context value, reducer action, and suggestion payload shapes are now explicit interfaces, and eslint suppression paths follow the renames.
Trunk bumped Prettier from 3.0.3 to 3.9.6, which indents these two continuation lines differently. Formatting only, no behavior change. Claude-Session: https://claude.ai/code/session_015VuPwr3z18cAox2kB4wp2H
An attribute-only suggestion has no structural op, so rejecting it skipped every clearOverlay() call. The overlay entry kept rendering the rejected value and carried it into the next proposal. Clear it with the guarded clearOverlayForComment() so an entry already reused by a newer suggestion survives. Claude-Session: https://claude.ai/code/session_01EmSXGAGtuLnk3UEe5kXPSU
The restore used the recorded fromParentClientId as its destination, but client ids are regenerated on every parse, so rejecting even a same-parent move inside a Group after a reload targeted a parent that no longer exists. A same-parent move now restores within the block's live parent; a cross-parent move uses the recorded parent only while it still exists. Claude-Session: https://claude.ai/code/session_01EmSXGAGtuLnk3UEe5kXPSU
Leaving the suggest intent cleared every debounce timer, so a proposal made moments before switching to Editing lived only in React state and was lost on save and reload. Switching intents is a normal part of the review workflow, so enqueue the pending saves immediately instead. Claude-Session: https://claude.ai/code/session_01EmSXGAGtuLnk3UEe5kXPSU
|
Claude worked through the latest review of this layer, three fixes landed:
|
Structured `after` values such as table rows and snapshot attributes were stored unfiltered for users without unfiltered_html. Reuse core's filter_block_kses_value() so they get the same walk parsed block attributes get. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHz7zkCC2S8crriSWsYPDk
The overlay context bundled every block's entries with its stable callbacks, and hasOverlay changed identity with entries, so each overlay write re-rendered every consumer. Entries now live in a small external store; a stable actions context and useOverlayEntry(), a per-clientId subscription, let per-block consumers opt out of other blocks' writes. useSuggestionOverlay() is unchanged. The orphan prune looks up each entry instead of building a full-tree Set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHz7zkCC2S8crriSWsYPDk
Every overlay change re-fingerprinted every entry and restarted every block's timer, so steady typing in one block postponed another block's save. Only entries whose object changed are rescheduled now. Pending saves are flushed on unmount instead of dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHz7zkCC2S8crriSWsYPDk
Lets the store interceptor skip store fires that leave the block tree unchanged without stranding a bypass token that the old per-fire walk would have consumed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHz7zkCC2S8crriSWsYPDk
| } | ||
|
|
||
| createNotice( | ||
| 'snackbar' as any, |
There was a problem hiding this comment.
The cast is hiding a legitimate TypeScript error that 'snackbar' is not a valid option here. Should it be 'success' ?
| 'snackbar' as any, | |
| 'success', |
There was a problem hiding this comment.
Good catch, thanks - that cast was leaking a notice type into the status slot. Fixed in 3219749 (and the two extra copies up in the wiring layer got the same treatment).
'snackbar' is a notice type, not a status, and the 'as any' cast was hiding the type error. Use 'success' for the applied and rejected snackbars; the options already set the snackbar type. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016qzrCziUUHcLxYeMHysRcQ
mikachan
left a comment
There was a problem hiding this comment.
Thanks for splitting this into a stack, @adamsilverstein. I've had Claude take a look at this and I've left some inline comments with the feedback.
| continue; | ||
| } | ||
|
|
||
| if ( timers.has( clientId ) ) { |
There was a problem hiding this comment.
I think the debounce is re-arming across blocks rather than per block. The effect depends on the whole entries object, so any overlay change re-runs it, and this loop then clears and restarts the timer for every block whose fingerprint doesn't match syncedOpsKey, not just the block that actually changed.
That doesn't match the docblock at the top of the file, which says each new edit on a block clears that block's timer. Could we keep the last fingerprint per clientId in a ref and skip re-arming when a block already has a live timer and its fingerprint hasn't changed?
| try { | ||
| if ( operations.length === 0 ) { | ||
| if ( commentId ) { | ||
| await deleteRef.current( { commentId } ); |
There was a problem hiding this comment.
When the overlay returns to baseline, we delete the note here, but I don't think anything removes the id from the block's metadata.noteId. createSuggestion writes it there with updateBlockAttributes, and that attribute is actual post content, so after suggesting an edit and then undoing it, the post saves carrying a noteId pointing at a deleted comment, and the getNoteIdsFromMetadata consumers downstream will try to resolve it.
The collab sidebar already handles this on the equivalent path, with removeNoteIdFromMetadata in packages/editor/src/components/collab-sidebar/hooks.js (around L379). Could we do the same here, wrapped in markNextChangeAsNotPersistent( { history: 'ignore' } ) to match the create path?
| * @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 ) { |
There was a problem hiding this comment.
The note above says a post editor already holds full edit permission over notes on their posts through the core fallback, which makes me wonder whether this override will change anything.
Core's update_item_permissions_check() calls check_edit_permission(), which falls through to current_user_can( 'edit_comment', $comment_ID ), and map_meta_cap()'s edit_comment case resolves to map_meta_cap( 'edit_post', $user_id, $comment->comment_post_ID ) (wp-includes/capabilities.php L578 in 6.8.1). That's the same check as the shortcut here, so everyone this admits already passes core's check, and everyone it rejects core rejects too.
I think test_editor_can_update_note_on_own_post would still pass with this method deleted.
If that's right, I think we should drop this and is_suggestion_lifecycle_update(). What do you think?
| // from the post-apply attributes. Outside Suggest mode the | ||
| // interceptor isn't running and these calls are no-ops. | ||
| requestInterceptorBypass( targetClientId ); | ||
| clearOverlay( targetClientId ); |
There was a problem hiding this comment.
clearOverlay( targetClientId ) runs before the save, but the catch below only rolls the attributes back and doesn't restore the overlay entry. So if the server rejects the status update, the attributes revert, but the suggester's overlay entry is dropped for good, and I don't think there's a way to recover it.
Would it be worth moving clearOverlay() to after the await, so it only runs once the save has actually succeeded?
When an overlay returns to baseline, auto-save trashes the note but left its id in the block's `metadata.noteId`. That attribute is post content, so the post saved a reference to a trashed comment that downstream `getNoteIdsFromMetadata` consumers would try to resolve. `deleteSuggestion` now takes the block's clientId and removes the id once the trash succeeds, outside undo history like the link written on create. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9vmiAPMMg3s4HNJxXAYxy
The attribute apply path cleared the overlay entry before saving the decision. On a server rejection the catch rolled the attributes back, but the suggester's overlay entry was gone for good, with no way to recover the still-pending proposal. Clear it only after the save succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9vmiAPMMg3s4HNJxXAYxy
The subclass let users with `edit_post` on the parent post update lifecycle-only fields on a note. Core already admits exactly those users: `check_edit_permission()` falls through to `edit_comment`, which `map_meta_cap()` resolves to `edit_post` on the comment's parent post. The shortcut granted nothing, and it skipped any `map_meta_cap` filter a site adds for `edit_comment`. Remove it and the reflection tests for its allowlist helper. The REST dispatch tests for an editor applying and a subscriber being refused still pass through core's check. Also correct the registration comment: core registers its routes at priority 99, so this subclass registers first, not after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E9vmiAPMMg3s4HNJxXAYxy
Part of #73411
What's in this PR
Adds the comment-meta storage backbone: _wp_suggestion payload meta and
sanitization (64KB cap, KSES on block snapshots), the 7.1 REST comment
controller subclass scoped to suggestion lifecycle updates, the
comment-meta SuggestionsProvider (create/update/delete, apply/reject for
attribute and structural operations, schema versioning + migration), the
pending-suggestion overlay store with its debounced write queue, and the
auto-save loop that persists overlay entries as note comments. Inline
(marker-based) operations are added by a later layer. Nothing captures
suggestions yet - that starts in the next layer.
Diagram
There is no UI in this layer. It is the write path that turns an edit into a stored suggestion, plus the read path back out:
Testing
This is one layer of the stack. To exercise the whole feature, #78994 bundles every layer into one branch and builds it in Playground:
👉 https://playground.wordpress.net/gutenberg.html?pr=78994
Enable Gutenberg > Experiments > Collaboration > Suggestion Mode, then follow the walkthrough in #73411, which also explains how to review the stack layer by layer.
Suggest mode stack
This rebuilds the manually-stacked Suggest mode work (#73411) as a GitHub Stack of 9 small, independently reviewable PRs, each building on the one below it:
Each follow up fix now sits in the layer that owns the code it changes, rather than piling onto the top of the stack. The whole feature can be exercised end-to-end via the combined testing branch #78994 (Playground). Behind the "Suggestion Mode" experiment (Gutenberg > Experiments).
AI Use
Code and description both written with 🤖 Claude Code. I will review and test.