Repository navigation
Add a timeline strip with playback controls and keyframe editing to the menu editor - #126
Merged
Merged
Conversation
ScottMorris
added
enhancement
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a1a68cb04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
ScottMorris
force-pushed
the
menu/08-timeline-ui
branch
from
August 19, 2026 14:37
5a1a68c to
13650b6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13650b6f08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This was referenced Aug 19, 2026
ScottMorris
force-pushed
the
menu/08-timeline-ui
branch
from
August 19, 2026 16:29
13650b6 to
0a71ce8
Compare
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a71ce82c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 157e1aa471
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8968e99046
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01b9f8fbac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dea339fb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
…ames Two codex round-H findings on the timeline: - Dragging a loop edge could shorten `loopDurationSecs` below the latest keyframe's loop-relative timestamp, stranding it out of range so an ordinary region drag failed the next build on `menu.motion-keyframe-out-of-range`. `TimelineRegionBar` now takes `minLoopDurationSecs` (the menu's latest keyframe timestamp, wired from `TimelineStrip`) and both loop edges clamp to it. - Deleting the selected keyframe with Delete/Backspace cleared only `selectedIndex`; an open popover kept its stale `popoverIndex` and silently rebound to the next keyframe. The keyboard path now closes the popover when deleting at or before its index — the same close-on-reorder guard the insert and retime paths use. test: region-bar clamps for both loop edges against a 6-second keyframe; keyboard delete with an open popover asserts the dialog closes.
…ar popover-delete selection Three editor-preview fixes: - **Preview text rendered at raw design px** (found live by Scott): node boxes scale with the displayed canvas via percentages, but `fontSize`/`letterSpacing` stayed absolute, so glyphs drew ~18% larger than the disc at 100% zoom and drifted further at other zooms. The viewport is now a CSS size container and text nodes plus button labels express their design-space sizes in `cqh` (`fontSize / designHeight * 100cqh`), keeping glyph proportion identical to the Skia render at every zoom. Verified live: computed 36.94px against the 37.1px disc-proportional expectation in a 405px viewport. - **Codex: `ended` beat the rAF wrap** — a loop window ending at the source's end could emit `ended` before the playhead tick observed the boundary, and the handler unconditionally paused. It now wraps to the loop start and resumes when the loop region is enabled, pausing only otherwise. - **Codex: popover delete left a stale selection** — the successor keyframe inherited the deleted keyframe's index and its selection, so a follow-up lane Delete keypress removed it unintentionally. The popover delete now clears the selection like the keyboard path does. test: ended-wrap case (mocked `HTMLMediaElement.play`), popover-delete selection regression.
Codex round-J finding: an in-flight blob fallback outlived its component — unmounting (or switching backgrounds) mid-download left the fetch consuming the full video, and a late completion created an object URL on a setter that no longer renders, so nothing ever revoked it. The fetch now carries an `AbortController` aborted from both the unmount cleanup and the asset-switch reset, and the completion path checks a mounted flag before creating the URL.
…mary `animationWriters.ts` opened with a multi-sentence implementation rationale ahead of the licence block — AGENTS.md's licence-header convention wants a concise one-sentence summary immediately before the copyright/SPDX block, with any further detail moved below it. Same fix pattern commit `c301eb1` already applied to `TimelineStrip.tsx` and `timelineUtils.ts`. No functional change.
…s empty
The popover's timestamp/opacity number inputs committed on every
`onChange`, and `Number('')` is `0` — clearing the field to type a
replacement immediately retimed the keyframe to 0. For a non-first
keyframe that crosses its earlier neighbour, re-sorts the track, and
closes the popover (the stale-index reorder guard `TimelineKeyframeLane`
already applies), leaving no way to finish typing the intended value.
Give both fields a local text draft: `onChange` always echoes the raw
text so the field can sit empty mid-edit, but only commits through
`onChangeTimestamp`/`onChangeValue` when the draft parses to a finite
number. The draft re-syncs from the keyframe's actual value whenever
that changes from outside the draft (switching keyframes, or the
value this same draft just committed coming back through as a prop),
and `onBlur` reverts an abandoned empty/invalid draft rather than
leaving the field stuck blank.
test: cover clearing the popover's timestamp field with a keyframe that
would otherwise retime to 0 and cross its neighbour.
…g drag Releasing a dragged keyframe diamond fires a native `click` right after `pointerup`. When the drag crossed a neighbour, `handlePointerUp` already clears `selectedIndex`/`popoverIndex` (the array just got re-sorted, so an index-keyed selection would silently point at whatever keyframe the sort left behind) — but that trailing click's own `onClick` handler immediately re-set `selectedIndex` back to its pre-drag index, undoing the guard. A follow-up Delete then removed whichever keyframe the re-sort left at that index, not the one the user actually dragged. The lane already tracks `hasMovedRef` to distinguish a real drag from a plain click-to-select (see the "commits exactly once on pointer-up" test). Reuse it: a click that lands with `hasMovedRef.current` still true is the drag's trailing click, not an independent selection gesture — consume it instead of calling `setSelectedIndex`. test: drag a keyframe past its neighbour, fire the trailing click, then assert a follow-up Delete does not remove anything (the selection stays cleared, as `handlePointerUp`'s reorder guard intended).
Double-clicking to insert the first keyframe on an `activate-colour` or `activate-opacity` track (nothing to sample yet, so `evaluateTrack` returns `null`) fell through `defaultValueForTarget` to the generic `position` (`point`) default — that fallthrough only handled `highlight-colour`/`highlight-opacity` explicitly. A `point` value on a colour/scalar target is a shape the preview and DVD-build planner both ignore, so the inserted keyframe was invisible. Source the activate pair from the menu's own authored `highlightColours.activateColour`/`activateOpacity`, mirroring the highlight pair right above it. Also extract `defaultValueForTarget` to a standalone, exported, pure function (taking `highlightColours` explicitly) so it can be unit-tested directly instead of only through `TimelineStrip`'s render tree. test: cover all six `AnimatableProperty` targets, including the two that previously fell through.
`handleRemoveNode` filters a deleted `group` node out of `scene.nodes` in a single top-level pass — that removes the whole child subtree in one go, since a `group`'s `children` are embedded directly on it. But it only ever called `removeNodeTracks` with the group's own id, so any descendant's animation tracks were left behind, referencing a scene node that no longer exists and tripping `menu.animation-node-missing` on the next validate. Add `collectSubtreeNodeIds`, which walks a node's own subtree (recursing into nested `group`s) and returns every id in it, and extend `animationWriters.removeNodeTracks` to accept either a single id or an array so the whole subtree's tracks can be dropped in one atomic call. `handleRemoveButton` doesn't need the same fix: a `button` scene node has no `children` field to recurse into (buttons can't nest today — `getMenuButtons` only scans top-level nodes), so its own id already is the whole subtree; a comment there notes why. test: `collectSubtreeNodeIds` over a leaf, a group with direct children, and nested groups; `removeNodeTracks` given an array of ids.
The HTML `min`/`max` attributes only mark the input invalid — the raw finite number still persisted, so a hand-typed `-1` or `2` was saved while CSS and the planner clamped it differently at render time. The draft-commit path now clamps before persisting.
… wins A hand-edited or imported document can carry multiple tracks for the same `(nodeId, target)` pair, and the consumers disagreed about which one counts: the DCSQ lowering's tie-break is last-listed-wins while the editor's writers (and the timeline's React keys) hit the first match, so editing the second lane silently mutated the first track. `MenuDocument::dedupe_animation_tracks` now runs from the load-path lift, keeping the LAST track per pair (preserving what the disc would have produced) so every consumer sees a single unambiguous track. `AnimatableProperty` gains `Hash` for the dedupe set. test: last-duplicate survives, relative order preserved, idempotent.
… cap on the stream Two codex round-M findings: - The editor's global undo can shrink a track's keyframe array outside every lane-guarded path, leaving `popoverIndex`/`selectedIndex` past the end — the next empty-lane double-click threw on `keyframes[popoverIndex].timestampSecs`. The lane now clears out-of-range indices when the array shrinks and the dereference sites are guarded. Regression test simulates the undo rerender. - The blob fallback's 512 MiB cap trusted `Content-Length`; a missing or malformed header bypassed it and `response.blob()` buffered the whole file. The cap is now also enforced while consuming the response stream, aborting the fetch when exceeded (with the header check kept as a fast-path reject and `response.blob()` retained as the no-stream fallback).
Codex round-N follow-on to the clamp fix: when the saved value already sits at a bound (opacity `1`, timestamp `0`/`loopDurationSecs`), typing past it persists the clamped value — which equals the saved one, so the prop-sync effect never fires and the field kept displaying the unsaved text. Both drafts now re-sync unconditionally on blur: unparseable drafts revert as before, and parseable-but-clamped ones snap to what was actually persisted.
…, not index Codex round-O found the third stale-index path (`handleAddKeyframeAtPlayhead` inserting through the parent, bypassing the lane's guarded insertion), so this ends the class instead of patching another instance: the selection and popover are now keyed by the keyframe's timestamp — its identity — with the writer-facing indices derived fresh each render. Consequences: - External inserts, reorders, drags, and popover retimes can no longer re-point the bindings at the wrong keyframe; they follow their keyframe through any re-sort (drag and retime paths retarget the stamp to the new timestamp before committing). - The `willReorder` close-guards became unnecessary and are gone — the popover now stays open on its own keyframe through reorders instead of closing, which is strictly better UX and is what the guards were approximating. - A binding whose keyframe doesn't currently exist derives to null and renders nothing, deliberately without clearing the stored stamp, so a retime whose parent commit lands a render later re-binds seamlessly; deleting through the popover clears its stamps explicitly. test: the reorder/insert tests now assert the popover stays bound to its own keyframe (verified against the parent-committed array via a rerender helper); the undo-shrink and trailing-click regressions still pass.
Codex round-P finding: with the lane's bindings keyed by timestamp, two keyframes sharing one timestamp were ambiguous (the lookup bound the first while the evaluator lets the later win). Rather than adding synthetic ids, duplicates are now unrepresentable: - `animationWriters.addKeyframe`/`moveKeyframe` merge on collision — inserting or retiming onto an occupied timestamp REPLACES the keyframe there (the acting keyframe survives, preserving the evaluator's later-wins outcome as the steady state). - `MenuDocument::dedupe_animation_tracks` also collapses shared timestamps within each track at load, keeping the LAST keyframe (the one the evaluator honoured), so hand-edited documents normalise before any consumer sees them. Also shortens `TimelineKeyframeLane.test.tsx`'s header to the one-sentence convention (codex round-P P1). test: writer merge-on-collision cases (insert and retime); Rust keep-last-on-shared-timestamp case.
Codex round-Q finding on the collision-merge interaction: with `moveKeyframe` replacing any keyframe at the destination, committing every parseable intermediate draft could destroy data — typing "10" passes through "1", which merges away a real keyframe at 1s that the final commit can't restore. The timestamp field now keeps its draft local and commits only on blur or Enter (unparseable drafts revert; committed values clamp to `[0, loopDurationSecs]` as before). The opacity field keeps per-keystroke commits — value updates are non-destructive.
…allback duration Codex round-R findings: - Enter keeps focus on the popover's timestamp field, so the later blur re-committed the same value — each commit is an undo entry, making the first Undo appear to do nothing (and a collision merge need two undos). Both commit paths now skip identity retimes. - A stale `play()` promise from a replaced video element (e.g. the keyed swap to the blob fallback) could settle late and overwrite the new element's `playing` state; the outcome now applies only while that element is still the registered one. - The timeline's fallback duration ignored the background asset's inspected `durationSecs`, so a failed preview (decode failure, blob cap) shrank the timeline to the authored windows and the region bar's max-edge clamp could permanently prevent extending the loop; the asset duration now participates in the fallback. Also trims `TimelineRegionBar.tsx`/`TimelineRuler.tsx` (flagged) and `TimelineRegionBar.test.tsx` (same pattern) to one-sentence headers.
… selected node Codex finding: `MenuEditor` registers a document-level Delete/Backspace shortcut that removes the selected scene node, and `preventDefault()` doesn't stop propagation — so deleting a keyframe with a node still selected removed both. The lane's keydown handler now stops propagation after handling the keyframe deletion.
… timelines; skip jitter commits Codex round-U findings: - The Motion Settings Duration input persisted any typed value, bypassing the latest-keyframe floor the region-bar drags enforce — stranding keyframes out of range. The floor now lives in `handleMotionDurationChange`'s writer so every duration input respects it (a cleared input's unset sentinel stays clearable). - `TimelineStrip` mounts before its `visible` early-return, so a plain still menu ran the playhead's `requestAnimationFrame` loop forever; `useVideoPlayhead` now takes an `enabled` flag and schedules nothing for a hidden strip. - Pointer jitter within one frame cell set `hasMovedRef` while snapping back to the original timestamp, committing an identity retime and burning an undo entry; the drag commit now requires the snapped time to actually differ.
…rags Codex round-V findings: - Prose spellings swept to `quantise`/`quantised`/`quantisation` across `SceneCanvas.tsx`, `timelineUtils.ts`/`.test.ts`, and `SettingsPage.tsx`'s dev-option description; the `quantizeOverlayPalette` identifier keeps the API spelling (it mirrors the Rust command's serde name). - A drag interrupted by `pointercancel` or lost capture left the region bar/keyframe lane in drag mode, letting a later stray pointer-up commit an unintended retime. Both now reset without committing on `pointercancel` (the region bar also on lost capture; the lane deliberately not — that event also fires after a normal pointerup and would defeat the trailing-click suppression).
…mmits Codex round-W findings, both follow-ons to the round-U clamps: - The controlled Motion Settings Duration input persisted per keystroke, so the writer's latest-keyframe floor clamped parseable intermediate values (typing "10" passes through "1" → floors to 8) and the rerender fought the user's typing. The input now keeps a local draft committed on blur/Enter, with a render-time re-sync when the authored value changes externally (undo, timeline drags); a cleared draft still writes the unset sentinel. - Region-edge drags that resolve to the existing timing (same-frame jitter, or held against a clamp) still reached `onSetTimingField`, burning an undo entry on an identity write; each edge now compares the constrained result with the authored value before committing.
Codex round-X follow-on: a focus-then-blur without editing (or Enter followed by its own blur) re-committed the same duration, and every write pushes an undo snapshot — empty undo steps, and an Enter-committed change appearing to need two Undos. `commitDurationDraft` now compares against the authored value (the clear path likewise only writes when something was authored).
…sible Codex round-Y finding: the one-shot scroll-sync effect found both refs unset when the strip first rendered hidden (still menu, no tracks) and never reran, so switching that menu to Motion left the scrubber unbound and out of alignment on horizontal scroll. The effect now reruns on `visible`.
Codex round-Z follow-on: with the authored duration already at the latest-keyframe floor, entering a smaller value wrote a clamped-back identity (empty undo step) and — since the authored prop never changed — the draft kept displaying the unsaved value. `commitDurationDraft` now mirrors the writer's floor: a value that would clamp back to the authored duration skips the write and resets the draft.
…le; skip clamped opacity identities Codex findings: - The still-menu degrade read `keyframes.first()` directly, disagreeing with the evaluator's explicit duplicate-timestamp rule (later-in-authoring-order wins at that instant) — so the load-time keep-last normalisation could change a hand-edited still menu's compiled overlay. The degrade now evaluates through `evaluate_track` at the first timestamp, so every consumer resolves a shared first timestamp identically and normalisation is behaviour-preserving. - Typing an out-of-range opacity when the saved value already sits at the bound clamped back to the same value but still persisted, burning an undo entry; clamped identities are now skipped.
Codex follow-on: a still menu with imported animation tracks shows the timeline, but its background renders an image and never registers a video element — the playhead loop woke the main thread every frame doing nothing. The hook now requires both visibility and a motion background.
Final codex round: motion mode alone still scheduled the no-op loop for a menu with no background assigned or a video-less asset. The gate is now an actual registered video element (which also covers the still-menu and hidden-strip cases).
ScottMorris
force-pushed
the
menu/08-timeline-ui
branch
from
August 20, 2026 14:04
1a3469e to
0aadfa2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User-facing changes
Implementation notes
requestAnimationFrameagainst the shared<video>element with a transient store subscription and direct style transforms — no per-frame React re-renders; preview sampling is isolated to a per-button leaf component.updateMenuDocument, so undo/redo is free.Closes #108 (together with #125, the model half).
Follow-up candidates
disc.standard(25 for PAL).Test plan
pnpm validatein the dev container: format, 322 vitest, tsc + vite build,cargo fmt --check, clippy-D warnings, 313 nextestStack
Part of the rich menu editor stack — merge bottom → top: