mirror of
https://github.com/ether/etherpad-lite.git
synced 2026-05-04 19:56:37 +02:00
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: https://github.com/ether/etherpad-lite/issues/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>
This commit is contained in:
John McLear
GitHub
parent
8b7155b612
commit
f186ea9d2c
@ -26,7 +26,7 @@
|
||||
const makeCSSManager = require('./cssmanager').makeCSSManager;
|
||||
const domline = require('./domline').domline;
|
||||
import AttribPool from './AttributePool';
|
||||
import {compose, deserializeOps, inverse, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, splitAttributionLines, splitTextLines, unpack} from './Changeset';
|
||||
import {compose, deserializeOps, inverse, isIdentity, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, splitAttributionLines, splitTextLines, unpack} from './Changeset';
|
||||
const attributes = require('./attributes');
|
||||
const linestylefilter = require('./linestylefilter').linestylefilter;
|
||||
const colorutils = require('./colorutils').colorutils;
|
||||
@ -139,52 +139,59 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
|
||||
BroadcastSlider.setSliderPosition(revision);
|
||||
}
|
||||
|
||||
const oldAlines = padContents.alines.slice();
|
||||
try {
|
||||
// must mutate attribution lines before text lines
|
||||
mutateAttributionLines(changeset, padContents.alines, padContents.apool);
|
||||
} catch (e) {
|
||||
debugLog(e);
|
||||
}
|
||||
|
||||
// scroll to the area that is changed before the lines are mutated
|
||||
if ($('#options-followContents').is(':checked') ||
|
||||
$('#options-followContents').prop('checked')) {
|
||||
// get the index of the first line that has mutated attributes
|
||||
// the last line in `oldAlines` should always equal to "|1+1", ie newline without attributes
|
||||
// so it should be safe to assume this line has changed attributes when inserting content at
|
||||
// the bottom of a pad
|
||||
let lineChanged;
|
||||
_.some(oldAlines, (line, index) => {
|
||||
if (line !== padContents.alines[index]) {
|
||||
lineChanged = index;
|
||||
return true; // break
|
||||
}
|
||||
});
|
||||
// some chars are replaced (no attributes change and no length change)
|
||||
// test if there are keep ops at the start of the cs
|
||||
if (lineChanged === undefined) {
|
||||
const [op] = deserializeOps(unpack(changeset).ops);
|
||||
lineChanged = op != null && op.opcode === '=' ? op.lines : 0;
|
||||
// Skip mutation for identity changesets (no actual change), but still advance
|
||||
// revision/time state. Identity changesets can appear when compose() of multiple
|
||||
// revisions produces a net-zero change, or from import/save sequences.
|
||||
// See https://github.com/ether/etherpad-lite/issues/5214
|
||||
if (!isIdentity(changeset)) {
|
||||
const oldAlines = padContents.alines.slice();
|
||||
try {
|
||||
// must mutate attribution lines before text lines
|
||||
mutateAttributionLines(changeset, padContents.alines, padContents.apool);
|
||||
} catch (e) {
|
||||
debugLog(e);
|
||||
}
|
||||
|
||||
const goToLineNumber = (lineNumber) => {
|
||||
// Sets the Y scrolling of the browser to go to this line
|
||||
const line = $('#innerdocbody').find(`div:nth-child(${lineNumber + 1})`);
|
||||
const newY = $(line)[0].offsetTop;
|
||||
const ecb = document.getElementById('editorcontainerbox');
|
||||
// Chrome 55 - 59 bugfix
|
||||
if (ecb.scrollTo) {
|
||||
ecb.scrollTo({top: newY, behavior: 'auto'});
|
||||
} else {
|
||||
$('#editorcontainerbox').scrollTop(newY);
|
||||
// scroll to the area that is changed before the lines are mutated
|
||||
if ($('#options-followContents').is(':checked') ||
|
||||
$('#options-followContents').prop('checked')) {
|
||||
// get the index of the first line that has mutated attributes
|
||||
// the last line in `oldAlines` should always equal to "|1+1", ie newline without attributes
|
||||
// so it should be safe to assume this line has changed attributes when inserting content at
|
||||
// the bottom of a pad
|
||||
let lineChanged;
|
||||
_.some(oldAlines, (line, index) => {
|
||||
if (line !== padContents.alines[index]) {
|
||||
lineChanged = index;
|
||||
return true; // break
|
||||
}
|
||||
});
|
||||
// some chars are replaced (no attributes change and no length change)
|
||||
// test if there are keep ops at the start of the cs
|
||||
if (lineChanged === undefined) {
|
||||
const [op] = deserializeOps(unpack(changeset).ops);
|
||||
lineChanged = op != null && op.opcode === '=' ? op.lines : 0;
|
||||
}
|
||||
};
|
||||
|
||||
goToLineNumber(lineChanged);
|
||||
const goToLineNumber = (lineNumber) => {
|
||||
// Sets the Y scrolling of the browser to go to this line
|
||||
const line = $('#innerdocbody').find(`div:nth-child(${lineNumber + 1})`);
|
||||
const newY = $(line)[0].offsetTop;
|
||||
const ecb = document.getElementById('editorcontainerbox');
|
||||
// Chrome 55 - 59 bugfix
|
||||
if (ecb.scrollTo) {
|
||||
ecb.scrollTo({top: newY, behavior: 'auto'});
|
||||
} else {
|
||||
$('#editorcontainerbox').scrollTop(newY);
|
||||
}
|
||||
};
|
||||
|
||||
goToLineNumber(lineChanged);
|
||||
}
|
||||
|
||||
mutateTextLines(changeset, padContents);
|
||||
}
|
||||
|
||||
mutateTextLines(changeset, padContents);
|
||||
padContents.currentRevision = revision;
|
||||
padContents.currentTime += timeDelta;
|
||||
|
||||
@ -201,7 +208,9 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
|
||||
revisionInfo.addChangeset(
|
||||
revision, revision + 1, changesetForward, changesetBackward, timeDelta);
|
||||
BroadcastSlider.setSliderLength(revisionInfo.latest);
|
||||
if (broadcasting) applyChangeset(changesetForward, revision + 1, false, timeDelta);
|
||||
if (broadcasting) {
|
||||
applyChangeset(changesetForward, revision + 1, false, timeDelta);
|
||||
}
|
||||
};
|
||||
|
||||
/*
|
||||
@ -276,7 +285,9 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
|
||||
changeset = compose(changeset, cs[i], padContents.apool);
|
||||
timeDelta += path.times[i];
|
||||
}
|
||||
if (changeset) applyChangeset(changeset, path.rev, true, timeDelta);
|
||||
if (changeset) {
|
||||
applyChangeset(changeset, path.rev, true, timeDelta);
|
||||
}
|
||||
} else if (path.status === 'partial') {
|
||||
// callback is called after changeset information is pulled from server
|
||||
// this may never get called, if the changeset has already been loaded
|
||||
@ -294,7 +305,9 @@ const loadBroadcastJS = (socket, sendSocketMsg, fireWhenAllScriptsAreLoaded, Bro
|
||||
changeset = compose(changeset, cs[i], padContents.apool);
|
||||
timeDelta += path.times[i];
|
||||
}
|
||||
if (changeset) applyChangeset(changeset, path.rev, true, timeDelta);
|
||||
if (changeset) {
|
||||
applyChangeset(changeset, path.rev, true, timeDelta);
|
||||
}
|
||||
|
||||
// Loading changeset history for new revision
|
||||
loadChangesetsForRevision(newRevision, update);
|
||||
|
||||
@ -0,0 +1,103 @@
|
||||
import {expect, test} from "@playwright/test";
|
||||
import {goToNewPad, getPadBody, clearPadContent, writeToPad} from "../helper/padHelper";
|
||||
|
||||
/**
|
||||
* Regression test for https://github.com/ether/etherpad-lite/issues/5214
|
||||
*
|
||||
* 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 the broadcast code tried to apply it as a real change.
|
||||
*
|
||||
* Identity changesets appear when compose() of multiple revisions produces
|
||||
* a net-zero change (e.g., type "hello" then delete "hello").
|
||||
*/
|
||||
test.describe('Timeslider with identity changesets (bug #5214)', function () {
|
||||
|
||||
test('timeslider playback advances through all revisions including identity changesets', async function ({page}) {
|
||||
// Create a pad with several revisions to exercise the timeslider
|
||||
const padId = await goToNewPad(page);
|
||||
const body = await getPadBody(page);
|
||||
await body.click();
|
||||
await clearPadContent(page);
|
||||
|
||||
// Create multiple revisions by typing, deleting, retyping.
|
||||
// When compose() composes the delete+retype, it can produce an identity changeset.
|
||||
await writeToPad(page, 'First revision text');
|
||||
await page.waitForTimeout(500);
|
||||
|
||||
// Select all and delete (creates a "delete everything" revision)
|
||||
await page.keyboard.down('Control');
|
||||
await page.keyboard.press('A');
|
||||
await page.keyboard.up('Control');
|
||||
await page.keyboard.press('Backspace');
|
||||
await page.waitForTimeout(500);
|
||||
|
||||
// Type new content (combined with delete above, compose can yield identity)
|
||||
await writeToPad(page, 'After delete');
|
||||
await page.waitForTimeout(1000);
|
||||
|
||||
// Navigate to timeslider at revision 0 so playback has revisions to advance through
|
||||
await page.goto(`http://localhost:9001/p/${padId}/timeslider#0`);
|
||||
await page.waitForSelector('#timeslider-slider', {timeout: 10000});
|
||||
await page.waitForTimeout(1000);
|
||||
|
||||
// Click play to start playback from rev 0
|
||||
await page.locator('#playpause_button_icon').click();
|
||||
|
||||
// Wait for playback to progress through revisions
|
||||
await page.waitForTimeout(5000);
|
||||
|
||||
// The page should not have crashed — check for error gritter popups
|
||||
const errors = page.locator('.gritter-item.error');
|
||||
expect(await errors.count()).toBe(0);
|
||||
|
||||
// The timeslider should still be functional
|
||||
await expect(page.locator('#timeslider-slider')).toBeVisible();
|
||||
});
|
||||
|
||||
test('timeslider can scrub through all revisions without error', async function ({page}) {
|
||||
const padId = await goToNewPad(page);
|
||||
const body = await getPadBody(page);
|
||||
await body.click();
|
||||
await clearPadContent(page);
|
||||
|
||||
// Create revisions
|
||||
await writeToPad(page, 'Hello');
|
||||
await page.waitForTimeout(300);
|
||||
await writeToPad(page, ' World');
|
||||
await page.waitForTimeout(300);
|
||||
|
||||
// Select all and delete
|
||||
await page.keyboard.down('Control');
|
||||
await page.keyboard.press('A');
|
||||
await page.keyboard.up('Control');
|
||||
await page.keyboard.press('Backspace');
|
||||
await page.waitForTimeout(300);
|
||||
|
||||
// Retype
|
||||
await writeToPad(page, 'New text');
|
||||
await page.waitForTimeout(1000);
|
||||
|
||||
// Go to timeslider
|
||||
await page.goto(`http://localhost:9001/p/${padId}/timeslider`);
|
||||
await page.waitForSelector('#timeslider-slider', {timeout: 10000});
|
||||
|
||||
// Get the total number of revisions from the slider
|
||||
const sliderLength = await page.evaluate(() => {
|
||||
return (window as any).BroadcastSlider?.getSliderLength?.() ?? 0;
|
||||
});
|
||||
|
||||
// Scrub through each revision from 0 to latest
|
||||
for (let rev = 0; rev <= sliderLength; rev++) {
|
||||
await page.goto(`http://localhost:9001/p/${padId}/timeslider#${rev}`);
|
||||
await page.waitForTimeout(300);
|
||||
|
||||
// Check no errors appeared
|
||||
const errors = page.locator('.gritter-item.error');
|
||||
expect(await errors.count()).toBe(0);
|
||||
}
|
||||
|
||||
// Final check: timeslider is still functional
|
||||
await expect(page.locator('#timeslider-slider')).toBeVisible();
|
||||
});
|
||||
});
|
||||
Loading…
x
Reference in New Issue
Block a user