Repository navigation
Add a Notes section in the Dashboard Activity widget - #13076
mcrisp1972 wants to merge 41 commits into
Conversation
…p into ticket/65890
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
WP_Comment exposes its columns as strings, so passing comment_post_ID straight to _draft_or_post_title() and get_edit_post_link() fails static analysis. It also repeated an error pattern the PHPStan baseline records once for the comments widget, pushing that entry out of date.
…p into ticket/65890
Tallying the open notes of the displayed posts loaded every one of those notes just to count rows, an unbounded query on a busy post. Query each post's count instead, which returns a single value and skips priming the comment and meta caches.
Covers the edit link, the post title and its placeholder, escaping, the open note count and its singular and plural forms, and the relative date each row ends with, including its use of the timezone of the site. The dashboard had no test coverage of its own before this.
…old. A note is created on hold and is approved when it is resolved, so the row tests now create their notes the way the editor does. Adds coverage of the listing itself: the open note count, which posts are listed and in what order, and the exclusion of resolved threads, replies and regular comments.
The section queried the notes that start a thread and dated each post by one of those, so a thread opened weeks ago and replied to this morning was dated weeks ago, and sorted below a thread nobody had touched since it was opened. Query replies too, and date each post by the most recent note or reply in a thread that is still open. Resolving a thread approves the note that starts it while its replies stay on hold, so the thread a reply belongs to is checked before the reply counts as activity. The threads are primed in one query per page rather than fetched one at a time. Also pass 'orderby' => 'none' to the open note counts, which do not need the ordering, matching get_comment_count().
|
Working on this a few points emerged:
|
Notes are only listed for posts the current user can edit, so a user who can edit none of them paged through every open note on the site, one query per page. Stop paging once a hundred notes per row have been examined, and prime the posts the capability checks load while skipping the note meta they never read.
The row put the post title in the narrow first column the sibling sections use for the date, so a title of any length wrapped while the open note count stretched across the free space. Render the date first, the title in the column that grows, and the count last, and drop a duplicated block of rules left over from an earlier iteration. Skip a row whose post no longer has an edit link along the way, as esc_url() is passed null for a post deleted since the notes were queried.
|
I had Claude review the branch, and pushed what it turned up: Two commits, both on top of the existing work here. d4d4678 bounds the paging loop. Notes are only listed for posts the current user can edit, so a user who can edit none of them - a Subscriber, or an Author on a multi-author site - never satisfied the c8c9b7f is the row layout. The markup was Tests came along with each: the paging bound, the edit link guard, and the two row helpers that read the date out of the markup. One review note that turned out to be a non-issue, for the record: @mcrisp1972 does the date-first row order look right to you? Happy to put it back if you had a reason for leading with the title. |
@adamsilverstein , Jen
@adamsilverstein , this is looking good. Regarding the ordering of row date and title, Jen and @aosmichenko built out the rows, so we might want their input on that, but I think the date first makes sense, especially if that is the sorted field. |
|
I was initially thinking of making "time ago" format, so if you would rather do that instead. I have no hard preferences on the format and column order, was trying to find the most useful to read. |
I like the human readable time ago format, we use that for most user facing dates. |
Review on the PR favored the relative "time ago" format over the Today/Tomorrow/date wording, matching how most user facing dates are shown elsewhere in the admin, such as the Media list table and the revisions screen. Compute the difference from the GMT date of the note so it is correct in any site timezone, which also removes the dead branch guarding a non-integer timestamp: get_comment_date( 'U' ) always returned one. The separate clock time is dropped because the difference already carries it, taking the '%1$s, %2$s' join and the date format strings out of the translation catalog. The row markup keeps its shape, so the column layout and dashboard.css are unchanged.
Co-authored-by: Weston Ruter <westonruter@gmail.com>
The row helper was a new global function with a single caller, which adds API surface core would have to keep. Rendering the rows in wp_dashboard_recent_notes() with a template also reads closer to the markup it produces than a printf() format string. The edit link check moves into the selection loop, so a post without one no longer takes up a row slot and leaves the section short. The row tests fold into the section tests, matching the markup whitespace-tolerantly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A71VCCi5zWeU1wpawPZqnx
That's a great idea to add it to the usual default intro post so folks can help discover it. Let's try it! |
Ok, I'll add this! |
`strtotime()` returns false for a date it cannot parse, and passing that to `human_time_diff()` is flagged by PHPStan. Skip the row instead, as there is no meaningful time to show for it. Claude-Session: https://claude.ai/code/session_01Dww9xkqg9XGQfHH3jwwSZo
A fresh install shows the default comment under Recent Comments in the Activity dashboard widget, but Recent Notes stays empty, so nothing points new users at Notes. Add an open note by the site admin and link it to the first block of the "Hello world!" post through the block's `metadata.noteId` attribute, so the note shows in the widget and on its block in the editor. No note is added when the first post has no blocks to attach it to, such as a multisite network's plain text first post option. Claude-Session: https://claude.ai/code/session_01Dww9xkqg9XGQfHH3jwwSZo
|
Thanks @annezazu, glad the idea made sense. Fresh installs now get a default note so Recent Notes shows up alongside Recent Comments. Claude Code wired this up and wrote the summary:
Should we also turn the multisite default |
|
…null` Fixes: ------ --------------------------------------------------------------------------------------------------------------------------------------------------- Line src/wp-admin/includes/dashboard.php ------ --------------------------------------------------------------------------------------------------------------------------------------------------- 1316 Parameter WordPress#1 $url of function esc_url expects string, string|null given. 🪪 argument.type ------ ---------------------------------------------------------------------------------------------------------------------------------------------------
Fixes the missingType.return PHPStan errors reported for `wpSetUpBeforeClass()` and each test method in `Admin_Includes_Dashboard_WpDashboardRecentNotes_Test`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The helper returned a bare `array`, so every value destructured from it
was `mixed`, failing the string parameters of `get_linked_titles()`,
`assertMatchesRegularExpression()`, and `substr_count()`. Return
`array{0: bool, 1: string}` instead, casting the buffer like `get_echo()`
does, and declare `$total_items` as `positive-int` to match
`wp_dashboard_recent_notes()`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es test `get_edit_post_link()` returns `null` when the post type does not exist, which `esc_url()` does not accept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`get_post()` returns `null` for a missing post, so the helper did not satisfy its `WP_Post` return shape, and reading `comment_count` from it directly accessed a property on a possibly-null value. Assert the instance first, after restoring the current blog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Parsed block attributes are untyped, so reaching into `['metadata']['noteId']` accessed offsets on `mixed`. Asserting the entire `attrs` array avoids that, and is stricter too: it also checks that no other attributes were added to the block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Declares the types already documented in the docblocks of the helpers in the `wp_dashboard_recent_notes()` and multisite install defaults tests, plus the `get_edit_post_link` filter callback, resolving PhpStorm's "Missing parameter's type declaration" inspection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Declares the return types already documented on the test helpers, and types the static user ID properties as `int`, resolving PhpStorm's "Missing function's return type declaration" and "Missing property's type declaration" inspections. The factory's `create()` now throws rather than returning a `WP_Error`, so the `int` return types are safe. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I've fixed the PHPStan errors in the new PHPUnit tests. |
westonruter
left a comment
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Findings from a medium-effort code review of the branch for Core-65890. They are left as inline comments, roughly in order of severity:
- The examined-notes cap can hide a user's own open notes entirely (
dashboard.phpL1199). - A
$total_itemsof0removes the query limit (L1182). - Paging keeps querying after a short page, and offsets can drift (L1259).
- The capability check is redundant with
get_edit_post_link(), and the link is computed twice (L1248). - Rejected posts are re-checked for every one of their notes (L1248).
- One
COUNTquery per displayed post (L1275). - Future-dated or zero-dated notes get a misleading relative time (L1308).
serialize_blocks()re-encodes the whole network-suppliedfirst_post(upgrade.phpL357).- The default note text cannot be customized per network (
upgrade.phpL338).
Item 1 changes behavior that test_should_stop_paging_after_a_hundred_notes_per_row currently asserts, so it is a design question as much as a bug.
| * that the section costs a bounded number of queries however many notes the | ||
| * site has. | ||
| */ | ||
| $max_notes_examined = $total_items * 100; |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The cap can silently hide a user's own open notes, and the section then reports that there are none.
An Author can only edit their own posts. If 500 or more newer open notes sit on other people's posts (100 or more when $total_items is 1), paging stops before reaching the Author's notes. The function returns false and no Recent Notes block appears, even though the Author has open notes. test_should_stop_paging_after_a_hundred_notes_per_row currently asserts exactly this outcome.
Scoping the query to posts the user can edit, instead of paging past the ones they cannot, would avoid the miss. For example, restrict to post_author for users without edit_others_posts, or use a post__in of editable posts.
| 'status' => 'hold', | ||
| 'orderby' => 'comment_date_gmt', | ||
| 'order' => 'DESC', | ||
| 'number' => $total_items * 5, |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The native int type from 5fe68ed rules out strings, but 0 still gets through. 'number' => 0 is treated by WP_Comment_Query as no limit (absint() gives 0, so no LIMIT clause is added), so every open note on the site is loaded.
$max_notes_examined is then also 0, so the loop exits after that single pass. Since count( $latest_notes ) === 0 is never true, every post with an open note is rendered instead of none.
The positive-int in the docblock is not enforced at runtime. An early if ( $total_items < 1 ) { return false; } would guard against it.
| } | ||
| } | ||
|
|
||
| $notes_query['offset'] += $notes_query['number']; |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
When a page comes back with fewer rows than 'number', the loop still runs another get_comments() at the next offset, which is bound to be empty. Breaking when count( $possible ) < $notes_query['number'] saves that round trip.
Separately, paging by offset over a live, date-ordered result set can skip a note if one is added or resolved between page queries. That is probably acceptable for a dashboard widget, but worth noting.
| * Notes are only visible to users who can edit the post they belong to, | ||
| * and each row links to the post's edit screen. | ||
| */ | ||
| if ( ! current_user_can( 'edit_post', $note_post_id ) || ! get_edit_post_link( $note_post_id ) ) { |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
current_user_can( 'edit_post', $note_post_id ) is redundant here, because get_edit_post_link() already returns null when the user cannot edit the post (see link-template.php).
The link is also computed again in the render loop (L1300), where the ! $edit_post_link branch can no longer be reached. Storing the link alongside the note in $latest_notes here would drop both the duplicate capability check and the second get_edit_post_link() call per row.
Also, posts that fail this check are not remembered. With 100 open notes on a post the user cannot edit, the capability and link checks run 100 times for the same post ID. Keeping a set of skipped post IDs, checked next to the isset( $latest_notes[ $note_post_id ] ) short-circuit, would reduce that to one check per post.
| $open_notes = array(); | ||
|
|
||
| foreach ( array_keys( $latest_notes ) as $note_post_id ) { | ||
| $open_notes[ $note_post_id ] = (int) get_comments( |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
The comment above explains why a count query per post is preferred over loading the notes. Even so, this still costs up to $total_items extra queries on every dashboard load that has notes.
A single grouped query would return all the counts in one round trip without loading any notes. It would select comment_post_ID, COUNT(*) with comment_post_ID IN (…), comment_type = 'note', comment_parent = 0 and comment_approved = '0', grouped by comment_post_ID.
| $note_post_title = _draft_or_post_title( $note_post_id ); | ||
|
|
||
| /* translators: %s: Human-readable time difference. */ | ||
| $note_time_diff = sprintf( __( '%s ago' ), human_time_diff( $note_time ) ); |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Minor: human_time_diff() gets a single argument, and the label is always "%s ago". So a note whose comment_date_gmt is ahead of the server clock (clock skew, or imported data) is shown as "X mins ago". A zero date (0000-00-00 00:00:00) is shown as "56 years ago".
The is_int() guard on L1296 never catches either case, because strtotime() returns an int for both.
|
|
||
| $wpdb->update( | ||
| $wpdb->posts, | ||
| array( 'post_content' => serialize_blocks( $first_post_blocks ) ), |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
Running the network-supplied first_post through parse_blocks() and then serialize_blocks() re-encodes the attributes of every block, not only the one being annotated. Characters such as &, <, > and -- come out as \u0026 and so on, and comment-delimiter spacing is normalized. The stored post content then no longer matches the configured network option byte for byte.
Rewriting only the first block's opening delimiter, for example with WP_Block_Processor or a targeted replacement of that one delimiter, would leave the rest of the content untouched.
| 'comment_author_url' => $note_author->user_url, | ||
| 'comment_date' => $now, | ||
| 'comment_date_gmt' => $now_gmt, | ||
| 'comment_content' => __( 'Hi, this is a note. Notes let you leave feedback on a block for anyone else working on this post. To add one, select some text or an entire block and choose Add note from its options menu. Resolve a note once its feedback has been addressed.' ), |
There was a problem hiding this comment.
🤖 Comment by Claude Opus 5.5
On multisite, every other piece of default content (first_post, first_page, first_comment, and so on) can be overridden through a network option. The default note text cannot.
A network that customizes first_post still gets this instructional note on every new site's first post, with its noteId written into the network's custom content. Short of overriding the pluggable wp_install_defaults(), there is no way to change or suppress it. A first_note network option, or at least a filter, would bring it in line with the rest.
| $first_block_meta = isset( $first_block_attrs['metadata'] ) && is_array( $first_block_attrs['metadata'] ) ? $first_block_attrs['metadata'] : array(); | ||
|
|
||
| $first_block_meta['noteId'] = $wpdb->insert_id; | ||
| $first_block_attrs['metadata'] = $first_block_meta; |
There was a problem hiding this comment.
Note there are two PHPStan errors on these lines:
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
Line src/wp-admin/includes/upgrade.php
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
348 Cannot access offset 'metadata' on mixed.
🪪 offsetAccess.nonOffsetAccessible
351 Cannot access offset 'metadata' on mixed.
🪪 offsetAccess.nonOffsetAccessible
------ ---------------------------------------------------------------------------------------------------------------------------------------------------
They are being addressed separately in #14457


We are adding notes posted to blocks in the editor to the Recent Activity widget in the admin dashboard.
Trac ticket: https://core.trac.wordpress.org/ticket/65890
Testing
Tool to create many notes: https://github.com/t-hamano/notes-data-generator
https://playground.wordpress.net/wordpress.html?pr=13076
Screenshot
More screenshots
Claude seeded a site with notes and captured the widget in each state:
The test site has six posts carrying note threads: several threads have replies, some threads are resolved, one post has every thread resolved, and one post belongs to another author.
The Activity widget with five posts that have open notes. Each row links to the post, counts the open threads on it, and is dated by the newest note or reply on that post, in the same date, title, count order the sections above it use.
Where those counts come from. The same post in the editor: four open threads, two of them with replies, and the resolved threads below the divider. The dashboard reads "4 open notes" for it.
A post with one open thread uses the singular string.
Every thread resolved on every post, so the Notes section is not rendered at all and the rest of the widget is unchanged.
A thread replied to minutes ago on a six-day-old note sorts to the top, dated by the reply rather than by the note that opened the thread.
Dates across the year: today, earlier this year, and last year, which picks up the year.
Long titles, a post with 24 open notes, and an untitled draft in the same list.
Signed in as an author who can only edit their own post: the posts they cannot edit drop out of the list, notes and all.
At 420px wide, the date and the count hold their columns and the title wraps into what is left.
The whole dashboard for context.
Commit message
Claude drafted this core commit message for review: