Notes: Refactor internals into smaller components#77614
Merged
Conversation
|
Size Change: -160 B (0%) Total Size: 7.82 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 621b929. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25047479527
|
t-hamano
reviewed
Apr 24, 2026
Contributor
t-hamano
left a comment
There was a problem hiding this comment.
Nice work! It looks good. I would like to suggest some additional name adjustments, but it is not a blocker.
CommentIconSlotFilltoNoteIconSlotFillblockCommentIdtonoteIdblockCommentUtilstoblockNoteUtilsaddBlockWithCommenttoaddBlockWithNoteclickBlockCommentActionMenuItemtoclickBlockNoteActionMenuItem
|
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. |
8f5dd79 to
46c5655
Compare
Member
Author
|
@t-hamano, feedback should be addressed now. |
46c5655 to
621b929
Compare
Member
|
Excellent work @Mamaduka! |
adamsilverstein
added a commit
that referenced
this pull request
Apr 29, 2026
…omment-type Resolve conflicts from trunk's collab-sidebar refactor (#77614): - Adopt new component structure (notes.js, note.js, note-thread.js) - Rename useBlockCommentsActions → useNoteActions; keep reactionsMap - Thread onToggleReaction and reactionsMap through Notes → NoteThread → Note - Render AddReactionButton + ReactionDisplay inside Note when isSelected - Delete obsolete comments.js - Replace experimental HStack with Stack from @wordpress/ui - Add missing useCallback import in hooks.js
adamsilverstein
added a commit
that referenced
this pull request
Apr 29, 2026
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).
t-hamano
added a commit
that referenced
this pull request
Apr 29, 2026
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
adamsilverstein
added a commit
that referenced
this pull request
Apr 29, 2026
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.
adamsilverstein
added a commit
that referenced
this pull request
Apr 29, 2026
Trunk refactored the collab-sidebar (#77614, etc.), splitting comments.js into note.js / note-thread.js / note-card.js. Re-port the phase 3 hooks - SuggestionActions in the body and SuggestionActionButtons in the header, plus the Resolve-button suppression for suggestion threads - into the new note.js.
adamsilverstein
added a commit
that referenced
this pull request
Apr 29, 2026
The merge of trunk's Notes refactor (#77614) into phase-3 ported a SuggestionActionButtons reference into the new note.js, but that named export does not exist on phase-3 — only `SuggestionActions` (default) ships in this phase. Header-slot icon buttons are a downstream refinement and will arrive in a later phase. Removes the unused named import and the header-slot usage so the package build no longer fails with 'No matching export'.
adamsilverstein
added a commit
that referenced
this pull request
May 14, 2026
* Try: add support for multiple note ids
* Apply code improvements from Code Rabbit
* Fix "Add Note" to create new note thread instead of focusing reply field
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>
* complete enabling multiple notes per block
* ensure most recent selected when adding additional note
* Remove unnecessary useMemo around getNoteIdsFromMetadata
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>
* Simplify useSelect by removing intermediate metadata variable
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>
* Add comment explaining isSubmittingRef guard in AddComment
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>
* remove blur guard
* Revert "ensure most recent selected when adding additional note"
This reverts commit 7d2ff81.
* Fix prettier formatting in comments.js
* Fix race conditions when adding a new note
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.
* Fix prettier formatting in collab-sidebar
* Fix duplicate snackbar in multi-note E2E tests
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.
* Add tests for multiple notes per block feature
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.
* Fix toolbar intercept in auto-select E2E test
* Notes: dedupe noteId mixed-type comparison and add layout coverage
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.
* Notes: add e2e coverage for legacy migration and reload persistence
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.
* Notes: tidy multi-note e2e tests and document race condition
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.
* Notes: restore focus parity with trunk in openTheSidebar
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.
* Notes: restore beforeEach/afterAll for Multiple notes per block tests
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.
* Notes: restore selectBlock call and simplify metadata selector
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.
* Notes: restore window-focus guard on AddNote blur handler
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.
* Notes: derive auto-select target outside the effect
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.
* Notes: nest 'Multiple notes per block' inside Block Notes describe
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.
* Notes: keep List View entry point routing through the row's clientId
`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.
* Notes: stop auto-select effect from re-expanding collapsed notes
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.
* Notes: pin first-id-stays-first contract for note id metadata
Adds a focused 'note id order preservation' suite covering the
invariant the multi-note design depends on: the first id in a block's
metadata is the first (block-aligned) note. Verifies that
addNoteIdToMetadata appends without reordering across multiple
sequential adds, that removeNoteIdFromMetadata preserves the order of
the surviving ids regardless of which id was removed, that an
interleaved add/remove sequence still yields the expected order, and
that migrating a legacy scalar id to an array keeps the original id
first.
* Notes: cleanups for multi-notes-per-block
- `useNoteThreads`: numeric Map keys; drop `Number(noteId) ?? noteId` double-lookup
- extract `pickPrimaryNote` (first unresolved else first); replace 3 inline copies in `index.js`/`notes.js`
- auto-select effect: `prevBlockIdRef` change-detect; drop `selectedNoteRef` workaround
- collapse `blockNoteIds`/`targetNoteId` into one `useMemo`
* Simplify new e2e tests and helpers
* Improve coverage for selection sync effect
* Notes: drop e2e tests already covered by unit tests
Three e2e tests in block-notes.spec.js exercise pure metadata
transforms or generic block serialization rather than user-visible
behavior:
- 'metadata noteId is cleaned up when all notes are deleted' duplicates
removeNoteIdFromMetadata unit tests asserting noteId becomes
undefined when the array empties (including from a legacy scalar).
- 'lazy-migrates a legacy scalar noteId to an array' duplicates
addNoteIdToMetadata unit tests covering scalar-to-array migration
with legacy id preserved as the first entry.
- 'multi-note metadata persists across save and reload' tests
block-editor metadata serialization, not multi-notes-specific
behavior; array shape and order preservation are pinned by the
order-preservation unit tests.
Drop them — unit tests run faster and cover the same invariants more
precisely.
* Notes: normalize stored note ids to numbers
getNoteIdsFromMetadata now coerces values to finite positive numbers,
so the new array format always contains numeric ids regardless of
whether legacy metadata stored a string. This drops the string/number
String() comparison shim from addNoteIdToMetadata and
removeNoteIdFromMetadata, and the redundant Number coercion from the
useNoteThreads consumer.
* Notes: simplify noteId helpers with Set
Refactor getNoteIdsFromMetadata, addNoteIdToMetadata, and
removeNoteIdFromMetadata to use Set instead of array operations.
Set preserves insertion order (so the first-note-is-block-aligned
contract still holds) and also deduplicates the normalized output,
which prevents the sidebar from rendering the same thread multiple
times if metadata ever contains repeated ids.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: Aki Hamano <tetsuaki.hamano@gmail.com>
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?
Hopefully, one last large refactoring for this feature, which evolved organically and required some corners to be cut.
Changes in this PR:
comment.jsinto smaller and focused components.NotesSidebarContent.P.S. I'm really sorry for all the merge conflicts, but this had to be done at some point, sooner rather than later (in the next release cycle).
Why?
Smaller components mean that changes like #74985 or #77446 should be cleaner to implement. Now only the
NoteorNoteCardcomponent is responsible for display, and those states can be locally, instead of affecting the whole tree.Testing Instructions
test/e2e/specs/editor/various/block-comments.spec.js.Testing Instructions for Keyboard
Same.
Use of AI Tools
Claude, by using my initial sketch/idea. Reviewed and tested myself.