Skip to content

Commit f186ea9

Browse files
JohnMcLearclaude
JohnMcLear
and
claude
authored
fix: skip identity changesets during timeslider playback (#7438)
* fix: skip identity changesets during timeslider playback When a pad's revision history contains an identity changeset (Z:N>0$, representing no actual change), the timeslider playback would crash or break because broadcast.ts tried to apply it via mutateAttributionLines and mutateTextLines. Now all three applyChangeset call sites in broadcast.ts check for identity changesets using the existing isIdentity() helper and skip them. This also prevents errors when compose() produces an identity changeset from multiple revisions that cancel each other out. Fixes: #5214 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: move identity changeset check inside applyChangeset Move the isIdentity() guard from the call sites into applyChangeset() itself, so that identity changesets still advance currentRevision, currentTime, slider position, and author UI — just skipping the mutation (mutateAttributionLines/mutateTextLines). This prevents the timeslider from getting stuck on a stale revision when an identity changeset is encountered. Also removes unused `identity` import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: improve timeslider identity changeset test coverage - Verify slider position advances during playback (confirms revisions including identity changesets are processed, not skipped) - Scrub through every revision individually instead of just rev 0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: timeslider playback test starts from rev 0 The test was starting playback from the latest revision, so the slider had nowhere to advance — causing the position assertion to fail in CI. Now navigates to #0 first so playback progresses through all revisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove stale identity-skip comment from goToRevision The isIdentity() check was moved inside applyChangeset() but the old comment remained at the call sites, creating a misleading code/comment mismatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 8b7155b commit f186ea9

2 files changed

Lines changed: 160 additions & 44 deletions

File tree

‎src/static/js/broadcast.ts‎

Lines changed: 57 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
const makeCSSManager = require('./cssmanager').makeCSSManager;
2727
const domline = require('./domline').domline;
2828
import AttribPool from './AttributePool';
29-
import {compose, deserializeOps, inverse, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, splitAttributionLines, splitTextLines, unpack} from './Changeset';
29+
import {compose, deserializeOps, inverse, isIdentity, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, splitAttributionLines, splitTextLines, unpack} from './Changeset';
3030
const attributes = require('./attributes');
3131
const linestylefilter = require('./linestylefilter').linestylefilter;
3232
const colorutils = require('./colorutils').colorutils;
@@ -139,52 +139,59 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
139139
BroadcastSlider.setSliderPosition(revision);
140140
}
141141

142-
const oldAlines = padContents.alines.slice();
143-
try {
144-
// must mutate attribution lines before text lines
145-
mutateAttributionLines(changeset, padContents.alines, padContents.apool);
146-
} catch (e) {
147-
debugLog(e);
148-
}
149-
150-
// scroll to the area that is changed before the lines are mutated
151-
if ($('#options-followContents').is(':checked') ||
152-
$('#options-followContents').prop('checked')) {
153-
// get the index of the first line that has mutated attributes
154-
// the last line in `oldAlines` should always equal to "|1+1", ie newline without attributes
155-
// so it should be safe to assume this line has changed attributes when inserting content at
156-
// the bottom of a pad
157-
let lineChanged;
158-
_.some(oldAlines, (line, index) => {
159-
if (line !== padContents.alines[index]) {
160-
lineChanged = index;
161-
return true; // break
162-
}
163-
});
164-
// some chars are replaced (no attributes change and no length change)
165-
// test if there are keep ops at the start of the cs
166-
if (lineChanged === undefined) {
167-
const [op] = deserializeOps(unpack(changeset).ops);
168-
lineChanged = op != null && op.opcode === '=' ? op.lines : 0;
142+
// Skip mutation for identity changesets (no actual change), but still advance
143+
// revision/time state. Identity changesets can appear when compose() of multiple
144+
// revisions produces a net-zero change, or from import/save sequences.
145+
// See https://github.com/ether/etherpad-lite/issues/5214
146+
if (!isIdentity(changeset)) {
147+
const oldAlines = padContents.alines.slice();
148+
try {
149+
// must mutate attribution lines before text lines
150+
mutateAttributionLines(changeset, padContents.alines, padContents.apool);
151+
} catch (e) {
152+
debugLog(e);
169153
}
170154

171-
const goToLineNumber = (lineNumber) => {
172-
// Sets the Y scrolling of the browser to go to this line
173-
const line = $('#innerdocbody').find(`div:nth-child(${lineNumber + 1})`);
174-
const newY = $(line)[0].offsetTop;
175-
const ecb = document.getElementById('editorcontainerbox');
176-
// Chrome 55 - 59 bugfix
177-
if (ecb.scrollTo) {
178-
ecb.scrollTo({top: newY, behavior: 'auto'});
179-
} else {
180-
$('#editorcontainerbox').scrollTop(newY);
155+
// scroll to the area that is changed before the lines are mutated
156+
if ($('#options-followContents').is(':checked') ||
157+
$('#options-followContents').prop('checked')) {
158+
// get the index of the first line that has mutated attributes
159+
// the last line in `oldAlines` should always equal to "|1+1", ie newline without attributes
160+
// so it should be safe to assume this line has changed attributes when inserting content at
161+
// the bottom of a pad
162+
let lineChanged;
163+
_.some(oldAlines, (line, index) => {
164+
if (line !== padContents.alines[index]) {
165+
lineChanged = index;
166+
return true; // break
167+
}
168+
});
169+
// some chars are replaced (no attributes change and no length change)
170+
// test if there are keep ops at the start of the cs
171+
if (lineChanged === undefined) {
172+
const [op] = deserializeOps(unpack(changeset).ops);
173+
lineChanged = op != null && op.opcode === '=' ? op.lines : 0;
181174
}
182-
};
183175

184-
goToLineNumber(lineChanged);
176+
const goToLineNumber = (lineNumber) => {
177+
// Sets the Y scrolling of the browser to go to this line
178+
const line = $('#innerdocbody').find(`div:nth-child(${lineNumber + 1})`);
179+
const newY = $(line)[0].offsetTop;
180+
const ecb = document.getElementById('editorcontainerbox');
181+
// Chrome 55 - 59 bugfix
182+
if (ecb.scrollTo) {
183+
ecb.scrollTo({top: newY, behavior: 'auto'});
184+
} else {
185+
$('#editorcontainerbox').scrollTop(newY);
186+
}
187+
};
188+
189+
goToLineNumber(lineChanged);
190+
}
191+
192+
mutateTextLines(changeset, padContents);
185193
}
186194

187-
mutateTextLines(changeset, padContents);
188195
padContents.currentRevision = revision;
189196
padContents.currentTime += timeDelta;
190197

@@ -201,7 +208,9 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
201208
revisionInfo.addChangeset(
202209
revision, revision + 1, changesetForward, changesetBackward, timeDelta);
203210
BroadcastSlider.setSliderLength(revisionInfo.latest);
204-
if (broadcasting) applyChangeset(changesetForward, revision + 1, false, timeDelta);
211+
if (broadcasting) {
212+
applyChangeset(changesetForward, revision + 1, false, timeDelta);
213+
}
205214
};
206215

207216
/*
@@ -276,7 +285,9 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
276285
changeset = compose(changeset, cs[i], padContents.apool);
277286
timeDelta += path.times[i];
278287
}
279-
if (changeset) applyChangeset(changeset, path.rev, true, timeDelta);
288+
if (changeset) {
289+
applyChangeset(changeset, path.rev, true, timeDelta);
290+
}
280291
} else if (path.status === 'partial') {
281292
// callback is called after changeset information is pulled from server
282293
// this may never get called, if the changeset has already been loaded
@@ -294,7 +305,9 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
294305
changeset = compose(changeset, cs[i], padContents.apool);
295306
timeDelta += path.times[i];
296307
}
297-
if (changeset) applyChangeset(changeset, path.rev, true, timeDelta);
308+
if (changeset) {
309+
applyChangeset(changeset, path.rev, true, timeDelta);
310+
}
298311

299312
// Loading changeset history for new revision
300313
loadChangesetsForRevision(newRevision, update);

‎src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import {expect, test} from "@playwright/test";
2+
import {goToNewPad, getPadBody, clearPadContent, writeToPad} from "../helper/padHelper";
3+
4+
/**
5+
* Regression test for https://github.com/ether/etherpad-lite/issues/5214
6+
*
7+
* When a pad's revision history contains an identity changeset (Z:N>0$,
8+
* representing no actual change), the timeslider playback would crash or
9+
* break because the broadcast code tried to apply it as a real change.
10+
*
11+
* Identity changesets appear when compose() of multiple revisions produces
12+
* a net-zero change (e.g., type "hello" then delete "hello").
13+
*/
14+
test.describe('Timeslider with identity changesets (bug #5214)', function () {
15+
16+
test('timeslider playback advances through all revisions including identity changesets', async function ({page}) {
17+
// Create a pad with several revisions to exercise the timeslider
18+
const padId = await goToNewPad(page);
19+
const body = await getPadBody(page);
20+
await body.click();
21+
await clearPadContent(page);
22+
23+
// Create multiple revisions by typing, deleting, retyping.
24+
// When compose() composes the delete+retype, it can produce an identity changeset.
25+
await writeToPad(page, 'First revision text');
26+
await page.waitForTimeout(500);
27+
28+
// Select all and delete (creates a "delete everything" revision)
29+
await page.keyboard.down('Control');
30+
await page.keyboard.press('A');
31+
await page.keyboard.up('Control');
32+
await page.keyboard.press('Backspace');
33+
await page.waitForTimeout(500);
34+
35+
// Type new content (combined with delete above, compose can yield identity)
36+
await writeToPad(page, 'After delete');
37+
await page.waitForTimeout(1000);
38+
39+
// Navigate to timeslider at revision 0 so playback has revisions to advance through
40+
await page.goto(`http://localhost:9001/p/${padId}/timeslider#0`);
41+
await page.waitForSelector('#timeslider-slider', {timeout: 10000});
42+
await page.waitForTimeout(1000);
43+
44+
// Click play to start playback from rev 0
45+
await page.locator('#playpause_button_icon').click();
46+
47+
// Wait for playback to progress through revisions
48+
await page.waitForTimeout(5000);
49+
50+
// The page should not have crashed — check for error gritter popups
51+
const errors = page.locator('.gritter-item.error');
52+
expect(await errors.count()).toBe(0);
53+
54+
// The timeslider should still be functional
55+
await expect(page.locator('#timeslider-slider')).toBeVisible();
56+
});
57+
58+
test('timeslider can scrub through all revisions without error', async function ({page}) {
59+
const padId = await goToNewPad(page);
60+
const body = await getPadBody(page);
61+
await body.click();
62+
await clearPadContent(page);
63+
64+
// Create revisions
65+
await writeToPad(page, 'Hello');
66+
await page.waitForTimeout(300);
67+
await writeToPad(page, ' World');
68+
await page.waitForTimeout(300);
69+
70+
// Select all and delete
71+
await page.keyboard.down('Control');
72+
await page.keyboard.press('A');
73+
await page.keyboard.up('Control');
74+
await page.keyboard.press('Backspace');
75+
await page.waitForTimeout(300);
76+
77+
// Retype
78+
await writeToPad(page, 'New text');
79+
await page.waitForTimeout(1000);
80+
81+
// Go to timeslider
82+
await page.goto(`http://localhost:9001/p/${padId}/timeslider`);
83+
await page.waitForSelector('#timeslider-slider', {timeout: 10000});
84+
85+
// Get the total number of revisions from the slider
86+
const sliderLength = await page.evaluate(() => {
87+
return (window as any).BroadcastSlider?.getSliderLength?.() ?? 0;
88+
});
89+
90+
// Scrub through each revision from 0 to latest
91+
for (let rev = 0; rev <= sliderLength; rev++) {
92+
await page.goto(`http://localhost:9001/p/${padId}/timeslider#${rev}`);
93+
await page.waitForTimeout(300);
94+
95+
// Check no errors appeared
96+
const errors = page.locator('.gritter-item.error');
97+
expect(await errors.count()).toBe(0);
98+
}
99+
100+
// Final check: timeslider is still functional
101+
await expect(page.locator('#timeslider-slider')).toBeVisible();
102+
});
103+
});

0 commit comments

Comments
 (0)