fix(studio): the host can name the Split key shown in the clip menu - #4985
Conversation
Edit accuracy: accurate 1556 (base branch 1556), smooth 1410 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
There was a problem hiding this comment.
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,studioShortcutsorShortcutsPanel. tsc --noEmitis clean.- Putting the hard-coded
Sback fails bothClipContextMenucases plus the overlay case. - Dropping
splitShortcut={overlay.splitShortcut}fails the overlay case.
CI to fix before merge:
- 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. - Comments (not required). The comment ratchet flags two added comment lines, one in
studioShortcuts.tsand one inTimelineTypes.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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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 thanS, because the default only applies toundefined. 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
useTimelineProviderStateline still passes. The two new tests enter below that hop: one rendersClipContextMenudirectly and the other injects overlay state. A test that rendersTimelinewith 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
What
A host that embeds Studio's timeline can now name the key its clip menu shows beside Split, with a new optional
splitShortcutprop onTimeline,EditorShellandTimelinePane. Without it the menu showsS, as before.Why
The clip menu's Split row had
Shard-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.tsnow exportsSPLIT_SHORTCUT_HINT, built fromSTUDIO_PLAIN_KEYS.split. The shortcuts panel and the clip menu both read it, so the default lives in one place.splitShortcuttravels the same path asclipMenuItems:EditorShellandTimelinePaneforward it toTimeline, then the provider state, the overlay state, andClipContextMenu.Test plan
ClipContextMenu.test.tsx: the Split row readsSby default and the host's label when passed. Putting the hard-codedSback fails it.TimelineOverlays.test.ts: a label set on the timeline's overlay state reaches the menu. Removing the overlay hop fails it.⌥Swould, in light and dark. Studio's own app shows no visible change.Before
A host passes
⌥S; the menu still showsS.After
A host passes
⌥S; the menu shows it. With no label the menu keepsS, as in Before.