Notes: Share thread focus recovery across actions - #85455

🤖 PR meta 🤖

🎉 Props

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Updated as activity occurs, without notifying anyone named here. Add the props-bot label to refresh.

⚡ Performance

_{8996bf5 Run}

🏁 Flaky tests

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.

Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Array [
    "rewrite_rules",
-   "SERIALIZED DATA",
+   "",
  ]
    at forEach (/home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/core-settings.spec.js:46:26)
    at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/core-settings.spec.js:43:35

Error: expect(locator).toBeVisible() failed

Locator: getByRole('region', { name: /^(Notes|Editor settings)$/ }).getByRole('treeitem', { name: 'Note: Four: Comprehensive considerations regarding extraordinarily sophisticated implementation characteristics notwithstanding.' })
Expected: visible
Timeout: 5000ms
Error: element(s) not found

Call log:
  - Expect "toBeVisible" getByRole('region', { name: /^(Notes|Editor settings)$/ }).getByRole('treeitem', { name: 'Note: Four: Comprehensive considerations regarding extraordinarily sophisticated implementation characteristics notwithstanding.' }) with timeout 5000ms
  - waiting for getByRole('region', { name: /^(Notes|Editor settings)$/ }).getByRole('treeitem', { name: 'Note: Four: Comprehensive considerations regarding extraordinarily sophisticated implementation characteristics notwithstanding.' })

    at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/block-notes-floating.spec.js:589:4

_{8996bf5 Run}

Code looks good, I'm going to do some manual testing in Playground.

This worked well in my manual testing. I also asked Claude to go thru and try various Note actions, tracking the focus and will post the results shortly.

This tested well and pulling the focus logic into one place makes sense. I had Claude test this in Playground and it worked as expected.

Claude reports:

Tested with three notes on three paragraphs in the "All notes" sidebar, checking focus and block selection after each action:

Scenario	Result
Delete the first (selected) note	Focus moves to the second note, its block is selected
Delete the last note	Focus moves to the previous note
Resolve a note	Focus stays on the note, which collapses
Escape on a selected note	Focus stays on the note, which collapses
Cancel a reply	Focus stays on the thread, which collapses
Delete a reply	Focus goes to the parent thread, which stays open
Delete notes whose blocks are gone, in a row	Focus moves to the next note each time

Step 4 (deleting the last note) behaved the same as the base commit in headless Chrome: the block is selected and its paragraph gets focus, then focus drops to the canvas body right away.

A few notes from reading the code:

• Delete looks up the neighbor after the request resolves, which works because the unmounting NoteThread still holds the threads list from before the delete. Makes sense, though a short comment there might save the next reader a minute.
• The neighbor is now only selected when the deleted thread was the selected one, where trunk always selected it. That seems more correct, and could be worth a line in the description.
• Calling useNoteActions per thread looks cheap, since it only reads selectors and dispatchers without per-thread subscriptions.
• No e2e test covers where focus lands after a delete yet. Steps 2-4 would make a cheap regression test before Notes: Add search and a status filter to the All Notes sidebar #84060 builds on this.

Thanks, Adam!

My Claude review reported similar cases. I ignored the extra description, which LLMs seem to like for some reason. The useNoteActions usage is acceptable; I reuse similar hooks all the time.