refactor(studio): extract shared timeline components and deduplicate code

[miguel-heygen]

Summary

Extract shared utilities to reduce duplication across timeline components. No new behavior — pure refactoring prep for the razor tool.

• PlayheadIndicator: shared playhead rendering (was duplicated in TimelineCanvas and TimelineEditorNotice)
• useContextMenuDismiss: outside-click/Escape dismiss pattern (was duplicated in ClipContextMenu and KeyframeDiamondContextMenu)
• TimelineCallbacks: shared callback interfaces (was duplicated in NLELayout and Timeline props)
• useTimelineZoom: consolidated 4 zoom store selectors into one hook
• timelineElementSplit: shared canSplitElement, buildPatchTarget, readFileContent
• gsapParser.test-helpers: shared test utilities for parser specs

Stack

1/3 — this PR (cleanup)
2/3 — #1330 (GSAP split engine)
3/3 — #1331 (razor tool UI)

[miguel-heygen]

• feat(studio): razor/blade tool UI for timeline clip splitting #1331
• feat(core): GSAP-aware split engine for timeline clip splitting #1330
• refactor(studio): extract shared timeline components and deduplicate code #1329 👈 (View in Graphite)
• main

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

[james-russo-rames-d-jusso]

Covering canonical rubric only; deferring HF-domain (GSAP timeline semantics, NLE conventions) to Vai.

Looks good on the canonical axes.

What I checked
• TS quality — no hallucinated APIs, helper signatures (useContextMenuDismiss, canSplitElement, buildPatchTarget, readFileContent, PlayheadIndicator, useTimelineZoom, TimelineCallbacks) match call sites. Reasonable abstractions, not over-engineered — every extraction has 2+ call sites in the diff.
• Public API surface — nothing exported from @hyperframes/core changes; all new files are internal to packages/studio or the parser test-helpers. Safe.
• Cross-stack — no consumer impact (pacific, SDK 1325).
• Test count — gsapParser.test-helpers.ts is dedup of test scaffolding, not new behavior. Stress + parser tests still exercise the same assertions through the new helpers (expectRawWithResolvable, parseAndSerialize, expectKeyframesFormat, etc.). LGTM.
• Bundle — no new dependencies.

Nits
• packages/studio/src/utils/timelineElementSplit.ts:1 — // fallow-ignore-file dead-code at the top of a file that's used in multiple places downstream (1330/1331) feels like belt-and-suspenders since the next two PRs in the stack consume every export. Once the stack lands, the directive becomes a lie. Worth removing in #1331 if not before. (nit)
• packages/core/src/parsers/gsapParser.test-helpers.ts:1 — same fallow-ignore-file dead-code — but here it's at least defensible because the helpers are test-scope. (nit)

What I didn't verify
• That PlayheadIndicator renders pixel-identical to the two inline copies it replaces — visual diff would be Vai's lane via Loom.
• That useContextMenuDismiss preserves the exact same mousedown ordering vs other document-level listeners (probably fine — useEffect ordering hasn't changed).

— Rames D Jusso

[vanceingalls]

Reviewed as a refactor PR meant to deduplicate. The extraction work is mostly clean (PlayheadIndicator, useContextMenuDismiss, useTimelineZoom, TimelineCallbacks) — those four are well-scoped, semantics-preserving, and reduce duplication. The problem is packages/studio/src/utils/timelineElementSplit.ts: it claims to consolidate but instead adds a second copy of helpers that already exist in packages/studio/src/hooks/timelineEditingHelpers.ts, and the new copies are less safe than the originals. Verdict on this PR is REQUEST_CHANGES on that file; the rest of the changes are fine.

Blockers

1. readFileContent in timelineElementSplit.ts drops the path-safety guard.

The existing copy in hooks/timelineEditingHelpers.ts (line 130-141) has:

if (targetPath.includes("\0") || targetPath.includes("..")) {
  throw new Error(`Unsafe path: ${targetPath}`);
}

The new copy omits this guard entirely. In PR #1331 this new copy is what useRazorSplit.ts imports, so the razor tool runs without the path guard. Two copies of the same helper with different safety semantics is a defect that will silently regress every time the "safer" copy is updated. Either import from timelineEditingHelpers.ts or move the canonical version into the new util and import there.

2. buildPatchTarget in timelineElementSplit.ts drops the hfId branch.

The existing copy handles hfId as a first-class selector (line 16-34 of timelineEditingHelpers.ts). The new copy does not — for an element that only has hfId (no domId, no selector), the new function returns null. sourcePatcher.ts line 280 actively uses target.hfId, and playerStore.ts:26 declares hfId?: string on TimelineElement, so this is a real code path. Once #1331 wires razor-split through this helper, splits on hfId-only elements will silently fail with "Clip is missing a patchable target."

Fix: drop the timelineElementSplit.ts copies of buildPatchTarget and readFileContent, import from timelineEditingHelpers.ts. Keep canSplitElement in timelineElementSplit.ts since it's the only genuinely-new utility.

Important

3. canSplitElement widens the set of splittable elements but the existing handler in this PR still rejects non-video/audio/img.

Pre-PR, ClipContextMenu checked ["video", "audio", "img"].includes(element.tag). Now it checks canSplitElement(element), which returns true for any non-locked element with explicit timing — including divs, text, svg, etc. But handleTimelineElementSplit in useTimelineEditing.ts:398-408 still rejects non-video/audio/img with no toast — a silent no-op. The menu will offer "Split at 2.34s" on a div clip, the user clicks it, nothing happens. UX inconsistency until #1331 lands. Either restrict canSplitElement to the same set or land the widening only in #1331 alongside the handler change.

Nits

4. // fallow-ignore-file dead-code on timelineElementSplit.ts. The buildPatchTarget and readFileContent exports are unused as of this PR — they only get wired up in #1331. Reviewer-hostile because it hides the fact that a tool flagged them as dead. If you go with the import-from-timelineEditingHelpers fix above, this comment can come out.

**5. expectKeyframesFormat's count arg and the test using expectKeyframe(kfs[1], 50, { x: 100 }, "power2.out") — the helper checks only the listed properties, not that no other properties leaked in. A bug that adds an extra property to a keyframe would slip past. Consider adding a strict-equal assertion.

— Vai

[james-russo-rames-d-jusso]

R2 — LGTM. The single nit from R1 was resolved, and the rewrite of timelineElementSplit.ts also fixed Vai's two blockers (path-safety guard drop, hfId selector drop) by reducing the file to a thin re-export of timelineEditingHelpers.ts. Clean.

R1 nit (resolved)
• packages/studio/src/utils/timelineElementSplit.ts:1 — // fallow-ignore-file dead-code directive is gone. The file now imports/re-exports buildPatchTarget and readFileContent from ../hooks/timelineEditingHelpers instead of duplicating them — single source of truth. ✅

What I didn't re-verify (Vai's lane)
• That PlayheadIndicator extraction still renders pixel-identical.
• gsapParser.test-helpers.ts semantics (Vai's expectKeyframesFormat strict-equal nit).

— Rames D Jusso

[vanceingalls]

LGTM. Confirmed with Rames's pass: the R1 finding (fallow-ignore lie) is resolved — timelineElementSplit.ts is now a thin re-export from timelineEditingHelpers, no suppression, plus a canSplitElement helper that consolidates the splittability predicate previously duplicated across TimelineToolbar, useAppHotkeys, and useRazorSplit. Good consolidation.

Review by Vai

[james-russo-rames-d-jusso]

R3 — no change since R2, still LGTM.

HEAD SHA 5b0d7b21 is the same as R2 (Vai approved at this SHA). No new commits; nothing to re-verify. Canonical-axes verdict stands.

— Rames D Jusso

[james-russo-rames-d-jusso]

R4 — no logic change since R3, still LGTM.

HEAD moved 5b0d7b21 → dafa301c — pure rebase onto newer main. Verified the four PR-specific files are byte-identical with R3 via GitHub Contents API blob SHAs:

• packages/studio/src/utils/timelineElementSplit.ts — same blob.
• packages/studio/src/player/components/PlayheadIndicator.tsx — same blob.
• packages/studio/src/hooks/useContextMenuDismiss.ts — same blob.
• packages/core/src/parsers/gsapParser.test-helpers.ts — same blob.

Vai's R2 approval on 5b0d7b21 carries. Nothing to re-verify on the canonical axes.

— Rames D Jusso

[miguel-heygen]

Merge activity

• Jun 11, 3:44 AM UTC: Graphite couldn't merge this PR because it failed for an unknown reason (GitHub is reporting that this PR is not mergeable, despite passing required status checks defined by your branch protection rules. Please check your rulesets for additional blocking criteria. Graphite Merge Queue does not currently support rulesets. Please contact Graphite support for further assistance.).