fix: skip identity changesets during timeslider playback

[JohnMcLear]

Summary

Fix in broadcast.ts: Identity changesets are now handled inside applyChangeset() — the mutation steps (mutateAttributionLines/mutateTextLines) are skipped, but revision state, timer, slider position, and author UI are still advanced correctly.

Root Cause

When a pad's revision history contains an identity changeset (Z:N>0$, meaning no actual change), the timeslider playback tried to apply it via mutateAttributionLines and mutateTextLines. These functions can error or corrupt state when given a changeset with no operations.

Identity changesets can appear in the revision history when:

• An import replaces all content (creating an empty-then-repopulate sequence)
• A save occurs with no pending changes
• compose() of multiple revisions produces a net-zero change

The existing truthiness check (if (changeset)) didn't catch identity changesets because they're non-empty strings like "Z:1>0$".

Test plan

• Frontend: 2 Playwright tests — playback verifies slider advances through all revisions, scrubbing visits every revision individually (both pass)
• Backend: messages.ts tests pass (10/10)
• Manual: import the .etherpad data from the issue, open timeslider, click play → should play through without errors

Fixes #5214

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Skip identity changesets during timeslider playback

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Skip identity changesets during timeslider playback to prevent crashes
• Add isIdentity() checks at all three applyChangeset call sites
• Import isIdentity helper from Changeset.ts module
• Add two Playwright regression tests for timeslider with identity changesets

Diagram

flowchart LR
  A["Timeslider playback"] --> B["Check if changeset is identity"]
  B -->|Identity| C["Skip applyChangeset"]
  B -->|Real change| D["Apply changeset normally"]
  C --> E["No crash or corruption"]
  D --> E

Loading

File Changes

1. src/static/js/broadcast.ts 🐞 Bug fix +12/-4

Add identity changeset checks to broadcast playback

• Import isIdentity helper function from Changeset module
• Add isIdentity() checks before all three applyChangeset call sites
• Skip applying identity changesets that represent no actual change
• Add explanatory comment referencing issue #5214

src/static/js/broadcast.ts

---

2. src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts 🧪 Tests +93/-0

Add timeslider identity changeset regression tests

• Add regression test for timeslider playback with multiple revisions
• Test delete-and-retype scenario that triggers identity changesets
• Verify no error gritter popups appear during playback
• Add test for scrubbing through all revisions without errors

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

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. Protocol-specific URLs in tests 📘 Rule violation ⚙ Maintainability

Description

New code introduces protocol-specific URLs (for localhost navigation and an issue link), violating
the requirement to use protocol-independent URLs. This can reduce portability across environments
and breaks the documented coding style rule.

Code

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[36]

+    await page.goto(`http://localhost:9001/p/${padId}/timeslider`);

Evidence

PR Compliance ID 8 requires protocol-independent URLs, but the added Playwright spec navigates using
http://localhost:9001/... and the added broadcast comment includes an https:// link.

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[36-36]
src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[78-82]
src/static/js/broadcast.ts[281-283]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Newly added code uses protocol-specific URLs (e.g., `http://...` and `https://...`) where the coding style requires protocol-independent URLs.
## Issue Context
This is explicitly disallowed by the PR compliance checklist (Coding Style requires protocol-independent URLs such as `//example.com`).
## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[36-36]
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[78-82]
- src/static/js/broadcast.ts[281-283]

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

---

2. Identity skip stalls revision ☑ 🐞 Bug ≡ Correctness

Description

broadcast.ts now skips applyChangeset() for identity changesets, but applyChangeset() is also what
advances padContents.currentRevision/currentTime and (when following latest) moves the slider;
skipping it can leave the timeslider desynced/stuck on an older revision. This can break
live-following after an identity revision arrives and can break scrubbing because goToRevision()
continues computing paths from a stale padContents.currentRevision.

Code

src/static/js/broadcast.ts[R204-206]

+    if (broadcasting && !isIdentity(changesetForward)) {
+      applyChangeset(changesetForward, revision + 1, false, timeDelta);
+    }

Evidence

applyChangeset() updates slider position (when not prevented) and is the only place that updates
padContents.currentRevision and padContents.currentTime; the PR’s new !isIdentity(...) guards
prevent applyChangeset() from running, so those state updates do not happen for identity revisions.
goToRevision() computes paths from padContents.currentRevision and is wired to slider movement
callbacks, so leaving currentRevision stale risks wrong path computation and UI desync.

src/static/js/broadcast.ts[135-196]
src/static/js/broadcast.ts[198-207]
src/static/js/broadcast.ts[265-315]
src/static/js/broadcast.ts[465-476]
src/static/js/Changeset.ts[1161-1164]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new identity-changeset guards skip `applyChangeset()`, but that function also advances `padContents.currentRevision/currentTime` and (in live-follow mode) moves the slider. For identity revisions, you still need to advance revision/time (and potentially slider) without calling `mutateAttributionLines()`/`mutateTextLines()`.
### Issue Context
- `applyChangeset()` is responsible for updating `padContents.currentRevision`, `padContents.currentTime`, and (when `preventSliderMovement` is false) `BroadcastSlider.setSliderPosition(revision)`.
- The PR added `!isIdentity(...)` checks at multiple call sites, so identity revisions currently do not advance state.
### Fix Focus Areas
- src/static/js/broadcast.ts[135-196]
- src/static/js/broadcast.ts[198-207]
- src/static/js/broadcast.ts[265-315]
### Implementation direction (one of)
1) Move the identity handling into `applyChangeset()`:
- At the top of `applyChangeset()`, if `isIdentity(changeset)` then:
- If `!preventSliderMovement`, do the slider movement bookkeeping (`goToRevisionIfEnabledCount++` + `BroadcastSlider.setSliderPosition(revision)`).
- Update `padContents.currentRevision = revision` and `padContents.currentTime += timeDelta`.
- Call `updateTimer()` and update authors UI as needed.
- Return early (do **not** call the mutators).
- Then remove (or relax) the `!isIdentity(...)` guards at the call sites.
2) Alternatively, add a small helper like `advanceRevision(revision, timeDelta, preventSliderMovement)` and call it in the `isIdentity` branches at each call site.
Either way, ensure live-following continues across identity revisions and `goToRevision()` ends with `padContents.currentRevision === path.rev` even if the composed changeset is identity.

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

---

3. Scrub test may skip revisions 🐞 Bug ☼ Reliability

Description

The scrub test derives sliderLength from window.BroadcastSlider with a ?? 0 fallback, so if
BroadcastSlider is not actually attached to window, the loop will only visit revision 0 and still
pass. This can leave identity-changeset regressions untested while the test suite reports green.

Code

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[R91-104]

+    // 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);
+    }

Evidence

The test reads window.BroadcastSlider?.getSliderLength?.() and falls back to 0, which makes the
scrub loop a no-op beyond rev 0 if window.BroadcastSlider is undefined. In the timeslider
implementation, BroadcastSlider is assigned to exports.BroadcastSlider (module export), while the
globally accessible revision count is window.revisionInfo.latest, which the test could use
instead.

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[91-104]
src/static/js/timeslider.ts[89-91]
src/static/js/broadcast_revisions.ts[53-59]
src/static/js/broadcast_revisions.ts[113-113]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The scrub test uses `window.BroadcastSlider?.getSliderLength?.() ?? 0`, which can silently evaluate to `0` if BroadcastSlider is not exposed on `window`, causing the loop to only test revision `0`.
### Issue Context
The timeslider code exposes `window.revisionInfo` globally and tracks the latest revision in `window.revisionInfo.latest`. The test should use a global that is guaranteed to exist (or explicitly assert the existence of BroadcastSlider) so the test actually scrubs across all revisions.
### Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[91-104]
- src/static/js/broadcast_revisions.ts[53-59]
- src/static/js/broadcast_revisions.ts[113-113]
### Suggested change
- Replace the `BroadcastSlider` lookup with something like `window.revisionInfo?.latest` (and `await page.waitForFunction(() => window.revisionInfo?.latest > 0)` before reading it).
- Add `expect(sliderLength).toBeGreaterThan(0)` (or a known minimum) so the test fails if revision history did not populate.

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

---

4. Fixed timeouts in tests 🐞 Bug ☼ Reliability

Description

The new Playwright tests use multiple waitForTimeout() calls to create revisions and to wait for
playback progress, which can be flaky under CI load and may not actually wait for the desired state
transition. This can lead to intermittent failures or false positives where playback hasn’t advanced
but the timeout happened to elapse.

Code

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[R23-55]

+    // 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
+    await page.goto(`http://localhost:9001/p/${padId}/timeslider`);
+    await page.waitForSelector('#timeslider-slider', {timeout: 10000});
+
+    // Record the initial slider position
+    const sliderBefore = await page.locator('#ui-slider-handle').getAttribute('style');
+
+    // Click play to start playback
+    await page.locator('#playpause_button_icon').click();
+
+    // Wait for playback to progress through revisions
+    await page.waitForTimeout(3000);
+
+    // The slider should have advanced from its initial position
+    const sliderAfter = await page.locator('#ui-slider-handle').getAttribute('style');
+    expect(sliderAfter).not.toBe(sliderBefore);
+

Evidence

The tests rely on hardcoded sleeps (e.g., 300ms/500ms/3000ms) rather than waiting for a
deterministic signal (slider position, revision label, or revisionInfo.latest). These time-based
waits are sensitive to runtime variability and can cause flaky results.

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[23-55]
src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[70-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The tests use `page.waitForTimeout()` to (1) create distinct revisions and (2) assert playback progress. These waits are nondeterministic and can be flaky or provide false confidence.
### Issue Context
Timeslider exposes DOM state (e.g., `#revision_label`, `#ui-slider-handle`) and globals (e.g., `window.revisionInfo.latest`) that can be polled to deterministically confirm progress.
### Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[23-55]
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[70-100]
### Suggested change
- Replace sleeps after typing/deleting with a wait for a revision increment (for example: capture current `window.revisionInfo.latest` after entering timeslider, then wait until it increases).
- Replace the 3s playback sleep with `await expect.poll(...)` or `page.waitForFunction(...)` that waits until the slider handle position (or revision label) changes from the initial value.

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

---

5. Unused identity import ☑ 🐞 Bug ⚙ Maintainability

Description

broadcast.ts imports identity from Changeset.ts but never uses it, which is likely to be flagged
by linting and fail the repo’s eslint-based lint script. This also increases confusion because the
fix only uses isIdentity().

Code

src/static/js/broadcast.ts[29]

+import {compose, deserializeOps, identity, inverse, isIdentity, moveOpsToNewPool, mutateAttributionLines, mutateTextLines, splitAttributionLines, splitTextLines, unpack} from './Changeset';

Evidence

The identity symbol is imported in broadcast.ts but is not referenced elsewhere in the file, and
the project lint script runs eslint ., which commonly reports unused imports.

src/static/js/broadcast.ts[26-33]
src/package.json[134-136]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`identity` is imported from `./Changeset` in `broadcast.ts` but never used.
### Issue Context
The project runs `eslint .` via the `lint` script, and unused imports are typically reported.
### Fix Focus Areas
- src/static/js/broadcast.ts[26-33]
### Suggested fix
Remove `identity` from the import list unless you end up using it as part of the identity-changeset handling refactor.

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

---

View more (2)

6. Identity case not exercised ☑ 🐞 Bug ⚙ Maintainability

Description

The new regression spec never verifies that an identity changeset scenario is actually reached (nor
that playback advanced), so it can pass without executing the isIdentity() branch that this PR
adds. This reduces the test’s ability to prevent regressions of the identity-changeset crash path.

Code

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[R13-52]

+  test('timeslider playback works when pad has many revisions', 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
+    await writeToPad(page, 'First revision text');
+    await page.waitForTimeout(500);
+
+    // Select all and delete (creates a "delete everything" revision similar to the bug)
+    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
+    await writeToPad(page, 'After delete');
+    await page.waitForTimeout(1000);
+
+    // Navigate to timeslider
+    await page.goto(`http://localhost:9001/p/${padId}/timeslider`);
+    await page.waitForSelector('#timeslider-slider', {timeout: 10000});
+
+    // Click play to start playback
+    await page.locator('#playpause_button_icon').click();
+
+    // Wait for playback to progress
+    await page.waitForTimeout(3000);
+
+    // The page should not have crashed — check for error gritter popups
+    const errors = page.locator('.gritter-item.error');
+    const errorCount = await errors.count();
+    expect(errorCount).toBe(0);
+
+    // The timeslider should still be functional
+    await expect(page.locator('#timeslider-slider')).toBeVisible();
+  });

Evidence

The production fix is specifically gated on isIdentity(changeset) and only triggers when
unpack(cs).ops === '' and oldLen === newLen. The new test only asserts there are no visible
gritter errors after a fixed wait and does not assert (a) that playback progressed (e.g., slider
handle moved / timer changed), or (b) that an identity changeset scenario exists in the history
being played/scrubbed.

src/static/js/broadcast.ts[135-203]
src/static/js/Changeset.ts[1155-1164]
src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[13-52]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new Playwright test does not prove that the identity-changeset handling added to timeslider playback is actually exercised. It only checks for absence of error popups after a fixed delay.
## Issue Context
The fix in the PR is gated on `isIdentity(changeset)` (identity is `unpack(cs).ops === '' && oldLen === newLen`). A regression test should ensure it hits that condition and that playback/scrubbing actually advances.
## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[13-52]
- src/static/js/broadcast.ts[135-203]
## Suggested change
- Adjust the revision setup to reliably create a net-zero span (e.g., rev0 empty → rev1 type "X" → rev2 delete-all back to empty), then *jump* across that span in timeslider (a jump forces composition and is where identity changesets can appear).
- Add an assertion that playback/scrubbing progressed (examples: `#ui-slider-handle` style/position changes, or `#timer` text changes between two waits).
- Keep the existing “no gritter errors” assertion as a secondary signal.

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

---

7. Scrub coverage incomplete ☑ 🐞 Bug ⚙ Maintainability

Description

The “scrub through all revisions” test only navigates to #0 once and then checks slider
visibility, so it does not actually scrub across revisions and won’t catch failures that occur when
moving through multiple revisions. This leaves the intended scrubbing regression scenario largely
untested.

Code

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[R54-92]

+  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});
+
+    // Scrub to revision 0
+    await page.goto(`http://localhost:9001/p/${padId}/timeslider#0`);
+    await page.waitForTimeout(1000);
+
+    // No errors should be visible
+    const errors = page.locator('.gritter-item.error');
+    expect(await errors.count()).toBe(0);
+
+    // Scrub forward to the latest revision
+    const slider = page.locator('#timeslider-slider');
+    await expect(slider).toBeVisible();
+  });

Evidence

The test performs a single navigation to the timeslider, then a single navigation to
.../timeslider#0, and stops after checking that no error popup is visible and the slider exists.
It never iterates across revision numbers or performs slider movements that would stress the
timeslider path composition/mutation logic across multiple steps.

src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[54-92]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The current scrub test does not scrub across revisions; it only jumps once to `#0` and asserts the slider is visible.
## Issue Context
The bug being fixed is in the timeslider changeset application path during playback/scrubbing. To validate scrubbing, the test should move across multiple revisions (preferably including a span that can compose to an identity changeset) and assert no errors.
## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[54-92]
## Suggested change
- Determine the latest revision number (for example via a page.evaluate reading the slider max if available, or by tracking how many edits you made).
- Loop through multiple revision targets (e.g., 0 → latest in steps, or every revision) using hash navigation or simulated slider dragging.
- After each move, wait for a reliable signal of revision application (e.g., `#timer` change) and assert no `.gritter-item.error` is present.

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

---

8. Misleading identity-skip comment ☑ 🐞 Bug ⚙ Maintainability

Description

In goToRevision(), the comment says identity changesets are skipped, but the code still calls
applyChangeset() for any truthy changeset string (identity changesets are truthy). This comment/code
mismatch is likely to confuse future maintainers and risks an incorrect refactor that reintroduces
timeslider desync bugs.

Code

src/static/js/broadcast.ts[R288-292]

+      // Skip identity changesets — they represent no actual change and can cause
+      // errors during timeslider playback. See https://github.com/ether/etherpad-lite/issues/5214
+      if (changeset) {
+        applyChangeset(changeset, path.rev, true, timeDelta);
+      }

Evidence

The comment claims identity changesets are skipped at the call site, but the guard is only `if
(changeset), while identity handling is actually implemented inside applyChangeset() via if
(!isIdentity(changeset)) { ...mutations... }`.

src/static/js/broadcast.ts[142-193]
src/static/js/broadcast.ts[288-292]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A comment in `goToRevision()` says identity changesets are skipped, but the code still calls `applyChangeset()` for identity changesets (they are truthy strings). This mismatch can mislead future refactors.
## Issue Context
Identity changesets are correctly handled inside `applyChangeset()` (mutations are skipped, but revision/time state still advances). The call site should reflect that.
## Fix Focus Areas
- src/static/js/broadcast.ts[288-292]

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

---

ⓘ The new review experience is currently in Beta. Learn more

[qodo-free-for-open-source-projects]

1. Protocol-specific urls in tests 📘 Rule violation ⚙ Maintainability

New code introduces protocol-specific URLs (for localhost navigation and an issue link), violating
the requirement to use protocol-independent URLs. This can reduce portability across environments
and breaks the documented coding style rule.

Agent Prompt

## Issue description
Newly added code uses protocol-specific URLs (e.g., `http://...` and `https://...`) where the coding style requires protocol-independent URLs.

## Issue Context
This is explicitly disallowed by the PR compliance checklist (Coding Style requires protocol-independent URLs such as `//example.com`).

## Fix Focus Areas
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[36-36]
- src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts[78-82]
- src/static/js/broadcast.ts[281-283]

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

[SamTV12345]

I see so we just skip identical changesets. How can they be created one after the other directly?

[JohnMcLear]

@SamTV12345 Good question! Identity changesets (net-zero changes) can appear in a few ways:

1. compose() of multiple revisions — this is the most common case in the timeslider. When goToRevision() composes a sequence of changesets to jump between revisions, the composed result can be an identity if the revisions cancel each other out (e.g., user types "hello" then deletes "hello").

2. Import sequences — when importing replaces all content, it can create an empty-then-repopulate sequence where composing produces identity.

3. Save with no pending changes — can create a revision with no actual ops.

The original code had if (changeset) which only caught null/undefined/empty-string, but identity changesets are non-empty strings like "Z:1>0$" so they passed through and crashed the mutators.

Also addressed the Qodo review feedback: moved the isIdentity() check inside applyChangeset() so that identity changesets still advance currentRevision/currentTime slider position — just skipping the mutation step. And removed the unused identity import.

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Skip identity changesets during timeslider playback

🐞 Bug fix

Walkthroughs

Description

• Move identity changeset detection inside applyChangeset() to skip mutations while advancing
  state
• Identity changesets now properly advance revision, time, slider position, and author UI
• Add regression tests for timeslider playback with delete-retype revision sequences
• Prevent timeslider from getting stuck on stale revisions when encountering identity changesets

Diagram

flowchart LR
  A["Identity Changeset<br/>Z:N>0$"] --> B["applyChangeset()"]
  B --> C{"isIdentity<br/>check"}
  C -->|True| D["Skip mutations<br/>mutateAttributionLines<br/>mutateTextLines"]
  C -->|False| E["Apply mutations"]
  D --> F["Advance state<br/>revision/time/slider"]
  E --> F
  F --> G["Timeslider continues<br/>without errors"]

Loading

File Changes

1. src/static/js/broadcast.ts 🐞 Bug fix +59/-44

Move identity changeset check into applyChangeset function

• Import isIdentity helper from Changeset module
• Wrap mutation logic in applyChangeset() with isIdentity() guard to skip mutations for identity
 changesets
• Identity changesets still advance currentRevision, currentTime, slider position, and author UI
• Add explanatory comments about identity changesets and issue reference
• Improve code formatting with consistent braces in conditional blocks

src/static/js/broadcast.ts

---

2. src/tests/frontend-new/specs/timeslider_identity_changeset.spec.ts 🧪 Tests +93/-0

Add regression tests for timeslider identity changeset handling

• Add regression test for timeslider playback with multiple revisions including delete-retype
 sequences
• Test verifies playback completes without errors or error popups
• Add test for scrubbing through all revisions without encountering errors
• Tests exercise the identity changeset scenario from issue #5214

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

---

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit d01c946