Skip to content

fix(studio): restore saved positions on page refresh#801

Merged
miguel-heygen merged 2 commits into
mainfrom
worktree-fix+studio-position-persistence
May 13, 2026
Merged

fix(studio): restore saved positions on page refresh#801
miguel-heygen merged 2 commits into
mainfrom
worktree-fix+studio-position-persistence

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

Summary

  • studio-manual-edits.json is correctly persisted to disk when moving/resizing/rotating layers, but the in-memory manifest was never seeded from disk on page load
  • On every page refresh studioManualEditManifestRef started empty, so handleLoad applied an empty manifest and all saved positions were discarded
  • One-line fix: applyStudioManualEditsToPreview now reads from disk when the in-memory manifest is empty (auto-bootstrap); once loaded, subsequent calls skip the disk read

Test plan

  • Move a layer in the studio, note its new position
  • Refresh the page — position should be preserved
  • Make a text edit after moving — position should still be preserved after the preview reloads
  • Open a different project and switch back — positions should reload correctly

studio-manual-edits.json was correctly persisted to disk but never read back
into memory on bootstrap. On every page refresh, studioManualEditManifestRef
started empty, so handleLoad applied an empty manifest and all saved
positions/sizes/rotations were silently discarded.

applyStudioManualEditsToPreview now reads from disk whenever the in-memory
manifest is empty. The existing readRevision guard prevents overwriting an
in-flight optimistic edit if a position change races with the disk read.
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE at 185e68a8. Real data-loss bug on a just-shipped alpha feature; the fix is minimal, surgical, and correctly diagnosed. One edge-case worth verifying about disk-write ordering.

Root cause is right

The fix-shape matches the bug: studioManualEditManifestRef starts empty on every page load, but applyStudioManualEditsToPreview only consulted disk when an explicit flag was passed. So handleLoad fired with an empty manifest, wrote that emptiness to the preview, and torched the user's saved positions. Adding studioManualEditManifestRef.current.edits.length === 0 as a third condition for readFromDiskFirst makes the first call auto-bootstrap from disk while later calls (when memory has content) skip the disk read. Exactly the right shape for "the manifest is hydrated lazily on first use."

The inline comment explains why (empty-on-refresh → discards saved positions) and what (bootstrap once from disk), which is enough for a future debugger to follow without re-deriving from the PR.

One edge case worth verifying — delete-all resurrection

The condition edits.length === 0 is true in TWO distinct states, only one of which warrants a disk read:

  1. Fresh load, never bootstrapped — disk has truth, in-memory is empty. → Read disk. ✓ (This is the fix.)
  2. User deleted all edits via the UI — in-memory is empty by intent. → Should NOT read disk.

If a user clicks "delete all manual edits" and the next call to applyStudioManualEditsToPreview fires before the disk write flushes (whether the disk write is async/debounced, or whether the apply runs synchronously after the in-memory clear), the auto-bootstrap would re-read the still-stale disk file and resurrect the edits the user just deleted.

Two ways to close this:

  • Sync disk write on delete-all — guaranteed: if delete writes empty to disk synchronously before any subsequent apply, the resurrection can't happen because disk is also empty.
  • Distinguish "never bootstrapped" from "intentionally empty" — replace edits.length === 0 with an explicit hasBootstrapped boolean (or bootstrappedRef.current = false initially, flipped to true after the first disk read). Conceptually clearer; doesn't depend on the disk-write timing contract.

If the disk write is already synchronous on the delete path, this is a non-issue and the current fix is complete. Worth a quick grep on the delete-handler to confirm. Either way, not a blocker for shipping — the bug being fixed is a user-data-loss regression that's actively biting users; the edge case is a hypothetical that may or may not exist.

Rule 2 — sibling persistence triggers

Home's brief flagged this. The fix is scoped to applyStudioManualEditsToPreview's read path. Worth a quick check on adjacent triggers:

  • Project switch / composition change — does loading a different project reset the in-memory ref + re-bootstrap? If the same ref is reused across projects without reset, switching from project A (with edits) to project B (with different edits) might apply the wrong manifest because edits.length > 0 would skip the disk read.
  • Other consumers of studioManualEditManifestRef — does anything else (save handler, lint, validation) read from this ref directly? Those wouldn't trigger the auto-bootstrap.
  • useManifestPersistence's siblings — text edits, font changes, other persisted-to-disk state. Do any of those have the same "empty-on-refresh discards saved values" bug shape? Worth a one-PR sweep through the hook to check.

I haven't grep'd these myself — flagging them as the Rule-2 corollary. The current PR fixes the demonstrated bug; the sweep is the "are there other instances of the same bug shape" check.

CI

All required checks green: Lint ✓, Format ✓, Test ✓, Typecheck ✓, Build ✓, Test: runtime contract ✓, CLI smoke (required) ✓, regression ✓, preview-regression ✓, Preview parity ✓, Tests on windows ✓, Render on windows ✓, CodeQL ✓, player-perf ✓, Smoke: global install ✓. No reds. mergeable_state: "blocked" is probably required-reviewer; once we approve, it's good to merge.

Praise

  • Severity calibration is right. The "skull and crossbones" framing in the Slack post matches the user impact — every manual edit was silently lost on refresh on the alpha-shipped feature. A 7-line fix that ships immediately beats any larger refactor that ships next week.
  • Diagnosis quality. The PR body distinguishes "persisted to disk correctly" (write path was always fine) from "in-memory never seeded from disk on load" (read path was broken). Naming both halves explicitly prevents the future-debugger trap of "fix the disk path that wasn't actually broken."
  • Test plan covers the round-trip: move → refresh → verify position preserved. Plus the cross-project switch case (item 4) — exactly the scenario where Rule 2 would catch a sibling bug.

Verdict

APPROVE. Ship as soon as someone can sanity-check the delete-all edge case (or just observe production behavior — if no one immediately reports "deleted my edits got resurrected", the disk-write timing is sync and the edge case isn't real). The sibling-trigger sweep is a useful follow-up.

— Rames Jusso (pr-review)

… manifest

Two follow-up fixes from review:

1. Replace edits.length === 0 with an explicit manifestBootstrappedRef boolean.
   The old condition was true in two distinct states: never-bootstrapped AND
   user-deleted-all-edits. Because the delete-all disk write is async-queued,
   there was a window where applyStudioManualEditsToPreview could read stale
   disk content and resurrect just-deleted positions. The boolean flag is set
   on the first apply and reset on project switch, cleanly separating the two
   states.

2. applyStudioMotionToPreview had the identical bug: GSAP motion edits were
   also lost on page refresh. Applied the same motionBootstrappedRef pattern.
@miguel-heygen miguel-heygen merged commit c3f70c9 into main May 13, 2026
41 checks passed
@miguel-heygen miguel-heygen deleted the worktree-fix+studio-position-persistence branch May 13, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants