Repository navigation
fix: count Who's Online overflow from the heartbeat total - #406
josephfusco merged 2 commits into
Conversation
|
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. |
🛝 WordPress PlaygroundsBuilt from 11251ad
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #406 +/- ##
============================================
- Coverage 96.85% 96.81% -0.04%
- Complexity 239 240 +1
============================================
Files 19 19
Lines 2826 2828 +2
============================================
+ Hits 2737 2738 +1
- Misses 89 90 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
10008a7 to
46d270b
Compare
josephfusco
left a comment
josephfusco
left a comment
There was a problem hiding this comment.
Fix itself is right, but the e2e failure is this spec's own: tick() only counts ticks that carry presence-online, so it times out whenever the page-load tick wins its leader-election race and the forced tick comes back presence-online-unchanged (moving the seeder wpCli call inside the try, after visitAdminPage, makes the next tick a state change either way), and separately test_heartbeat_payload_holds_every_other_user_at_the_expandable_maximum still passes with array_slice( $entries, ... ) reverted because add_user_to_room( 'dashboard', 0 ) ties every timestamp so the viewer sorts last, which $i + 1 fixes.
The tick response caps at VISIBLE_ROWS + OVERFLOW_THRESHOLD, so the capped array can never exceed the threshold and summary mode was unreachable after the first tick. Branch and count on presence-online-total instead. presence-online-total counted the viewer while every display path filters them out, so heartbeat_received() now filters through the same helper as render(). That also fixes the cap boundary, where at exactly 23 other users the client received 22 and the expanded list silently dropped one.
The e2e spec seeded the room before the page load, so whenever the load tick carried the final state the forced tick came back presence-online-unchanged and the listener never fired. Seed after the load instead, which makes the next tick a state change either way. The payload guard seeded every user at the same date_gmt, which wp_get_presence() orders by, so where the viewer landed in the tie decided whether the guard caught anything. Stagger the ages so the viewer is newest and sorts first by construction.
46d270b to
11251ad
Compare
josephfusco
left a comment
josephfusco
left a comment
There was a problem hiding this comment.
Thanks for the adjustments. Looks good!
The load render is correct and the first tick regresses it.
render()branches on the full overflow list, so 50 people online loads as+47 more — view all users. The tick response caps atVISIBLE_ROWS + OVERFLOW_THRESHOLD, the client splits 3 visible rows off that capped array, andoverflow.length > overflowThresholdcan never be true afterwards, so the link is replaced by a+19 moreexpandable list and 28 people become unreachable.Both the branch and the count now come from
presence-online-total, which was already in the response and unread.The decision this needed that the issue does not cover.
presence-online-totalcounted the viewer, whilerender()and the widget JS both filter the viewer out, so the total was one too high and could not be used as it stood. Rather than subtracting one on the client,heartbeat_received()now filters the viewer the same wayrender()does, through a sharedwithout_current_user(). Two consequences worth your review:presence-onlineno longer carries the viewer's own entry. Three existing tests asserted that it did, including one where the viewer was the only person in the room. They now assert the same payload shape against a second user.The client signature now leads with the total. Without that, a user leaving from below the cap moves the count while the capped entries stay identical, and the widget keeps showing the old number.
One correction to the issue body: the count saturates at 19, not 20, for the same off by one reason.
Verified on a local site with 50 users online. Before: server render
+47 more — view all users, first tick payloadentries: 23, total: 51, widget then shows+19 morewith the link gone. After: same server render, payloadtotal: 50, widget still shows+47 more — view all users.PHPUnit goes 401 to 403, both green. The new e2e spec covers the tick, and its second phase covers the stale count. Each new guard was mutation checked.
Fixes #360
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Implementation, the PHPUnit and e2e tests, and the local reproduction rig. I reviewed the change and checked the before and after on a local site before opening this.