fix: skip identity changesets during timeslider playback#7438
Open
JohnMcLear wants to merge 1 commit intoether:developfrom
Open
fix: skip identity changesets during timeslider playback#7438JohnMcLear wants to merge 1 commit intoether:developfrom
JohnMcLear wants to merge 1 commit intoether:developfrom
Conversation
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: ether#5214 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review Summary by QodoSkip identity changesets during timeslider playback
WalkthroughsDescription• 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 Diagramflowchart 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
File Changes1. src/static/js/broadcast.ts
|
Code Review by Qodo
1. Protocol-specific URLs in tests
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| await page.waitForTimeout(1000); | ||
|
|
||
| // Navigate to timeslider | ||
| await page.goto(`http://localhost:9001/p/${padId}/timeslider`); |
There was a problem hiding this comment.
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
Comment on lines
+204
to
+206
| if (broadcasting && !isIdentity(changesetForward)) { | ||
| applyChangeset(changesetForward, revision + 1, false, timeDelta); | ||
| } |
There was a problem hiding this comment.
2. Identity skip stalls revision 🐞 Bug ≡ Correctness
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.
Agent Prompt
### 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix in
broadcast.ts: All threeapplyChangesetcall sites now check for identity changesets using the existingisIdentity()helper fromChangeset.tsand skip them.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 viamutateAttributionLinesandmutateTextLines. These functions can error or corrupt state when given a changeset with no operations.Identity changesets can appear in the revision history when:
compose()of multiple revisions produces a net-zero changeThe existing truthiness check (
if (changeset)) didn't catch identity changesets because they're non-empty strings like"Z:1>0$".Test plan
messages.tstests pass (10/10)Fixes #5214
🤖 Generated with Claude Code