feat(studio): header logo, playbar cleanup, and I/O work-area markers

[miguel-heygen]

Summary

• Header logo: Hyperframes icon mark (gradient </> chevrons from logo-light.svg) added to the left of the project name in the studio header
• Playbar cleanup: m:ss toggle and frame input removed from the visible controls bar; timecode is now clickable to switch time/frame mode; frame jump lives in the ⌨ panel
• Loop button: replaced text label with a repeat icon (same square footprint)
• Shortcuts panel: ⌨ icon is now click-to-open with three sections — Jump to frame, Work area I/O display (with clear buttons), and a full shortcut reference
• I/O work-area markers (closes (studio) Add I/O markers and "A/E" navigation shortcuts to the Studio timeline #807):
  • I / ⇧I: set / clear in-point at current playhead
  • O / ⇧O: set / clear out-point at current playhead
  • A: jump to in-point (or composition start if unset)
  • E: jump to out-point (or composition end if unset)
  • Forward and backward loop both respect the in/out boundaries when set
  • Seek bar renders a teal work-area band and tick markers at in/out positions

Test plan

• Header shows gradient </> icon left of project name
• Clicking the timecode toggles between time and frame display
• ⌨ panel opens on click; frame jump, work-area rows, and shortcut list all render
• I sets in-point; seek bar shows teal band from in-point; ⇧I clears it
• O sets out-point; seek bar shows teal band to out-point; ⇧O clears it
• A jumps to in-point (or 0); E jumps to out-point (or duration)
• Loop with I/O set: forward playback loops back to in-point at out-point
• Loop with I/O set: backward (J) playback loops back to out-point at in-point
• Clear buttons in ⌨ panel remove in/out markers
• Loop icon button toggles loop; tooltip reads "Loop playback"

[jrusso1020]

APPROVE at c0c3b8de. Clean UX refinement with proper power-user keyboard discipline. All 26 required checks green at review time, screenshots confirm the new panel layout is clear and well-hierarchized (label chips on the left, semantic groupings: Jump → Work Area → Playback).

What's well-built

Three-grep discipline passes (per my refactor-review memory):

• New addEventListener('mousedown', ...) for outside-click on the shortcuts panel — properly scoped (only attached when showShortcuts is true), proper cleanup in useEffect return. ✓
• No new setTimeout / setInterval. ✓
• PlayerControls was already memoized; no new component boundaries to wrap. ✓

Loop integration with I/O boundaries (useTimelinePlayer.ts:185-200, :272-300) is the right shape:

• Forward tick: time >= loopEnd instead of >= dur; on loop, seek to loopStart instead of 0
• Reverse tick: nextTime <= loopStart instead of <= 0; on loop, jump to loopEnd
• Both read from usePlayerStore.getState().inPoint/outPoint at tick time (not closed over at effect setup) — picks up live edits without re-mounting the RAF loop

State management in playerStore.ts:

• setInPoint/setOutPoint are guarded with Number.isFinite(time) — defensive against NaN/Infinity from upstream
• reset() clears both in/out — verified the new fields are in the reset spread
• Setters accept null for clearing, matching the keyboard handlers' Shift+I/Shift+O paths

Keyboard handler discipline (usePlaybackKeyboard.ts:131-156):

• All new key handlers (I/O/A/E) call e.preventDefault() to suppress browser defaults
• A and E read inPoint/outPoint from usePlayerStore.getState() (live state, not stale closure)
• Defaults thread sensibly: A → inPoint ?? 0, E → outPoint ?? duration
• Deps array correctly updated to include getAdapter, seek

Time-display click-to-toggle is the right consolidation — the prior m:ss button and the separate frame input were both demoting the timecode itself to passive text. Now the timecode IS the toggle, freeing the button row for higher-frequency actions (loop, shortcuts). The disabled:pointer-events-none + hover:opacity-80 + title are all the right affordances.

HyperframesIcon inline SVG in StudioHeader.tsx: ~50 lines of inline JSX for the two-chevron gradient logo. Fine choice over <img src=logo-light.svg> — gives full control over the size prop and avoids a network request for a 1KB asset. Two named <linearGradient> defs with distinct ids (hf-icon-g0, hf-icon-g1) — no SSR/StrictMode collision risk if HyperframesIcon ever renders twice on the same page.

One real edge case worth handling — inPoint > outPoint

The store's setInPoint/setOutPoint don't validate ordering against the sibling value. User flow that's plausible:

1. User sets out-point at 5s (O at playhead=5s) — outPoint=5
2. User scrubs to 8s, sets in-point (I at playhead=8s) — inPoint=8
3. Now inPoint=8 > outPoint=5

useTimelinePlayer.ts:189-191 then computes:

const loopEnd = outPoint !== null ? outPoint : dur;   // → 5
const loopStart = inPoint !== null ? inPoint : 0;     // → 8
if (time >= loopEnd && !adapter.isPlaying()) { ... seek(loopStart) ... }

With loopStart > loopEnd, the forward-loop condition fires at any time >= 5, seeks to loopStart=8, plays forward → immediately past loopEnd=5 again → infinite tight-loop. Same shape in reverse direction.

Three ways to close it (any is fine, in increasing complexity):

1. Validate at set-time — setInPoint rejects (or clamps) when time >= outPoint, and symmetrically for setOutPoint. Simplest; might surprise the user who's mid-edit.
2. Validate at tick-time — in the loop guard, treat loopStart >= loopEnd as "no work area defined" and fall through to the full-composition loop. Lets the user set values in any order; the loop only activates when ordering is sane.
3. Auto-swap — if user sets a new value that crosses the sibling, swap them (in becomes out and vice versa). Most forgiving UX but can confuse users who didn't realize they crossed.

My preference: option 2 (validate at tick-time). One conditional in useTimelinePlayer.ts covers both forward and reverse paths, lets users set values in any order, and the visual seek-bar band degrades gracefully (the new <div> with left: in% right: 100-out% would render with negative width and display: none semantics).

Currently the only way to hit the bad state is the manual sequence above — but the shortcuts panel's "Clear" buttons make it discoverable enough that I'd expect at least one user to find it. Worth handling.

Smaller observations (non-blocking)

1. .filesize-allowlist entry for PlayerControls.tsx is new — the +286 lines pushed it over the LOC threshold. Acceptable in this PR; the three new panel sections (Jump, Work area, Shortcuts) could be extracted to sub-components in a follow-up if the file grows further. Per my prior session memories, the allowlist is the right escape hatch.

2. Seek bar markers: the in/out tick markers are 2px wide × 10px tall, centered on the playhead row. Visually distinct from the 12px round playhead thumb (z-index 4 for thumb, z-index 3 for markers — correct layering so the playhead never disappears behind a marker). Nice z-order discipline.

3. Outside-click handler scoping: uses document.addEventListener('mousedown', ...). Correctly bound only when showShortcuts is true (effect deps [showShortcuts]). One alternative pattern is pointerdown for touch-device parity, but mousedown matches the rest of the codebase (the speedMenuContainerRef outside-click also uses mousedown). Consistent. ✓

4. SHORTCUT_SECTIONS as const: type-narrowed, immutable, good. The two sections (Playback + Work area) cover the keyboard surface. The Work area section's labels match the docstrings in usePlaybackKeyboard.ts exactly — same string in both places. Worth a future pass to deduplicate (the hint label is currently the source of truth in the UI but the keyboard handler doesn't reference it; if they drift, the panel could lie about what the keys do).

5. Loop button width transition (w-14 → w-7): the icon button is now square. Verified the icon-only design has aria-label for screen readers ("Disable loop playback" / "Enable loop playback") and aria-pressed={loopEnabled} for state announcement. Good a11y.

6. Test plan checkboxes all unchecked (10 of 10). For a UI feature, manual visual verification is the canonical proof — worth a walkthrough before merge, especially the loop-with-I/O paths since that's the trickiest behavior to verify by code review alone. Vai may want to do the visual pass since the screenshots in the Slack post only show the static panel state.

CI

All required checks green: Lint, Format, File size check, Build, Test, Typecheck, Test: runtime contract, CLI smoke (required), regression, preview-regression, Preview parity, player-perf, CodeQL, Analyze (×3), Smoke: global install. Tests on windows + Render on windows still in_progress, no failures.

Verdict

APPROVE. Worth handling the inPoint > outPoint ordering edge case before shipping (option 2 is the cleanest); other observations are follow-up housekeeping. UI design per screenshots is well-executed.

— Rames Jusso (pr-review)

[vanceingalls]

Studio UI cleanup PR — header logo, playbar simplification, and I/O work-area markers (closes #807). Clean diff (~387 LOC, 6 files, mostly localized), CI green on all required checks (Windows tests pending — typical), and the work-area model is a welcome power-user addition.

Strengths

• PlayerControls.tsx:50,56-66 — already memo-wrapped with per-field Zustand selectors (inPoint/outPoint added on the same pattern). Player-tick subscribers are exactly the seam where a sloppy selector cascades re-renders; this stays clean.
• usePlaybackKeyboard.ts:133-155 — keyboard shortcut routing reuses shouldIgnorePlaybackShortcutEvent (input/contenteditable/Cmd/Ctrl/Alt filter), so the new A/E/I/O keys don't collide with Cmd+A select-all or fire while focus is in the frame-jump input.
• PlayerControls.tsx:140-152 — the new shortcuts-panel outside-click useEffect correctly returns removeEventListener, gated on showShortcuts so the listener doesn't sit idle when the panel is closed.

Findings

blocker — playerStore.ts:113-114: setInPoint / setOutPoint accept any finite number with no relational invariant. Workflow that breaks it: user hits I at t=5s, scrubs to t=2s, hits O → state is {inPoint: 5, outPoint: 2}. Now in useTimelinePlayer.ts:188-198 (forward RAF loop body) the check time >= loopEnd evaluates 2 >= 2 → seek to loopStart=5 → next tick 5 >= 2 again → infinite tight RAF loop with setIsPlaying(true) re-armed every frame. Same shape in the reverse loop at useTimelinePlayer.ts:275-291. Failure mode: pegged CPU + frozen player UI on a sequence any non-expert can produce.
Fix: enforce the invariant at the setter — setInPoint(t) => clamp t to [0, outPoint ?? Infinity], setOutPoint(t) => clamp to [inPoint ?? 0, duration]. Bonus: also guard against inPoint < 0 / outPoint > duration (current code is Number.isFinite only, so setInPoint(-100) is accepted).

important — useTimelinePlayer.ts:191: non-loop forward playback silently ignores outPoint. Exit branch is time >= loopEnd && !adapter.isPlaying() — but the underlying engine only auto-pauses at dur, not at outPoint. So with work-area set + loop OFF, playback sails past the out-marker straight to the composition end. The PR description ("Forward and backward loop both respect the in/out boundaries when set") implies this is intentional, but it diverges from Premiere / AE / Resolve convention where the work-area out-point bounds all playback. Either pause at outPoint in non-loop mode too, or be explicit in UX/docs that the markers are loop-only.

important — useTimelinePlayer.ts:246-248: play() end-of-stream reset is still hardcoded to adapter.seek(0). With inPoint set and the playhead at the end, pressing Space restarts from 0 instead of inPoint — breaks the "work area = my edit region" mental model. Should be adapter.seek(usePlayerStore.getState().inPoint ?? 0).

nit: usePlaybackKeyboard.ts:133-155 — four new shortcut branches with no tests. JKL/Space precedent has none either, so not a merge blocker, but the work-area boundary math is the kind of thing that quietly regresses on the next refactor. A handful of unit tests around setInPoint / setOutPoint invariants + the loop-boundary branches would pay for themselves.

nit: .filesize-allowlist:3 adds PlayerControls.tsx (now 677 LOC). The file already mixes seek bar + drag + speed menu + time display + shortcut panel. Extracting the shortcuts panel (lines 478-651 of the new file) into its own component would unwind the allowlist entry and tighten the memo boundary.

Verdict: REQUEST CHANGES
Reasoning: The infinite-loop case on inPoint > outPoint is a real, user-reachable freeze and worth fixing before merge; the other items are smaller follow-ups but the play()-at-end and non-loop-outPoint behavior are user-facing semantics that should be settled now while the shape of the feature is in flight.

— Vai

[jrusso1020]

Updating my verdict — REQUEST_CHANGES, additive to Vai's review.

She caught two real correctness gaps I missed:

Gap 1 — non-loop forward playback ignores outPoint

useTimelinePlayer.ts:191 only consults loopEnd = outPoint ?? dur inside the if (time >= loopEnd && !adapter.isPlaying()) branch — the loop-handler. The forward-play tick path itself has no early-pause at the out-marker. So when loop is OFF and the user has set an out-point, they expect the playhead to stop at out-point; today it sails past it to dur.

This breaks the "I/O work area defines a sub-range" mental model the PR is trying to establish. The seek-bar's teal work-area band promises a bounded range; play-through should honor that bound regardless of loop state.

Gap 2 — play() end-of-stream reset hardcodes seek(0) instead of seek(inPoint ?? 0)

At useTimelinePlayer.ts:246, when the playhead is at end-of-stream and the user clicks play, the code resets to absolute zero instead of inPoint ?? 0. Inconsistent with the rest of the work-area model — if inPoint is set, hitting play after end should jump to the in-point, not to the start of the composition.

Why I missed these

I checked the loop tick paths (forward + reverse, both correctly updated to use loopStart/loopEnd) and stopped there. Should have audited every other site that operates on the timeline range:

• Loop tick (covered — my prior review)
• Non-loop forward tick (Vai's gap 1 — should pause at outPoint)
• Non-loop reverse tick (symmetric — should pause at inPoint?)
• play() end-of-stream reset (Vai's gap 2 — should reset to inPoint, not 0)
• Manual seek (probably fine — user-initiated jump can land anywhere; doesn't need to be clamped to work area)
• Step-frame (probably fine, same reasoning)

Vai's two gaps round out the "every entry path that traverses the timeline" audit. This is the same Rule-2 corollary I keep hitting — generalizing from "audit recovery paths" to "audit every site that should honor a new state-machine contract." Adding to my lessons.

Severity calibration on the inPoint > outPoint edge case

Vai and I converged on the diagnosis; she leans pre-merge fixable, I leaned non-blocking. She's right that a one-line setter validation is cheaper than the tick-time conditional I suggested, and prevents the bad state from existing rather than tolerating it. Re-calibrating toward her shape — fix at the setter:

setInPoint: (time) => set((state) => {
  if (time !== null && state.outPoint !== null && time >= state.outPoint) return state; // reject ordering violation
  return { inPoint: time !== null && Number.isFinite(time) ? time : null };
}),

(Symmetric for setOutPoint.) Easier-to-reason-about contract than the tick-time fallback.

Verdict shape

REQUEST_CHANGES, three items:

1. Non-loop forward tick should pause playback at outPoint when set
2. play() end-of-stream reset should seek(inPoint ?? 0) instead of seek(0)
3. setInPoint/setOutPoint setters should reject ordering violations (per Vai's framing — at the setter, not at tick-time)

All three are local fixes (~5-10 lines each) closing the work-area contract end-to-end. Once they land, the feature does exactly what the seek-bar visual + shortcut panel promise. Will re-stamp on the fix-up.

The visual UI work in the screenshots, the keyboard handler dispatch, the seek-bar markers/band, and the loop-tick integration are all still well-built — the work-area state-machine just needs to extend to non-loop playback paths.

— Rames Jusso (pr-review)

Note to my future self: when a PR adds a new state-machine contract (work area, undo stack, focus mode, any "new dimension on the timeline / canvas"), audit every other path that operates on the dimension the contract constrains — not just the path the diff modifies. Rule-2 generalization beyond the recovery-code case in feedback_audit_every_entry_path_to_failure_state.md.

[O1Anas]

great PR tbh, hopefully a much cleaner more mature UI/UX

[miguel-heygen]

great PR tbh, hopefully a much cleaner more mature UI/UX

​pretty clean!

[jrusso1020]

Re-approved at e6de8a08. All three items closed correctly + a bonus defensive guard at the tick layer.

Item-by-item verification

1. Non-loop forward tick now pauses at outPoint (Vai's gap 1)

useTimelinePlayer.ts:188-202:

• Removed the && !adapter.isPlaying() guard from the loop-end-detection branch
• Added explicit if (adapter.isPlaying()) adapter.pause(); in the non-loop path

So time >= loopEnd now fires on every forward tick regardless of isPlaying() state, and the non-loop case actively pauses the adapter. Forward play-through now honors the out-marker as the seek-bar's teal band promises. ✓

2. play() end-of-stream reset uses seek(inPoint ?? 0) (Vai's gap 2)

useTimelinePlayer.ts:250:

adapter.seek(usePlayerStore.getState().inPoint ?? 0);

Reads live state from store, falls back to 0. ✓

3. inPoint > outPoint ordering — clear-the-conflicting-sibling shape (my finding)

playerStore.ts:117-133 (both setters):

• setInPoint(t) where t >= state.outPoint → clear outPoint to null
• setOutPoint(t) where t <= state.inPoint → clear inPoint to null

Different from my "reject the ordering violation" suggestion AND from Vai's "validate at setter" framing — Miguel chose a third shape: preserve the user's most-recent intent and clear the conflicting sibling. Defensible UX: if I set out=5, then move playhead to 8 and press I, I want the new in-point I just set; the stale out-point that contradicts it should clear. This avoids the "reject silently leaves user confused why I/O didn't take" problem of strict rejection. ✓

Bonus — defensive tick-time guard

Both forward and reverse ticks now have an explicit rawLoopStart < rawLoopEnd check:

const loopEnd = rawLoopStart < rawLoopEnd ? rawLoopEnd : dur;
const loopStart = rawLoopStart < rawLoopEnd ? rawLoopStart : 0;

Belt-and-suspenders: if the setter ever fails to prevent a bad state (theoretically impossible now, but defense-in-depth), the tick falls through to full-composition bounds instead of looping tightly. Good. ✓

Reverse-tick path also updated

Verified useTimelinePlayer.ts:277-291 (the reverse tick function) now reads inPoint/outPoint from the store and applies the same rawLoopStart < rawLoopEnd guard. Reverse playback now honors work-area bounds for both loop and non-loop cases. Symmetry preserved. ✓

Coverage check on every-path audit

Per the lesson I just memoized: I should enumerate every path that operates on the timeline range. Let me re-verify against the new diff:

• ✓ Forward play tick (loop ON) — loopStart / loopEnd honored
• ✓ Forward play tick (loop OFF) — actively pauses at loopEnd
• ✓ Reverse play tick (loop ON) — loopStart / loopEnd honored
• ✓ Reverse play tick (loop OFF) — actively halts at loopStart
• ✓ play() end-of-stream reset — seeks to inPoint ?? 0
• ✗ Manual seek (still allows free seek outside work area) — intentional; user-initiated jump can land anywhere
• ✗ Step-frame (still allows step past boundary) — intentional; granular control
• ✗ Jump-to-frame (still allows jump outside) — intentional; explicit user input

The three "still allows" cases are correctly NOT clamped — explicit user-initiated navigation should be able to land anywhere on the timeline, even outside the work area. Only auto-advancing playback paths need to honor the bounds. That's the right scoping.

CI

Comprehensively green on the new SHA so far: Lint ✓, Format ✓, Test ✓, Typecheck ✓, Build ✓, Test: runtime contract ✓, Preview parity ✓, preview-regression ✓, Perf: scrub ✓, Perf: load ✓, CodeQL ✓, File size check ✓, Semantic PR title ✓, CLI smoke in_progress (trending), windows jobs + regression-shards still running, no failures.

Verdict

APPROVE. The work-area state-machine is now contract-complete across every auto-advancing playback path. Defensive tick-time guard catches any future setter bugs. Ship 🚀

— Rames Jusso (pr-review)

[vanceingalls]

APPROVE at e6de8a08. All three findings from my prior REQUEST_CHANGES are addressed cleanly. Two follow-up commits (c7eefdfa defensive tick-time fallback, e6de8a08 setter cross-clear + non-loop outPoint pause + play() seek to inPoint) close the work-area contract end-to-end.

Prior findings — status

blocker — setter invariant — ADDRESSED at playerStore.ts:116-132: setInPoint(t) now cross-clears outPoint when t >= state.outPoint; setOutPoint(t) symmetrically clears inPoint when t <= state.inPoint. The bad state inPoint >= outPoint is unreachable through these setters. I'd originally suggested clamp/reject; Rames suggested reject; Miguel went with cross-clear (set the new one, drop the conflicting sibling). Cross-clear is actually the most forgiving UX of the three — the user's most-recent action wins, no silent rejection of a keystroke they just made, and the inverted state never materializes. Good call.

important — non-loop forward outPoint — ADDRESSED at useTimelinePlayer.ts:191-201: forward tick no longer gates on !adapter.isPlaying() — the time >= loopEnd branch fires while playback is running and now explicitly calls adapter.pause() on the non-loop path. With work-area set + loop OFF, playback pauses at outPoint instead of sailing to dur. Matches Premiere/AE/Resolve work-area semantics.

important — play() end-of-stream reset — ADDRESSED at useTimelinePlayer.ts:250: now adapter.seek(usePlayerStore.getState().inPoint ?? 0) instead of seek(0). End-of-stream → Space resumes from in-point, consistent with the work-area mental model.

Rule-2 audit-all-sites — clean

Walked every entry path that operates on the work-area dimension:

• Setters (setInPoint / setOutPoint): cross-clear, no inversion possible ✓
• Forward RAF tick: pauses at loopEnd regardless of loop state, defensive fallback if inverted ✓
• Reverse RAF tick (useTimelinePlayer.ts:275-302): symmetric — pauses at loopStart (i.e. inPoint ?? 0) when loop is off. Reverse playback now respects in-point too, which is the symmetric correctness fix the forward path needed.
• play() end-of-stream reset: seeks to inPoint ?? 0 ✓
• reset() (composition switch): clears both points ✓ (already correct in prior review)
• Keyboard I/O/A/E: all route through the setters / read live store state ✓
• No other call sites to setInPoint/setOutPoint in the repo

The defensive tick-time fallback (rawLoopStart < rawLoopEnd ? rawLoopStart : 0) at useTimelinePlayer.ts:194-195, 279-280 is belt-and-suspenders — even if some future code path mutated the store directly and bypassed the setter, the RAF loop won't infinite-tight-loop. Good defense in depth.

Net-new observations (non-blocking)

1. Defensive fallback uses dur/0 instead of "no work area": when rawLoopStart >= rawLoopEnd the tick falls back to full-composition range. Could alternatively be treated as "work area is invalid → no work area at all". Functionally equivalent since the setter prevents inversion; fine as-is.

2. PlayerControls.tsx now 677 LOC with the allowlist entry. Still worth extracting the shortcuts panel (the ~170 lines of the bottom popover JSX) into its own component in a follow-up to retire the allowlist entry. Not a blocker for this PR.

3. Still no unit tests for the setter cross-clear or the loop-boundary math. The behavior is subtle enough that a regression would be quiet — a handful of tests on setInPoint/setOutPoint invariants + the forward/reverse loopEnd/loopStart computation would pay for themselves. Follow-up, not a blocker.

CI

Required checks green at review time: Lint, Format, File size check, Build, Test, Typecheck, Test: runtime contract, regression (still finishing shards but none failed), preview-regression, Preview parity, player-perf (all four perf jobs), CodeQL, Analyze (×3). Windows tests + CLI smoke still in_progress, no failures.

Verdict

APPROVE.

Reasoning

All three prior findings addressed at the right seam (setter for the blocker, tick path for forward outPoint, play() entry path for the end-of-stream reset). Cross-clear semantic on the setters is actually a small UX improvement over what I'd originally suggested. Rule-2 audit clean — every entry path that operates on the work-area contract honors it now. CI green, no new findings rise to blocker.

— Vai