Skip to content

fix(studio): the host can name the Split key shown in the clip menu - #4985

Merged
miguel-heygen merged 3 commits into
mainfrom
fix/studio-clip-menu-split-shortcut
Oct 4, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
fix/studio-clip-menu-split-shortcut

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 4, 2026 •

Copy link
Copy Markdown
Collaborator

What

A host that embeds Studio's timeline can now name the key its clip menu shows beside Split, with a new optional splitShortcut prop on Timeline, EditorShell and TimelinePane. Without it the menu shows S, as before.

Why

The clip menu's Split row had S hard-coded. In Studio, S splits, so that is right. An embedder that binds Split to another key (for example Option+S) had no way to change the hint, so its menu showed a key that does nothing there.

Related work

None. A small standalone change: no open PR in this repo carries related clip menu work.

How

  • studioShortcuts.ts now exports SPLIT_SHORTCUT_HINT, built from STUDIO_PLAIN_KEYS.split. The shortcuts panel and the clip menu both read it, so the default lives in one place.
  • splitShortcut travels the same path as clipMenuItems: EditorShell and TimelinePane forward it to Timeline, then the provider state, the overlay state, and ClipContextMenu.

Test plan

  • Unit tests added/updated:
    • ClipContextMenu.test.tsx: the Split row reads S by default and the host's label when passed. Putting the hard-coded S back fails it.
    • TimelineOverlays.test.ts: a label set on the timeline's overlay state reaches the menu. Removing the overlay hop fails it.
    • 187 tests across the clip menu, overlays, editor shell and NLE folders pass, 3 runs in a row.
  • Typecheck, oxlint and oxfmt clean on the changed files.
  • Manual testing performed: the captures below render the clip menu alone in a throwaway page (not committed), as a host passing ⌥S would, in light and dark. Studio's own app shows no visible change.
  • Comments follow CONTRIBUTING.md "Comments".

Before

A host passes ⌥S; the menu still shows S.

before, light
before, dark

After

A host passes ⌥S; the menu shows it. With no label the menu keeps S, as in Before.

after, light
after, dark

@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 1556 (base branch 1556), smooth 1410 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 4, 2026 05:12

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at efe76000. The code is right. Two CI reds need fixing before merge, and one of them is a required check.

Label vs. binding. Leaving the binding with the host is the right shape. Studio binds S in appHotkeysDispatch.ts:167 (STUDIO_PLAIN_KEYS.split), through useAppHotkeys. Only App.tsx mounts that hook, and src/index.ts does not export it. So a host embedding EditorShell or TimelinePane never got Studio's S binding anyway: the host already owns the key, and the menu label is the only part it couldn't set. The label and the key can disagree only if a host passes a label for a key it doesn't bind, which the TimelineTypes doc already covers. This matches how the shortcuts panel works: hosts already relabel it through shortcutSections.

Default path. SPLIT_SHORTCUT_HINT = hintKey("s"), which is "S", the same string the menu hard-coded before. The shortcuts-panel entry reads the same constant, so it is unchanged too.

Every render site. ClipContextMenu renders only from TimelineClipMenuOverlay (TimelineOverlays.tsx). CanvasContextMenu and TrackGapContextMenu only mention it in comments. The path is EditorShell, then EditorShellBody / TimelinePane, then Timeline, then useTimelineProviderState, then the ...state spread in useTimelineOverlaysState, then the overlay. That is the same path as clipMenuItems. The only other "S" hint is the toolbar tooltip "Split at playhead (S)" in TimelineToolbar.tsx:383. That toolbar mounts only in App.tsx and isn't exported, so hosts never see it.

Types. The prop is declared on EditorShellProps and TimelinePaneProps, and both are exported. TimelineProps itself isn't exported from index.ts, but that was already true for clipMenuItems.

Verified locally:

  • 278 studio test files pass (3516 tests), including every test file that imports ClipContextMenu, TimelineOverlays, studioShortcuts or ShortcutsPanel.
  • tsc --noEmit is clean.
  • Putting the hard-coded S back fails both ClipContextMenu cases plus the overlay case.
  • Dropping splitShortcut={overlay.splitShortcut} fails the overlay case.

CI to fix before merge:

  1. Studio and player captures (required). The capture "after, dark, no host label" (9c22f2a8) is byte-identical to "before, dark" (e757941b). That's actually correct: the default rendering is unchanged, which is what you want. But the gate reads any After capture that matches a Before capture as a misattached recording. Drop that image, or move it out from under ## After. Either is a PR-body edit, so no push is needed.
  2. Comments (not required). The comment ratchet flags two added comment lines, one in studioShortcuts.ts and one in TimelineTypes.ts. Both files' comment share went up. Fixing this means a push, and HF OSS needs a fresh approval at the new head, so re-pin me if you push.

Nonblocking test gap. If I delete the splitShortcut, line that useTimelineProviderState hands to the overlay state, all 4071 tests in src/player/components and src/components still pass, and tsc doesn't catch it either, because the prop is optional. The overlay test sets the label on the overlay state directly, so nothing covers the step from the Timeline prop to the overlay. A single test that renders Timeline with splitShortcut and opens the clip menu would cover the whole path.

— Rames

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-approving at 77ccbd90. The only change since efe76000 (my approval 5404454367) is the removal of two doc-comment lines, one in TimelineTypes.ts and one in studioShortcuts.ts. No code changed. The two checks that were red at the previous head now pass at this one:

  • Comments
  • Studio and player captures (required)

My earlier findings still apply, and the non-blocking test gap is unchanged. Nothing tests the step where useTimelineProviderState forwards splitShortcut into the overlay state.

— Rames

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review at 77ccbd90 (full PR, 10 files, +70/−2). This is a comment, not an approval. No blockers.

The optional splitShortcut reaches the clip menu through every layer: EditorShellProps / EditorShellBody / TimelinePaneProps → TimelineProps → useTimelineProviderState → useTimelineOverlaysState → TimelineOverlaysState → TimelineClipMenuOverlay → ClipContextMenu. When no prop is passed, the label is still S: ClipContextMenu defaults to SPLIT_SHORTCUT_HINT, which is hintKey(STUDIO_PLAIN_KEYS.split), the same value the shortcuts panel already used. The change only touches the label; the key binding is unchanged.

Notes (none block):

  • An empty string is a value, not "absent": splitShortcut="" renders an empty hint span rather than S, because the default only applies to undefined. That's probably the right way for a host to hide the hint, but no test covers it.
  • I agree with Rames's survivor: dropping the useTimelineProviderState line still passes. The two new tests enter below that hop: one renders ClipContextMenu directly and the other injects overlay state. A test that renders Timeline with the prop would cover the whole path.

CI at this head: Test (studio) and both captures checks pass. At posting time, 22 checks were still pending, including edit accuracy, and none were red. I tried to run the two new test files in a parked clone, but neither loaded because the clone's dependencies are stale. So for test results I'm relying on CI, not a local run. Own pass only; no Codex.

— Somu

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit dda0e75 Oct 4, 2026
168 of 169 checks passed
@miguel-heygen
miguel-heygen deleted the fix/studio-clip-menu-split-shortcut branch October 4, 2026 06:19
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.

3 participants