Skip to content

Auto-save subsystem for pending suggestions - #78308

Closed
adamsilverstein wants to merge 113 commits into
phase-5b-collab-sidebar-actionsfrom
suggest-mode-autosave
Closed

adamsilverstein wants to merge 113 commits into
phase-5b-collab-sidebar-actionsfrom
suggest-mode-autosave

Conversation

@adamsilverstein

@adamsilverstein adamsilverstein commented May 14, 2026 •

Copy link
Copy Markdown
Member

Overview

Follow-up to #77407 (Phase 5). Carves the auto-save subsystem out of Phase 5 so each PR concentrates on one concern — Phase 5 stays focused on REST permissions + PHP tests, and this PR owns the Google Docs–style background save.

Tracked in #73411.

What's in this PR

  • SuggestionAutoSave — invisible component that debounces overlay edits (1.5 s per block) and persists them as note comments with the _wp_suggestion meta payload. Replaces the explicit SuggestionCommitBar toolbar button (restored on Phase 5 so the prior stack stays functional in isolation).
  • Per-block save queue — saves on the same clientId chain sequentially so a slow network call can't race with a follow-up edit and produce duplicate POSTs or out-of-order writes. Different blocks run concurrently.
  • updateSuggestion / deleteSuggestion on the provider — once a comment id is linked, subsequent edits PUT the same comment; a fully reverted overlay trashes the note. Includes a stale-link check via core-data so a peer-resolved comment doesn't get clobbered mid-session.
  • Overlay plumbing — commentId and syncedOpsKey fields on overlay entries, SET_COMMENT_ID / SET_SYNCED_OPS_KEY reducer cases, and matching setCommentId / setSyncedOpsKey callbacks on the context.
  • Tests — `test/auto-save.js` covers debounce, queue ordering, create/update/delete branching, and stale-link orphan handling. `test/overlay-context.js` covers the new reducer cases.

Test plan

  1. Enter Suggest mode and edit a paragraph.
  2. After ~1.5 s the network shows a `POST /wp/v2/comments` creating the note.
  3. Continue typing — subsequent saves are `PUT`s against the same comment id.
  4. Revert the overlay to baseline — the note is trashed.
  5. Have a peer accept the suggestion mid-session — the next local edit creates a fresh comment instead of clobbering the resolved one.

```bash
npm run test:unit -- packages/editor/src/components/suggestion-mode/
```

Stack

🗺️ PR Stack Navigation

# PR Phase
1 #77403 Intent scaffolding Edit / Suggest / View mode
2 #77404 Overlay capture In-memory suggestion overlay
3 #77405 Provider + Accept/Reject _wp_suggestion meta, provider, sidebar actions
4 #77406 Summary + docs + attribute tests Add/Delete/Formatting summary, architecture stub, conflict scoping
5a #78351 REST permissions and PHP coverage Permissions, payload cap, PHP tests
5b #78352 Summary + attribute conflict + docs Renderer, per-attribute staleness, architecture docs
5c #78353 Surface Apply/Reject in the collaboration sidebar Icon buttons + e2e + sidebar wiring
6 #78308 Auto-save subsystem ← this PR Background debounced save (replaces commit-bar)

📋 Tracking issue: #73411

adamsilverstein and others added 30 commits March 23, 2026 13:11
When clicking "Add Note" on a block that already has an existing note,
the user was being redirected to the reply field of the existing note.
This prevented users from creating multiple independent note threads
on the same block.

This change:
- Adds an `addNewNote` parameter to `openTheSidebar()` to differentiate
  between clicking "Add Note" (should always create new thread) and
  clicking the avatar indicator (should focus existing thread)
- Updates `AddCommentMenuItem` to pass `{ addNewNote: true }`
- Adds E2E tests to verify multiple notes can be added to the same block

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
getNoteIdsFromMetadata is a trivial pure function that normalizes
a scalar/array value. The overhead of useMemo exceeds the cost of
calling it directly each render.

Addresses review feedback from @Mamaduka.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Use direct property access for noteId instead of extracting
metadata into a separate variable.

Addresses review feedback from @Mamaduka.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Clarify why the blur handler needs to bail out during submission:
clicking the submit button triggers blur before the async onSubmit
completes, which would otherwise close the form prematurely.

Addresses review feedback from @Mamaduka.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Prevent blur from closing the add-note form while the async
submit is in progress by guarding with isSubmittingRef. Also
stop the auto-select effect from overwriting the "new" note
selection, and use a ref for noteThreads so the effect only
runs when blockNoteIds changes, not on every thread update.
The first "Note added." snackbar persists when adding a
second note, causing a strict mode violation (2 elements)
and a premature metadata assertion. Dismissing the first
snackbar before adding the second note fixes both tests.
Extend E2E tests with scenarios for deleting one of multiple notes,
resolving individual notes, auto-selecting the first unresolved note,
and verifying metadata cleanup when all notes are removed. Add unit
test edge cases for falsy noteId values, string coercion, and
null/undefined metadata handling.
Reconcile the multiple-notes-per-block data model with trunk's
notes architecture refactor. Keep getNoteIdsFromMetadata /
addNoteIdToMetadata / removeNoteIdFromMetadata utilities and adapt
useBlockComments to iterate arrays of note IDs through trunk's
threadsById partition. Union the utility tests with trunk's
calculateNotePositions coverage.
Resolve conflicts from PR #77614 (Notes refactor) by porting the
multiple-notes-per-block feature into trunk's split component
structure (notes.js, add-note.js, hooks.js, utils.js).
addNoteIdToMetadata and removeNoteIdFromMetadata now compare ids using
String() so a string-typed legacy id ('5') and a numeric id (5) match.
Without this, the dedupe check could miss and the array could contain
duplicates that round-trip differently than expected.

Adds unit coverage for:

- mixed-type dedupe in addNoteIdToMetadata
- mixed-type removal in removeNoteIdFromMetadata
- calculateNotePositions with two and three threads sharing a block
  (identical blockRect.top), exercising the same-block layout path
  introduced by multi-note support.
Adds two e2e tests inside the 'Multiple notes per block' describe:

- A legacy scalar noteId is converted to an array on first multi-note
  write, and the original (orphan) id is preserved as the first entry.
- A block with two notes round-trips through saveDraft + page.reload
  with both notes still attached and the metadata array intact in the
  same order.

Closes the two highest-leverage e2e gaps for multi-note support: the
backwards-compat claim from the PR description, and end-to-end
serialize/parse persistence.
The 'Multiple notes per block' describe was a sibling of 'Block Notes',
not a child, so its inner test.use/beforeEach/afterAll ran in addition
to the file-level ones — every test in that describe was creating each
post twice and deleting all notes twice. Removes the duplicate hooks.

Adds two helpers on BlockNoteUtils to collapse repeated boilerplate:

- dismissSnackbar(text) for clearing the previous toast before asserting
  a new one
- addAnotherNoteToCurrentBlock(content) for the click-add-note + fill +
  submit sequence used by every multi-note scenario

Also adds an inline comment in useNoteActions.onCreate marking the
read-modify-write race that PR #75147 alone cannot fix; a real merge
strategy is tracked in #74751.
The merge from trunk (PR #77614 refactor) kept the branch's pattern of
calling focusNoteThread() directly after selectNote(noteId). That call
fires before the sidebar's useEffect re-renders the AddNote form, so
sidebarRef.current may not yet contain the new treeitem when the
MutationObserver inside focusNoteThread starts watching, and the
synchronous menu close steals focus before the async .focus() resolves.

Switch to selectNote(noteId, { focus: true }) so the existing useEffect
in notes.js drives focus after the sidebar has rendered, matching
trunk's working behaviour. Drops the now-unused focusNoteThread import.
Commit a325780 removed the per-test admin.createNewPost() and
afterAll cleanup from the 'Multiple notes per block' describe on the
premise they duplicated the file-level hooks. They didn't: the
'Block Notes' describe is a sibling, not a parent, so its hooks never
applied to these tests. Removing them left every test in the block
running against whatever page the previous run left behind, which
manifested in CI as page.waitForFunction timeouts on window.wp.blocks
because there was no editor page to test against.

Also fix the save/reload test to explicitly open the notes sidebar
after page.reload() — the pinned sidebar isn't restored on reload, so
the note threads aren't in the rendered DOM until it's reopened.
Restore the selectBlock(clientId, null) call inside openTheSidebar so
the List View entry point — where the block isn't already the canvas
selection — keeps working. The accompanying refactor that introduced
multiple-note support had dropped this in favor of the trimmed function
signature.

Also drop the rawNoteId rename and the redundant '?? null' coalesce on
the noteId selector, per review feedback.
The 'Multiple notes per block' refactor replaced the document.hasFocus
check with the isSubmittingRef guard, but they cover different cases —
window/tab losing focus vs. an in-flight async submit. Keep both so
neither path can prematurely close the form.
Replace the selectedNoteRef/notesRef render-time assignments with a
useMemo that picks the priority thread (first unresolved, else first)
and a thinner effect that bails on the 'new' state or when the user
already has a thread on the block selected. This keeps explicit thread
choices stable while letting status changes promote a new priority,
without writing to refs during render.
Move the new describe block so it sits alongside Keyboard inside the
top-level Block Notes describe and inherits the parent's beforeEach
post-creation and afterAll comment cleanup, instead of duplicating
them at the top level.
`AddNoteMenuItem` is a slot fill rendered per block in the List View
row menus and passes the row's clientId to its onClick handler. The
prior simplification of openTheSidebar's signature dropped that arg
on the floor, which would target the canvas selection instead of the
block whose menu was opened.

Accept an optional clientId option on openTheSidebar, look up threads
for that block directly so the right note is resolved, and pipe the
slot-provided clientId through from AddNoteMenuItem.
The 2709799 refactor moved selectedNote into the auto-select effect's
deps, so collapse paths (Escape/Cancel/Shift-Tab) immediately re-fired
the effect and re-expanded the just-collapsed note. Track selectedNote
in a ref updated inside an effect — keeps Mamaduka's no-refs-during-
render rule and runs the auto-select only when the block context
actually changes.
Links to wordpress-develop#12215, which backports the render_block filter
that strips inline note markers from rendered output.
Inline-note highlights are rendered through the annotations API: each
note adds a `core-note` annotation, which the editor paints as a
`#annotation-text-{id}` mark. Switching to the code editor and back
left these highlights blank until the next block edit or a reload.

Two things conspire:

- Entering the code editor unmounts the canvas RichText, whose
  `core/annotation` change handler removes the note annotations from the
  store. Returning to the visual editor does not re-parse the blocks, so
  `useAnnotateBlocks`'s memoized `annotations` keeps the same reference
  and the add-once effect never re-runs.
- The notes sidebar unmounts in code view, unregistering the `core/note`
  format. On remount the `annotations` memo runs once while the format
  is still missing, so `findNoteInBlock` can't resolve any markers and
  caches an empty result that never recomputes.

Track the `core/note` format registration reactively so the derivation
re-runs once it is registered again, and reconcile the effect against
the annotations actually present in the store (add missing, remove
stale) instead of adding once. The highlights now survive repeated
code-editor round-trips.

Add an e2e regression test covering the round-trip.
@adamsilverstein

Copy link
Copy Markdown
Member Author

Thanks @saroshaga - both are real and now fixed in 39ac4ff.

1. Duplicate note while a save is in flight. You were right: the per-block queue serialized execution, but the new comment id flowed back only through React state (setCommentId) and was read via a ref refreshed in an effect. A save chained behind an in-flight create runs as a microtask the moment the create resolves - before React commits the id - so it read null and POSTed a second note. Fixed by tracking the id (and the synced-ops fingerprint) in synchronous ref maps written the instant a save learns them, so a queued save always sees the latest linkage regardless of render timing. The maps are seeded from overlay state on first touch (remount-safe) and pruned when an overlay entry clears so a reused clientId can't inherit a stale id.

2. Updating a peer-resolved note from stale cache. Also correct - the check read the comment from this tab's core-data cache, which can pre-date a peer's accept/reject. Now we force a fresh read before updating. One subtlety worth flagging: a single-item GET /comments/<id>?context=edit requires moderate_comments, which a suggester usually lacks, so it would 403 and silently fall back to stale cache. Instead it queries the note collection filtered to the id (?post=…&type=note&include[]=…&context=edit) - the path the note REST controller grants to edit_post users, same as the sidebar - and mirrors the result back into core-data.

Both scenarios have regression tests (test/auto-save.js), each verified to fail without its fix.

@adamsilverstein
adamsilverstein force-pushed the suggest-mode-autosave branch 2 times, most recently from cad24a9 to 41e40b1 Compare June 18, 2026 17:12
The suggest-mode stack is meant to ship on top of the inline-notes feature
(#78218): the stack's e2e specs exercise the inline "Add note" rich-text
format and per-author highlight tint, but the implementation (the `core/note`
format, `note-highlight-styles`, and the annotate/format-registration wiring)
was never merged in, so those specs failed.

Merge add/inline-notes-hybrid so the inline-notes layer is a true ancestor.
Conflict resolutions:
- collab-sidebar/note.js: keep inline-notes' collapsible "Show more" content
  and re-apply the suggest-mode SuggestionActions integration (hide Resolve on
  suggestion notes, surface Accept/Reject).
- block-notes.spec.js: take inline-notes' canonical inline-note specs.
- lib/compat PHP: keep the suggest-mode suggestion backend (the `_wp_suggestion`
  meta + REST suggestion-lifecycle permissions); inline-notes only relocated
  the inline-marker stripping.
- CHANGELOG / suppressions: union.
@adamsilverstein
adamsilverstein force-pushed the phase-5b-collab-sidebar-actions branch from 9ec3f09 to 45907a5 Compare June 18, 2026 18:09
Per-block debounced background save for Suggest mode. After a 1.5 s idle
window, pending overlay edits persist as a note comment; subsequent
edits on the same block update the existing note's meta rather than
creating a new one, and a fully reverted overlay trashes the note.
Saves on the same clientId run sequentially via a per-block promise
queue, while different blocks run concurrently. Replaces the explicit
SuggestionCommitBar toolbar button.
Move the auto-save subsystem's ref syncing out of render and into an
effect, matching the store-interceptor fix, so the react-hooks rule
against accessing refs during render passes. Prune the now-unused
auto-save.js suppression along with the stale bulk-suppressions the
stack carried before trunk's ESLint dependency bump. lint:js passes.
…ution races

Address two timing concerns raised in review of the auto-save subsystem.

1. Duplicate note when the comment id has not propagated yet. The per-block
   queue serialized save execution, but the new comment id flowed back only
   through React state (setCommentId) and was read via entriesRef, which is
   refreshed in an effect. A save chained behind an in-flight create runs as a
   microtask the instant the create resolves, before React commits the id, so
   it read null and POSTed a second note. Track the id (and the synced-ops
   fingerprint) in synchronous ref maps written the moment a save learns them,
   so a queued save always sees the latest linkage regardless of render timing.
   The maps are seeded from overlay state on first touch (remount-safe) and
   pruned when an overlay entry is cleared so a reused clientId can't inherit a
   stale id.

2. Updating a note a peer already resolved. The stale-link check read the
   comment from this tab's core-data cache, which can pre-date a peer's
   accept/reject, so a resolved note could be clobbered. Force a fresh read
   before updating, querying the note collection filtered to the id (the path
   the note REST controller grants to edit_post users; the single-item edit
   context requires moderate_comments) and mirror the result back into
   core-data so the sidebar stays in sync.

Add regression tests for both, each verified to fail without its fix.
@adamsilverstein
adamsilverstein force-pushed the suggest-mode-autosave branch from 41e40b1 to d726b49 Compare June 18, 2026 18:11
adamsilverstein added a commit that referenced this pull request Jun 18, 2026
Three e2e tests asserted behavior not present on this branch; they were
masked until the JS build break was fixed and the suite could run.

editor-intent-switcher: the intent is session-scoped by design (see
setEditorIntent / editorIntent reducer) - reload returns to Edit. Rewrite
the reload test to assert that reset instead of expecting Suggest to
persist, which the implementation deliberately refuses.

suggestion-mode: 'auto-saves a content edit' and the heading-level
capture test wait for an automatic debounced POST /wp/v2/comments. The
debounced auto-save subsystem is Phase 6 (#78308), not in this stack's
ancestry; here a suggestion is only persisted via the explicit commit-bar
Submit button. Skip both with a tracking reference to #78308.
@adamsilverstein
adamsilverstein force-pushed the phase-5b-collab-sidebar-actions branch 2 times, most recently from e9d4d4f to 8d2858a Compare June 19, 2026 08:47
Bring the auto-save branch up to date with its base after the stack was
cascaded onto latest trunk.

Reconcile the divergent REST layer onto phase-5b's architecture: phase-5b
defers note permissions to core (Notes GA in 6.9) and layers only the
suggestion-specific behavior via Gutenberg_REST_Comment_Suggestions_Controller
(lib/compat/wordpress-7.1). The older monolithic
class-gutenberg-rest-comment-controller-6-9.php carried by autosave duplicated
core's note-permission handling and was already dead code (never required), so
it is removed; the suggestion REST test now targets the 7.1 controller.

Keep autosave's JS auto-save subsystem (it replaces the commit bar): resolve the
suggestion-mode component conflicts to the auto-save versions and drop the
orphaned commit-bar.js re-introduced by the merge. store-interceptor.js keeps
the effect-synced refs (react-hooks compliant). Regenerate eslint suppressions
and refresh the architecture doc's REST/PHP table.
…ta is registered

The note/suggestion comment meta (`_wp_note_status`, `_wp_suggestion`,
`_wp_suggestion_status`) is registered by `gutenberg_register_block_comment_metadata()`
in `lib/compat/wordpress-6.9/block-comments.php`, but `lib/load.php` only required the
7.1 inline-notes companion — never the 6.9 base file. As a result the function was
undefined at runtime, so the REST suggestion meta was never registered and the
`WP_Test_REST_Comments_Controller_Gutenberg` suite errored on every test
(`Call to undefined function gutenberg_register_block_comment_metadata()`) in its
`set_up()` re-registration hook.

Require the 6.9 file before the 7.1 companion. The file self-hooks on `init` and guards
its helpers with `function_exists`, so it composes cleanly with core's GA notes. Verified:
full PHP suite green (1991 tests, 0 failures) with this require in place.
…ments test

`ReflectionMethod::setAccessible( true )` is deprecated as of PHP 8.5 (it has been
a no-op since 8.1, which grants reflective access automatically). PHPUnit promotes
the deprecation to an error, so `test_lifecycle_update_rejects_non_allowlisted_fields`
and `test_lifecycle_update_accepts_form_encoded_bodies` errored on the PHP 8.5 CI job
while 7.4 passed.

Remove the two no-op calls; the immediately following `->invoke( null, $request )`
still works on 8.1+. Verified green on PHP 8.5.7 (wp-env).
Removing setAccessible() outright fixed PHP 8.5 (where it is a deprecated no-op)
but broke PHP 7.4, which still requires it to invoke the private static
is_suggestion_lifecycle_update() via reflection.

Call setAccessible() only on PHP < 8.1, where it is both required and not
deprecated; on 8.1+ reflective access is automatic and the call is skipped to
avoid the 8.5 deprecation. Verified the pattern on PHP 7.4.33 and 8.5 (both
invoke cleanly, no deprecation); PHPCS clean.
The `static-checks` CI job (lint:js) failed with "There are suppressions left
that do not occur anymore" — the trunk cascade (and the suggestion-mode code
changes) eliminated violations whose entries still lingered in
`tools/eslint/suppressions.json`. Ran `npm run lint:js:prune-suppressions` to
drop the 92 now-unused entries. Verified `lint:js`, `lint:css`, type-check
build, and `other:check-local-changes` all pass.
… into suggest-mode-autosave

# Conflicts:
#	phpunit/experimental/class-wp-rest-comments-controller-gutenberg-test.php
#	tools/eslint/suppressions.json
The phase-5a fix added a wordpress-6.9/block-comments.php require, which
the merge into autosave duplicated against the one this branch already
had. Two plain require() calls of the same file trigger a redeclare
fatal that breaks PHP boot and every Playwright shard. Drop the
duplicate so the file loads once.
@github-actions

Copy link
Copy Markdown

Flaky tests detected in bab0c8a.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28148665510
📝 Reported issues:

Absorb the inline-note tint test removal from 5b without applying it here:
the per-author tinting feature (note-highlight-styles.js) lives on this
branch, so the test passes and must stay. Recording the merge keeps future
5b->autosave cascades from re-deleting it.
@adamsilverstein adamsilverstein changed the title Suggest mode: Auto-save subsystem (Phase 6) Suggest mode: Auto-save subsystem for pending suggestions Jun 30, 2026
@adamsilverstein adamsilverstein changed the title Suggest mode: Auto-save subsystem for pending suggestions Auto-save subsystem for pending suggestions Jun 30, 2026
@adamsilverstein

Copy link
Copy Markdown
Member Author

Superseded by the fresh-stack restructure of Suggest mode (#73411).

The feature has been re-sliced from this 16-deep stack onto current trunk as a clean 2-PR stack:

The whole feature now sits behind a Suggestion Mode experiment (Settings → Experiments → Collaboration). Closing in favour of the new stack — details: #73411 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backport from WordPress Core Pull request that needs to be backported to a Gutenberg release from WordPress Core [Feature] Notes Phase 3 of the Gutenberg roadmap around block commenting [Package] Editor /packages/editor [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants