Notes: inline (partial-text) notes via hybrid marker + strip-on-render approach#78218
Open
adamsilverstein wants to merge 47 commits into
Open
Notes: inline (partial-text) notes via hybrid marker + strip-on-render approach#78218adamsilverstein wants to merge 47 commits into
adamsilverstein wants to merge 47 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.
Contributor
|
Wild and beautiful! 
Impressive. One thing, the marked text at rest, when not selected, is quite stark: 
Since we have, courtesy of @t-hamano, an array of colors assigned to users when they multi-edit, can we use those same colors for the markers at all time, and simply change the opacity based on selected or not? I would start there, and if it's insufficient for color contrast, we should add text-underlines that are 2px or thicker, in solid colors, to indicate the marked text. |
The previous highlight used the admin theme color at 20%/40% opacity via color-mix(), which stylelint bans and which @jasmussen flagged as too stark at rest. Switch to AVATAR_BORDER_COLORS (the same palette used for multi-user editing) so each marker carries its author's color, and emit the per-note background rules from a small <NoteHighlightStyles> component that targets the annotation IDs the API already renders. Opacity steps up on hover, focus-within, and when the matching thread is the editor's selected note.
Member
Author
Good suggestion, I added this in 74c9b9f |
Member
Author
This isn't quite working in my testing, working on it some more. |
Switch NoteHighlightStyles from rendering a <style> element in the sidebar to calling useStyleOverride() so the per-note background rules reach the iframed editor canvas where the <mark> annotations actually render. Also adds a base reset for mark.annotation-text-core-note so the browser's default yellow doesn't show through before the per-author rule applies.
Extract buildHighlightCss() as a pure helper so the per-author tint logic can be unit-tested without touching React, and add the matching test suite alongside the existing collab-sidebar/test/ folder. Also add a Playwright spec that selects text, runs the inline 'Add note' flow, and asserts the rendered <mark> carries the current user's avatar color at the rest opacity — guards against regressions in the iframe canvas style override too.
4 tasks
Member
Author
|
Selections with avatar based colors: 
|
Within a block, notes were rendered in creation order, which forces the reader to mentally scan from highlight to highlight to find the one they're reading. Sort each block's notes by inline marker offset instead so the sidebar mirrors top-to-bottom reading order. Block-level notes (no inline anchor) sort first within their block as the 'overall comment'; same-offset ties fall back to note id. Across blocks, useNoteThreads already iterates getClientIdsWithDescendants in document order, so the existing behaviour carries over for free — reorder a block and the sidebar follows. Extracts getInlineMarkerStart() so the sort key can be unit-tested against rich-text fixtures.
Member
Author
|
When testing this, I felt that avatar colors for user id 1 and 3 where very close and difficult to distinguish. I opened a follow up Issue to discuss this here: #78255 |
This was referenced May 13, 2026
Contributor
|
Fantastic work. |
Member
Author
|
In bc8c9f8 I adjusted the sorting of notes when there are multiple selections per block. The order now matches the selection order (within the block) instead of following chronological order helping the notes relate better to the selections. Thinking the pinned sidebar would still show the notes in reverse chronological order. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Implements inline (partial-text) Notes — the next milestone in #59445, listed as a WordPress 7.1 deliverable in #76316.
Hybrid of the two earlier exploration PRs from @Mamaduka:
core/noterich-text format (resilient anchoring, but pollutes frontend HTML).This PR uses
core/noteas the in-content anchor so the marker survives edits, and strips the span wrapper onrender_blockso public HTML stays clean. Visual highlight is applied via@wordpress/annotationsso the styling rides on existing infrastructure.Stacked on
Builds on #75147 (multi-note per block). The array shape from #75147 is required so a single block can carry multiple inline notes. Base for review:
add/enable-multiple-notes-per-block— change totrunkonce #75147 merges.Screenshot
How?
Storage
_wp_note_selection={ attributeKey, start, end }registered inlib/compat/wordpress-6.9/block-comments.php. Fallback anchor when the in-content marker can't be found.metadata.noteIdarray (from Notes: Support multiple note threads per block #75147) tracks which threads belong to a block — inline notes are added to it like block-level notes.Anchoring
core/noterich-text format, registered inpackages/editor/src/components/collab-sidebar/format.js. Serializes as<span class="wp-note" data-id="N">…</span>.useNoteActions.onCreatenow captures a non-collapsed in-rich-text selection, saves the comment with_wp_note_selectionmeta, and wraps the selected text with thecore/noteformat.findNoteRange( richText, noteId )(new inutils.js) scans a rich-text record for the marker and returns its current{ start, end }— meaning offsets re-derive after edits rather than going stale.Frontend HTML stays clean
gutenberg_strip_inline_note_markers()is hooked torender_block. It unwraps<span class="wp-note">…</span>so the public output never contains the marker, while rawpost_content(and the RESTrawview, revisions, exports) keeps the mark so the editor can re-attach on reload.Visual decoration
useAnnotateBlockshook reads unresolved threads, prefersfindNoteRangefor offsets, falls back to the stored meta, and registers each range with the annotations store under thecore-notesource..annotation-text-core-noteuses admin-theme tinting at 20% / 40% opacity (matching @jasmussen's design comment in Inline block commenting #59445). Always visible — no need to open the sidebar.Forward compatibility with Yjs / CRDT
Real-time collaboration with Yjs is on the roadmap (see
crdt-blocks.tswork). The architecture here is intentionally shaped so the inline-note feature carries over with a single, localized swap rather than a rewrite:findNoteRangere-scans the rich-text record for the marker on every read. There are no absolute indices baked into the document. This matches how CRDT positions work — they are references that resolve to current offsets, never frozen ones. When Yjs lands,findNoteRangeis the single function that switches from "scan for marker" to "resolveY.RelativePosition/ yjs range," and every consumer (useAnnotateBlocks, sidebar, anchor preview) keeps working unchanged.data-id="N"and the block'smetadata.noteIdarray stores comment IDs. The thread ↔ anchor relationship is identity-based, which is the same model a yjs-anchored range would use (a stable id pointing at a CRDT range). Migrating means swapping the anchor representation, not the linkage model.(content, threads)and never written back into the content or the store. Whatever produces{ start, end }— today the marker, tomorrow a yjs range — feeds the sameuseAnnotateBlockspipeline. The styling layer is yjs-ready as-is.gutenberg_strip_inline_note_markers()is a singlerender_blockfilter in one file. When CRDT anchoring replaces the in-content marker, deleting that filter (and thecore/noteformat registration) is the entire frontend-cleanup story — no scattered cleanup across rendering, REST, or export paths._wp_note_selectionis stored on the comment, not on the block. It already works without the in-content marker, so it remains a valid fallback during and after a CRDT migration (e.g., for legacy posts authored before yjs was enabled).metadata.noteIdis an unordered set of IDs per block — no positional coupling between the array order and where the notes anchor in text. Cross-block notes (#73416) and yjs-anchored notes both fit this shape without schema changes.Net effect: when
crdt-blocks.tsexposes range/position primitives, the migration is roughly (1) swap the body offindNoteRange, (2) drop thecore/noteformat + strip filter. The data model, the sidebar, the annotations decoration, and the storage in comment meta all carry over.Testing instructions
<span class="wp-note">in the HTML, content reads cleanly.Automated tests
JS: 7 new cases in
test/utils.jsforfindNoteRangecovering RichTextData/string input, id matching/mismatch, numeric/string coercion, and null guards. 63 total in this suite, all passing.PHP: 6 new cases in
phpunit/tests/strip-inline-note-markers-test.phpcovering single/multiple wrappers, no-op when no marker, classes alongside others, unrelated spans, and end-to-endrender_blockfilter behavior.Follow-ups (out of scope)
noteIdsshape from Notes: Support multiple note threads per block #75147 supports this; UI/storage work is its own iteration.crdt-blocks.tsexposes range primitives, retire the in-content marker in favor of CRDT-anchored ranges and remove the strip filter.AI usage
Implementation drafted with Claude Code: spec, plan, and code. Plan reviewed and approved by me before edits, all commits and the PR reviewed before push.
Refs #59445, #76316.