Repository navigation
fix(studio): a preview reload writes the composition as it is now, not as it was when it asked - #6998
Merged
Merged
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Approve at d8ab94c8. No blockers.
Re-pin: I was asked to review eac0946e, and the head moved during my review. eac0946e..d8ab94c8 only renames SWAP_REFETCHES to SWAP_REFETCHES_BEFORE_THE_COPY_IN_HAND_WINS and drops two comments. Behavior is unchanged. At this head, useTimelinePlayer.shadowReload.test.ts and studioFileVersion.test.ts pass (64 tests) and tsc --noEmit is clean.
- I checked the
fails without the fix:claim locally. Withmain'ssceneSwap.ts,useTimelinePlayer.ts,useShadowPreviewReload.tsandstudioFileVersion.ts, "swaps the composition as it is now when a write lands while the HTML is loading" fails (expected "vi.fn()" to not be called with arguments). The shadow test and the refetch-bound test also fail there. Ateac0946e, all ofsrc/playerplusstudioFileVersion.test.tspass (197 files, 2481 tests). - Freshness checks:
sceneSwapForchecks the gate afterresponse.text(), andswapFreshly's.thenre-checks after the swap lands. So a write during either the fetch or the in-frame swap triggers a refetch. That is bounded at 5, after which the old behavior takes over. A superseded swap still bails early through!isCurrent(). - Revision counting: every Studio write path claims a token (
studioWriteHeadersormarkStudioWriteToken), including the undo path inusePersistentEditHistory. Not counting the echo is the right call. - The body is honest that #6989 stays open, with only a ref and no
Fixes.
Non-blocking:
- Unlike the swap, the shadow path's restart in
heldOrStalehas no refetch bound. Under writes that never stop, it keeps reloading instead of promoting. Each of those writes already starts its own reload, so this is not a new starvation mode. A bound like the swap's would make the two paths symmetric. - The "What I measured" table has a leftover placeholder: the unslowed 40-run row reads
PLAIN40in the "with the fix" column. Please fill it in or remove it.
Contributor
Edit accuracy: accurate 271 (base branch 271), smooth 115 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (3)
|
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
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.
What
A preview reload no longer writes a copy of the composition the project has already moved past, so
the undos that land while it is loading are not thrown away.
Refs #6989. It does not close that issue: see "What I did NOT exercise".
Why
A burst of undos leaves the preview showing an older version of the file than the one on disk.
One step of the keyframing sequence cannot be expressed as a patch — it takes the keyframes away,
which removes a script element — so it replaces the preview instead. Replacing it means fetching the
preview HTML and writing it over the live document, either as a scene swap or, when the film changed
outside its scenes, as a shadow reload. That fetch is in flight for a few hundred ms and the undos
that follow land inside it: each patches the live document and reports itself done, then the fetched
copy arrives and overwrites all of them with the project as it was several undos ago.
The reload only ever checked that no newer reload had superseded it. Nothing asked whether the
project itself had moved since it requested the file.
How
studioFileRevision()inutils/studioFileVersion.ts— the module that already owns writeidentity, and the one place every Studio write passes through — counts one step per write claimed.
A copy of the project read at a lower revision is stale.
moved; it fetches again, bounded at five, after which the newest copy in hand wins exactly as
before.
heldOrStalealready restarted a shadow that a gesture save had outrun; itnow checks the revision too, which is the path this sequence actually takes.
Patches keep applying in place, so the undo path itself is unchanged and no extra reload is added to
the ordinary edit: only the choice of which fetched copy a replacement is allowed to write.
The revision deliberately does not move again on the server's echo of a write it already counted.
The echo of the write that caused a reload always arrives while the shadow is still loading, so
counting it would make every single edit reload twice.
Test plan
src/player/hooks/useTimelinePlayer.shadowReload.test.ts,src/utils/studioFileVersion.test.ts,and the whole
packages/studiosuite.HF_SLOW_HISTORYintests/e2e/edit-accuracy/case.mjsis new: it holds each history reply back sothe next undo is always pressed while the previous one's preview update is still running. That
turns this race from about one run in three into every run, and is the deterministic regression for
it.
fails without the fix:
useTimelinePlayer scene swap > swaps the composition as it is now when a write lands while the HTML is loadinginpackages/studio/src/player/hooks/useTimelinePlayer.shadowReload.test.tsWhat I measured
undo.box284.41useTimelinePlayer.shadowReload.test.ts+studioFileVersion.test.tspackages/studiosuitetsc --noEmitoxfmt --check,oxlint,fallow audit --fail-on-issues,comment-ratchetNeither test is vacuous: forcing the freshness check true fails two of them, and removing the
refetch bound fails the one that pins it.
What I did NOT exercise
With the fix in, the deterministic lever stops producing the 284 px state and produces the issue's
exact 240.4176 px instead. A probe in the restore path shows why: the last undo lands on a freshly
reloaded document whose fetch was served before the last writes reached disk, and the restore then
writes that same stale state onto it. Closing that needs the refetch to wait until no write is
outstanding rather than refetch at once — the write-token map already models claimed-but-not-echoed
— and that is a third mechanism in this race, left for its own change.
run, which is what makes the remaining work tractable.
Before
The box after a burst of undos, with the history reply held back. The preview is showing the file as it was several undos earlier.
After
The same run with the fix. The preview is no longer a stale fetch; it is still displaced, by the second defect described above, which this change does not claim to fix.