Fix: Hidden post-edit tabs still refresh the post-lock presence entry#78
Fix: Hidden post-edit tabs still refresh the post-lock presence entry#78Intenzi wants to merge 2 commits into
Conversation
- Defer heartbeat-send listener registration to run after WP Core's handler
▶ Preview in WordPress PlaygroundBoots a fresh WordPress with this PR's presence-api build, seeds 5 demo users, and drops you on the dashboard. Stress-test variant: 40 demo users · Built from |
|
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. |
josephfusco
left a comment
There was a problem hiding this comment.
Good fix. One suggestion inline.
| const hidden = await captureHeartbeatSend( page ); | ||
| expect( hidden[ 'presence-editor-ping' ] ).toBeUndefined(); | ||
| expect( hidden[ 'wp-refresh-post-lock' ] ).toBeUndefined(); | ||
|
|
There was a problem hiding this comment.
Might be worth adding a positive assertion on the visible side too, so a regression that always deletes the key doesn't slip through:
| expect( visible[ 'wp-refresh-post-lock' ] ).toBeDefined(); | |
There was a problem hiding this comment.
Hi @josephfusco, I'd love your input on the implementation direction here.
I went through with implementing the positive assertion and it has lead to a blocker.
After applying the positive assertion, the test was consistently failing when I ran the e2e tests locally.
Upon digging deeper into how Core handles post locking, I realised that the current negative assertion implementation done by me is also a false positive.
In Core's post.js, wp-refresh-post-lock is only attached if #post-lock-dialog exists in the DOM:
// wp-admin/js/post.js
/**
* Heartbeat locks.
*
* Used to lock editing of an object by only one user at a time.
*
* When the user does not send a heartbeat in a heartbeat-time
* the user is no longer editing and another user can start editing.
*/
$(document).on( 'heartbeat-send.refresh-lock', function( e, data ) {
var lock = $('#active_post_lock').val(),
post_id = $('#post_ID').val(),
send = {};
if ( ! post_id || ! $('#post-lock-dialog').length )
return;
send.post_id = post_id;
if ( lock )
send.lock = lock;
data['wp-refresh-post-lock'] = send;In standard Core PHP (_admin_notice_post_locked()), Core only outputs #post-lock-dialog into the HTML if there is an active lock conflict from another user. The another user check is performed inside wp_check_post_lock :-
...
if ( $time && $time > time() - $time_window && get_current_user_id() !== $user ) {
return $user;
}
return false;
}For a single user editing a post alone, Core never outputs that div, so post.js skips attaching wp-refresh-post-lock altogether.
Because of this, the test was passing the negative assertion simply because Core never attached the key in the first place, not because our visibility handler stripped it.
Proposed Fix:
We can implement a separate test to use a genuine multi-user flow using Playwright's browser.newContext() to simulate two actual users (User A & User B):
User B opens the post first in Context 1 to acquire the lock.
User A opens the post second in Context 2, which triggers Core's multi-user lock dialog naturally.
Then we perform visibility, detached from visibility tests on it.
Alternatively, we could simulate the lock conflict state by updating _edit_lock meta prior to page load if we want a single-browser run.
Which approach would you prefer for the test suite?
There was a problem hiding this comment.
Great catch, you're right that the negative assertion was always true. I'd maybe lean towards the browser.newContext() approach. Have User B navigate to the post edit page first to naturally acquire the lock, then User A opens the same post and the lock dialog is present. Run the visibility assertions from there. That's the most authentic test of the behavior.
Resolves Issue: #21
Related PRs: #19 , #20
How?
wp-refresh-post-lockkey from the shared heartbeatdataobject when the tab/document is backgrounded (document.visibilityState === 'hidden').$(function() { ... })). Because our script runs earlier/faster than WordPress Core, registering our listener resulted in an empty data object when attempting to delete the key. Delaying to document ready ensures that thedata['wp-refresh-post-lock']key exists by the time our handler runs, allowing us to successfully delete it.wp-refresh-post-lockis omitted from the heartbeat payload when the editor tab is hidden.Note
Since we defer the registration specifically to ensure we run after WordPress Core's handler. Keeping all of our code inside this single deferred handler is the simplest approach for now. If we ever need to run other tasks at the very beginning of the heartbeat tick in the future, we can easily split this into two separate listeners (one immediate, one deferred) without any breaking changes.
Use of AI Tools
AI assistance: Yes
Tool(s): Antigravity IDE
Model(s): Gemini 3.5 Flash (Low)
Used for: Identifying relevant code file, going through related PRs, it suggested utilising the deferred execution later when debugging.