fix(studio): captions UX — mode exit, undo, autosave surfacing, honest gating

[vanceingalls]

Summary

Caption-editing fixes from the studio UX review — this surface held 5 of the 13 criticals. The dominant theme: the editing UI had shipped far ahead of its apply/persist pipeline, so several controls mutated an in-memory model with no downstream effect, and the mode itself could never be exited.

Changes

Mode trap (critical):

• Caption edit mode auto-activated on detection and had no exit — setEditMode(false)/reset() had zero call sites, and the caption overlay replaced normal element editing for the rest of the session, even after switching compositions. Now: store resets on composition change, an "Editing captions · Exit" pill sits on the preview, and a re-enter button appears when dismissed.

Honest gating of dead surfaces (2 criticals):

• Animation tab (31 presets × duration/ease/stagger/intensity) edited state that was never applied to playback nor serialized. Wiring it means extending the CaptionOverride schema in packages/core plus a runtime engine to rebuild entrance/highlight/exit tweens — deferred. The tab is now visibly disabled with an amber "isn't applied to playback or saved yet" notice instead of silently discarding work.
• Timing edge-drags moved a block that never changed playback and never saved; the handles are removed (blocks remain as select/seek targets). Verified bonus: double-click split desynced the overlay↔DOM index mapping (split grows groupOrder but not the iframe's .caption-group count) — split is out until regeneration exists.

Undo (critical):

• Store-level undo/redo stacks (cap 50, 800ms coalescing by edit-target) across all 10 mutations; ⌘Z/⇧⌘Z intercepted while caption mode is active and reapplied to the live iframe. Previously ⌘Z reverted an unrelated file edit while the bad caption drag persisted.

Autosave (critical):

• Save failures (incl. non-2xx) surface a persistent "not saved — Retry" banner (the code's own comment called this a data-loss path; it was telemetry-only). Debounced saves flush on unmount instead of being discarded; beforeunload flushes + warns while pending; corrupt overrides JSON is distinguished from a missing file.

Input safety + a11y:

• Arrow-key nudge no longer hijacks arrows inside form inputs; all numeric fields are NaN-proof (finite-only commits — typing "-" used to inject NaN into gsap and persist null); "Mixed" indicator on multi-select divergence; Escape cancels an in-flight drag and restores the pre-drag transform; ⌘A selects all; timeline blocks are keyboard-selectable with a playhead line and click-to-seek (the onSeek prop existed but was never passed); 24px hit areas around the 8px handles; zero-visible-boxes hint ("scrub to a caption or select it in the track below"); visible input focus styles; tablist semantics.

Perf: the 66ms getBoundingClientRect polling loop is replaced with event-driven updates (player-store subscription, preview messages, ResizeObserver, rAF-coalesced) — the interval only runs during playback.

Verification

• vitest run src/captions: 58/58 pass; oxlint/oxfmt/tsc clean

Stack

PR 7/7 (top) of the studio UX-review fixes — this branch is the fully-green state: tsc clean, 1189 studio tests + 25 render-route + 249 player tests passing, full bun run build green.

🤖 Generated with Claude Code

[vanceingalls]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(studio): captions UX — mode exit, undo, autosave surfacing, honest gating #1968 👈 (View in Graphite)
• fix(studio): player UX — keyboard timeline, honest waveform, keyframe menu actions #1967
• fix(studio): sidebar/panels UX — asset delete confirm, rename, search trap, undo #1966
• fix(studio): editor panel UX — commit safety, keyboard a11y, wired BlockParamsPanel #1965
• fix(studio): shell UX — data-loss guards, error surfacing, dialog contracts, toasts #1964
• feat(studio,studio-server,cli): render cancel end-to-end + renders/nle/storyboard UX #1963
• feat(studio): harden ui primitives — focus rings, keyboard tooltips, dialog hook #1962
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

Review: fix(studio): captions UX — mode exit, undo, autosave surfacing, honest gating

VERDICT: Approve (comment-only per process)

TITLE: PR 7/7, the capstone — rewires 5 of the 13 studio UX criticals in the captions surface.

Summary

This is a well-structured fix for a cluster of real problems: a mode trap (caption editing auto-activated with no exit), dead-letter controls (animation panel + timing drags mutated state that was never applied or saved), no undo for canvas edits, silently swallowed autosave failures, and arrow keys hijacking form inputs. The author's honest-gating approach is the right call — disabling controls that lie is better than ripping out the UI or wiring a half-baked pipeline.

The diff is clean, well-commented, and internally consistent. All mutations push history. The autosave lifecycle (debounce → flush on unmount → flush + warn on beforeunload → error banner with retry) is thorough. The 66ms polling → event-driven + rAF-coalesced refactor in CaptionOverlay is a solid perf win. CI is green.

Ponytail Lens (system-level coherence)

Strong. The PR respects module boundaries throughout:

• Undo lives in the caption store (not wired into the file-level history system), and useAppHotkeys dispatches to it only when caption mode is active — correct layering.
• The iframe registry (registerCaptionIframe / applyCaptionModelToIframe) is a clean seam for non-component code to push state into the preview without coupling to React render cycles.
• dismissed flag in the store + reset-on-composition-change in useCaptionDetection closes the state lifecycle properly — no stale caption models leaking across compositions.
• Error banner state lives in the store (not component-local), so the sync hook can drive it from async fetch callbacks — correct for this control flow.

The ANIMATION_PIPELINE_WIRED = false constant is a good honest gate pattern. When the pipeline is eventually wired, a single constant flip enables everything — no scattered feature flags.

Findings

[Medium] Duplicate isEditable* utilities
keyboard.ts introduces isEditableEventTarget(target) which checks INPUT|TEXTAREA|SELECT|isContentEditable. There is already a more robust isEditableTarget(target) in utils/timelineDiscovery.ts (lines 14-29) that additionally handles ARIA roles (textbox, searchbox, combobox) and ancestor-walks via .closest() for nested editable contexts (CodeMirror editors, etc.). These should be consolidated to avoid subtle divergence — e.g., a role="textbox" caption input field today would let arrow-key nudging steal focus under the simpler check but not under the existing one.

[Low] NumberField does not clamp on commit
NumberField in shared.tsx passes min/max to the <input>, but the handleChange callback commits parsed to the store without clamping to the [min, max] range. Browser-native clamping only applies to spinner clicks / step increments — typed values bypass it. So typing 999 into a "Duration" field with max={2} commits 999 to the model. This is the same category as the NaN fix (invalid values persisting) and could be tightened:

if (raw.trim() !== "" && Number.isFinite(parsed)) {
  const clamped = min != null && max != null
    ? Math.min(max, Math.max(min, parsed))
    : parsed;
  onCommit(clamped);
}

[Low] beforeunload save is fire-and-forget
In useCaptionSync, the handleBeforeUnload handler calls save() (which initiates a fetch() returning a Promise) and then calls e.preventDefault() to show the browser's "unsaved changes" dialog. But save() is async and the tab may close before the PUT completes. The comment says "best-effort" which is honest, but navigator.sendBeacon() would improve reliability here for the actual save payload. Not blocking, since the browser confirmation dialog gives the user a chance to stay.

[Nit] Export style inconsistency in keyboard.ts
shouldHandleCaptionNudgeKey uses inline export function, while the new isEditableEventTarget uses a trailing export { ... } re-export. Minor, but worth aligning for consistency within this small file.

[Nit] Store reset preserves retrySave but not syncError
reset() spreads initialState (which has syncError: null) and explicitly preserves retrySave. This means if a corrupt-overrides error was showing when the user switches compositions, the error clears silently. That's probably fine (new composition = fresh state), but worth a conscious note since the comment only mentions retrySave.

DIFF_STATS

• 13 files changed, +736 / -177
• Core changes: store.ts (undo/redo + history), CaptionOverlay.tsx (perf refactor + escape/select-all), useCaptionSync.ts (error surfacing + beforeunload), CaptionTimeline.tsx (timing handles removed, playhead + keyboard nav added), StudioPreviewArea.tsx (mode pill + error banner + re-enter button)
• Tests: keyboard.test.ts expanded for editable-target check
• CI: all green

Well-executed capstone PR for the stack. The medium finding (duplicate editable check) is worth addressing before this ships — the rest is polish.

Review by Miga

[miguel-heygen]

Stack review for PR 7/7.

Audited: packages/studio/src/captions/store.ts, hooks/useCaptionSync.ts, keyboard.ts, components/shared.tsx, CaptionOverlay.tsx, CaptionTimeline.tsx, StudioPreviewArea.tsx, and the hotkey/detection wiring at head 64673c1.

The mode-exit lifecycle is coherent: explicit dismissal suppresses auto re-entry, composition switches reset the lifecycle, and the UI has a clear path back into caption edit mode. Caption undo is store-level with bounded history and coalescing, and autosave failures are surfaced through store-backed error state plus retry rather than disappearing into telemetry. The beforeunload path is honestly best-effort, but it at least flushes the debounce and warns while writes are pending.

Remaining items are non-blocking: the editable-target helper should eventually consolidate with the richer timeline helper, and numeric fields should clamp typed values to min/max before commit.

Verdict: APPROVE
Reasoning: The captions delta replaces silent/dead behaviors with explicit mode, undo, save-error, and disabled-state handling; I did not find a blocker in the audited paths. This approval does not remove the separate #1965 stack blocker.

— Magi

[vanceingalls]

Addressed at head 2627d6d51: the duplicate editable-target check is consolidated — captions/keyboard.ts now delegates to the richer isEditableTarget from utils/timelineDiscovery (ARIA textbox/searchbox/combobox + .closest() ancestor walk) and re-exports it, killing the divergence risk and the export-style inconsistency in one move. NumberField clamps typed values to [min, max] before commit. The sendBeacon idea and reset-clears-syncError notes are acknowledged as conscious trade-offs.

🤖 Generated with Claude Code

[james-russo-rames-d-jusso]

Reviewed at 2627d6d.

Building on Miga's detailed review + Magi's approval, layering the following net-new items. Two of Miga's findings appear to have been resolved at HEAD after her review posted:

• Her [Medium] duplicate-isEditableEventTarget finding — keyboard.ts now re-exports isEditableTarget from utils/timelineDiscovery.ts (export { isEditableTarget as isEditableEventTarget }), so the two can no longer diverge. Nice.
• Her [Low] NumberField clamp-on-commit finding — NumberField.handleChange in shared.tsx now clamps parsed to [min, max] before calling onCommit, exactly the shape she sketched.

The capstone shape is strong overall — honest gating on the animation panel and timing-drag removal is the right call, and the undo / autosave lifecycle work is load-bearing product hardening. Below is the second-pass lens.

Should-fix

Comp-switch save race can surface stale errors on the new composition. hooks/useCaptionDetection.ts × hooks/useCaptionSync.ts — on composition switch, useCaptionDetection's reset effect calls store.retrySave?.() (fires an async PUT) THEN store.reset() (drops the model, clears syncError). But save()'s .catch calls useCaptionStore.getState().setSyncError("Caption changes couldn't be saved") on whatever store is current when the promise settles — which is the NEW composition's store after reset(). Result: a user on comp B can see an error banner for a failure that belongs to comp A they've already left, with no obvious way to reconcile.

Fix shape mirrors the pending-flag pattern already in the .then branch: capture editSeqRef.current at save() entry and drop the setSyncError call in .catch if the sequence changed. (The .then already has if (editSeqRef.current === seqAtSave) pendingRef.current = false — extending that to also gate setSyncError closes the leak.)

Observability gap on the new user-visible failure / action paths. useCaptionSync.ts already emits studio_caption_autosave_failed (good pattern), but the surrounding lifecycle work is telemetry-silent:

• useCaptionSync.ts corrupt-JSON path — setSyncError("caption-overrides.json is corrupt — earlier caption edits didn't load") emits nothing. Users see a banner; team has no signal on frequency — invisible in aggregate.
• StudioPreviewArea.tsx — Retry button click after autosave failure. Knowing "user recovered" vs "user gave up" is the recovery-effectiveness signal.
• StudioPreviewArea.tsx — Exit / Re-enter caption mode. This IS the auto-trap fix; without a signal there's no way to know whether users actually use it or whether the trap is still trapping in a shape we haven't modeled.
• store.ts undo/redo — adoption signal for the new capability, and a proxy for how load-bearing the new stack really is.

None blocking; the studio_caption_autosave_failed template extends naturally. A single follow-up adding these events would close the loop on this PR's own thesis (the whole point of surfacing failures is prioritizing them).

Nits

No unit tests for the store's undo/redo behavior. HISTORY_COALESCE_MS = 800 coalescing by key, HISTORY_CAP = 50, model-shape restoration, key-based coalescing (seg-style:${segmentId} vs seg-timing:${segmentId} never coalesce into each other) — all easy to regress silently. keyboard.test.ts set the precedent for happy-dom store-adjacent tests; a store.test.ts would pin the undo semantics that the ⌘Z hotkey routing depends on.

No test coverage for useCaptionSync lifecycle. The beforeunload flush + warn, unmount flush, retrySave registration, and corrupt-JSON error path are all data-loss-adjacent — regressions would silently reintroduce the exact silent-swallow this PR fixes, and pass CI.

APG tabs completeness in CaptionPropertyPanel.tsx. role="tablist" + role="tab" + aria-selected are added — good. Missing: role="tabpanel" + aria-controls/aria-labelledby linkage between the tab and the panel body, and arrow-key nav between tabs (APG expects Left/Right to switch tabs; users currently need to Tab through everything else first). Forward-looking; not blocking.

Reset preserves retrySave but silently clears syncError. store.ts reset() — Miga flagged this as intended-but-not-documented; noting that combined with the race finding above, the silent clear is what makes the cross-comp leak invisible during code review. If the race fix lands, this becomes a non-issue.

CaptionOverlay.tsx — no e.isComposing guard on the window keydown. During IME composition (Japanese / Chinese / Korean input), Escape and ⌘A may interfere with input-method cancel/commit semantics. Small, but the handler already respects isEditableEventTarget for arrow-nudges — worth extending to the Escape / ⌘A branches too (if (e.isComposing) return; at the top).

— Review by Rames D Jusso