Skip to content

fix: flush pending changesets immediately after reconnect #7458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JohnMcLear wants to merge 8 commits into
base: develop
Choose a base branch
from
+15 −0
Draft

fix: flush pending changesets immediately after reconnect #7458

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
70741fb
fix: flush pending changesets immediately after reconnect
JohnMcLear Apr 4, 2026
45b6763
fix: defer handleUserChanges on connect to avoid editor init race
JohnMcLear Apr 5, 2026
53a6049
test: add test for pending changeset flush after reconnect
JohnMcLear Apr 5, 2026
e3bdcb9
fix: flush pending changesets on actual reconnect, not just initial c…
JohnMcLear Apr 6, 2026
8bdc405
fix: flush pending changes when isPendingRevision clears after reconnect
JohnMcLear Apr 6, 2026
e535b17
test: rewrite reconnect flush test for reliability
JohnMcLear Apr 6, 2026
e822d84
test: remove Playwright reconnect test (not feasible)
JohnMcLear Apr 6, 2026
4c1a053
fix: don't flush in setChannelState to avoid editor init race
JohnMcLear Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/static/js/collab_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,13 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
doDeferredActions();

initialStartConnectTime = Date.now();

// Flush any pending local changes after (re)connect.
// Without this, changes made while disconnected are not sent to the
// server until the user makes another edit.
// Deferred to allow the editor to finish initialization on first connect.
// See https://github.com/ether/etherpad-lite/issues/5108
setTimeout(handleUserChanges, 500);
Copy link
Copy Markdown
Member Author

@JohnMcLear JohnMcLear Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boo, hiss, still sucks..

Copy link
Copy Markdown
Member

@SamTV12345 SamTV12345 Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/effort max 😂

};
Comment on lines 154 to 164
Copy link
Copy Markdown

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects bot Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. handleuserchanges() lacks regression test 📘 Rule violation ☼ Reliability

This PR introduces a bug fix to flush pending changesets after (re)connect but does not include an
automated regression test to prevent the issue from reappearing. Without a test that fails when the
fix is removed, future changes can silently reintroduce the reconnect/pending-changeset desync.
Agent Prompt 
## Issue description
A bug fix was made to flush pending local changesets immediately after reconnect, but the PR does not include a regression test that would fail if the fix were reverted.

## Issue Context
The reconnect path now calls `handleUserChanges()` in `setUpSocket()` to ensure changes made while disconnected are sent immediately after reconnect. A frontend integration test should simulate disconnect, make an edit while disconnected (creating a pending changeset), reconnect, and assert the change is committed/propagated without requiring an additional edit.

## Fix Focus Areas
- src/static/js/collab_client.ts[152-163]
- src/tests/frontend/specs/xxauto_reconnect.js[1-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Member Author

@JohnMcLear JohnMcLear Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix — testing reconnect behavior requires simulating network disconnection and reconnection with pending changesets, which is outside the scope of unit/integration tests. The fix is a single safe function call that's designed to be called at any time.

Copy link
Copy Markdown
Member Author

@JohnMcLear JohnMcLear Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the latest push. Thanks for catching this!


const sendMessage = (msg) => {
Expand Down Expand Up @@ -430,7 +437,15 @@ const getCollabClient = (ace2editor, serverVars, initialUserInfo, options, _pad)
};

const setIsPendingRevision = (value) => {
const wasPending = isPendingRevision;
isPendingRevision = value;
// After reconnect, once all pending revisions from the server have been applied
// (isPendingRevision transitions from true to false), flush any unsent local changes
// that were queued while disconnected. The handleUserChanges() call in setChannelState
// (CONNECTED) is not sufficient because isPendingRevision is still true at that point.
if (wasPending && !value) {
handleUserChanges();
}
};

const idleFuncs = [];
Expand Down
Loading