Repository navigation
Auto-save subsystem for pending suggestions - #78308
adamsilverstein wants to merge 113 commits into
Conversation
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>
This reverts commit 7d2ff81.
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.
|
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 ( 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 Both scenarios have regression tests ( |
cad24a9 to
41e40b1
Compare
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.
9ec3f09 to
45907a5
Compare
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.
41e40b1 to
d726b49
Compare
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.
e9d4d4f to
8d2858a
Compare
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
… 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.
|
Flaky tests detected in bab0c8a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28148665510
|
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.
|
Superseded by the fresh-stack restructure of Suggest mode (#73411). The feature has been re-sliced from this 16-deep stack onto current
The whole feature now sits behind a Suggestion Mode experiment (Settings → Experiments → Collaboration). Closing in favour of the new stack — details: #73411 (comment) |
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 asnotecomments with the_wp_suggestionmeta payload. Replaces the explicitSuggestionCommitBartoolbar button (restored on Phase 5 so the prior stack stays functional in isolation).clientIdchain sequentially so a slow network call can't race with a follow-up edit and produce duplicatePOSTs or out-of-order writes. Different blocks run concurrently.updateSuggestion/deleteSuggestionon the provider — once a comment id is linked, subsequent editsPUTthe 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.commentIdandsyncedOpsKeyfields on overlay entries,SET_COMMENT_ID/SET_SYNCED_OPS_KEYreducer cases, and matchingsetCommentId/setSyncedOpsKeycallbacks on the context.Test plan
```bash
npm run test:unit -- packages/editor/src/components/suggestion-mode/
```
Stack
🗺️ PR Stack Navigation
_wp_suggestionmeta, provider, sidebar actions📋 Tracking issue: #73411