feat(studio): timeline UI overhaul — flat clips, thumbnails, visual cleanup#994
Conversation
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Read all 7 files at b6862eda. Verdict is would-be APPROVE but holding the stamp per merge policy (Miguel not on the trusted-stamper list — same gate as recent pacific PRs). Direction is right; the diff is a net deletion of 52 lines that genuinely simplifies the visual contract.
Strong points
- Flat unified theme —
UNIFIED_STYLEreplacing the 9-tag-keyedTRACK_STYLESmap is the right call. Per-tag teal-gradient styling never communicated semantic difference; collapsing to one accent + label color reduces the visual-noise floor without losing any signal. The 3px left accent stripe at constant 0.3/0.7 opacity is the new scanability mechanism — works. - Conditional rendering for trim handles (
{showHandles && capabilities.canTrimStart && (...)}instead of opacity-toggle) — fewer always-mounted DOM nodes, cleaner unmount when capabilities forbid trimming. Removes a class of "ghost handle catches a pointer event by 0.01 opacity" bugs. mix-blend-mode: lighten+#1c2028thumbnail body bg — the right fix for transparent caption-overlay compositions disappearing into a pitch-black underlay. Symmetric application: same color instudioServer.tsthumbnail-server andvite.browser.tsgenerator. Good cross-file consistency.compSrcURL normalization inuseRenderClipContent.ts— real correctness fix. The "absolute URL got nested into preview path" 404 mode is exactly what the regex-against-previewPrefixslice catches. Defensivetry/catchfalls back to the original string on URL parse failure.
Substantive concerns (all non-blocking)
1. Light-background composition contrast with the new blend
mix-blend-mode: lighten against the #1c2028 thumbnail underlay assumes underlying composition content is brighter than the underlay (the comment in the PR body confirms this — "dark backgrounds blend away, bright content shows through"). For compositions authored on a light canvas (cream paper, white background, editorial-style), the underlying content can be darker than #1c2028 in some regions (e.g., black body text on cream paper) — those regions would blend to the cream, not stay readable.
Worth a manual check before merge on:
- A composition with a bright/cream background + dark text (editorial-style template)
- A composition where the visible content is dark-on-light (e.g., a SaaS pricing card)
If the thumbnail of those looks washed-out or low-contrast, the blend-mode choice may need to be normal with a per-composition decision, or darken for light-canvas compositions. Probably fine for the captions/chrome overlay shapes that motivated this fix, but worth confirming.
2. Dead-code residue from the per-tag → unified collapse
createTrackStyle(_tag: string) now ignores its parameter (returns UNIFIED_STYLE regardless). The function exists as a wrapper around a singleton. getTimelineTrackStyle still has the h1/h2/h3...h6 collapse logic that funnels to createTrackStyle("h1") — but createTrackStyle("h1") and createTrackStyle("div") return the same object. Two cleanup paths:
- Drop
createTrackStyleentirely;getTimelineTrackStylereturnsUNIFIED_STYLEdirectly. ~6 lines removed. - Retain the per-tag dispatch as a placeholder for future per-tag theming. If keeping, add a code comment noting the intentional stub so the next reviewer doesn't simplify it.
Same issue with _accentColor underscore-prefixed prop in CompositionThumbnail — the underscore signals "unused" but callers still pass the prop, and the public interface still accepts it. Drop the prop from CompositionThumbnailProps or keep with a note. Minor.
3. compSrc URL normalization — small but worth pinning by test
The new logic strips /api/projects/<pid>/preview/ from the absolute URL to get the relative path. This is a real bug fix and is the kind of regex-shaped string manipulation that's easy to break in 6 months when someone changes the API path. A 5-line unit test (one positive case: absolute URL with the preview prefix → relative; one negative case: absolute URL with a different prefix → falls through unchanged) would lock the contract. Not a blocker.
4. Bug-fix-in-UI-PR scope mixing
The "Fixed thumbnail URLs" bullet in the PR body is a correctness fix; the rest is presentation. They make sense as one PR (the URL fix changes thumbnail load behavior, which the UI change relies on for the mix-blend-mode story to be visible). But the title says "timeline UI overhaul" and won't surface "fixed 404s on composition thumbnail URLs" to someone doing a future git log --grep. Cosmetic; if you want to retitle, something like "feat(studio): flat-clip timeline UI + fix nested compositionSrc 404s" lands both.
Preview-vs-render parity check
Per the prior memory pattern (hf#965 / #978 / #981 / #986), I checked whether anything touches the render path. Answer: no. The changes are studio-only — studioServer.ts thumbnail generation, vite.browser.ts generator (also studio-thumbnail-scope), and useRenderClipContent.ts (studio hook). Render and bundler unaffected.
Verdict
Solid small PR. Would-be APPROVE; posting as COMMENT per merge policy. The light-background-contrast question is the one worth a manual check before James greenlights merge; everything else is cosmetic / test-coverage shape.
— Rames Jusso
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Solid visual cleanup PR. The unification of theme into UNIFIED_STYLE (timelineTheme.ts:36-46) is clean — every removed export (TRACK_STYLES, TIMELINE_TEAL, DEFAULT_TRACK_STYLE) has zero orphan consumers in packages/. The compositionSrc URL-normalization in useRenderClipContent.ts:27-36 is defensive — handles both absolute and relative inputs, with the new URL(...) constructor catching parse failures gracefully. studioServer.ts:349-355 transparent-body fallback is correctly scoped via getComputedStyle check, so it won't clobber compositions that intentionally set their own background.
important — dangling animation reference, CompositionThumbnail.tsx:115. The new style={{ animation: "fadeIn 200ms ease-out", ... }} references a @keyframes fadeIn that doesn't exist anywhere in packages/studio/ — no studio.css keyframes, no Tailwind animation config, no global stylesheet. Verified by grep -rn "@keyframes" packages/studio (returns nothing). The animation property is a silent no-op; the thumbnail just snaps in. Fix: add @keyframes fadeIn { from { opacity: 0 } to { opacity: 1 } } to packages/studio/src/styles/studio.css, or use a Tailwind animate-in fade-in utility, or drop the line. Failure mode is cosmetic (intended polish ships dead) but the property is misleading to future readers.
nit — dead accentColor param, CompositionThumbnail.tsx:55. Renamed to _accentColor to silence unused-arg lint, but useRenderClipContent.ts:51, 65, 121 still passes accentColor: style.clip at every call site. The prop is accepted-and-discarded. Either drop it from CompositionThumbnailProps and stop passing at call sites, or actually use it (the left-edge accent stripe in TimelineClip is the natural place, though that's now trackStyle.accent from a different source).
nit — createTrackStyle(_tag) no longer needs the param, timelineTheme.ts:43. With UNIFIED_STYLE as the single return value, getTimelineTrackStyle could just return UNIFIED_STYLE directly. Minor; keeps the indirection if you want to re-introduce per-tag styles later.
note — CI red is npm-registry flake, not a code defect. Both Preview parity and preview-regression failed because bun install --frozen-lockfile couldn't extract @fontsource/noto-sans-jp@5.2.9 from registry.npmjs.org (Fail extracting tarball at 2026-05-20T22:57:05Z in run 26194735075). The parity tests never ran. Re-trigger the workflow and it should go green. Not a code-defect blocker, but worth re-running before merge so the visual-regression gate has actually executed against this UI rewrite.
note — Preview parity is the right gate for this PR specifically. UI overhaul + thumbnail changes + body-background tweak in vite.browser.ts:128-129 and studioServer.ts:349-355 are exactly what that suite is meant to catch. Worth ensuring it produces a real signal on this diff before merging.
Interaction parity audit (drag, trim, select, drill-down, hover): all preserved in TimelineClip.tsx. Trim hit-target shrunk from 18px to 14px wide — borderline-acceptable for col-resize; flagging as a feel-check, not a blocker. Resource-cleanup audit (Rule 6): ResizeObserver in CompositionThumbnail still teardown-correct via useMountEffect + setContainerRef.disconnect(); no new listeners/timers in TimelineClip.
Verdict: APPROVE
Reasoning: No correctness or contract-violation blockers; CI red is npm-registry flake unrelated to the diff. The dangling fadeIn animation is an important cosmetic gap worth a follow-up commit before merge, but doesn't block ship.
Review by Vai
…ng thumbnails Visual redesign of the timeline: - Flat solid clip backgrounds, no gradients or multi-layer shadows - Unified neutral color palette — all clips use the same base color - Clean 6px border-radius instead of organic asymmetric radii - Single-line labels, no redundant tag badge or time range - 3px teal accent stripe on the left edge of every clip - Simplified trim handles (2px accent bars) Thumbnail fixes: - Fixed broken thumbnail URLs — compositionSrc was an absolute URL that got nested inside the preview/comp path. Now normalized to relative path before constructing the thumbnail URL - Thumbnail background changed from #000 to #1c2028 so transparent overlay compositions render visible content against a matching dark background - mix-blend-mode: lighten on thumbnail layer — dark backgrounds blend away, bright content shows through - Removed double-label in CompositionThumbnail (was showing both a badge at top and text at bottom)
…keyframes, compSrc test
Fallow audit reportFound 20 findings. Duplication (13)
Health (7)
Generated by fallow. |
miguel-heygen
force-pushed
the
worktree-feat-timeline-ui
branch
from
25a1314 to
36898f7
Compare
3 participants
Summary
6pxcorners, unified neutral base color (#1c2028)compositionSrcwas an absolute URL that got nested into the preview path, causing 404s. Now normalized to relative path#000to#1c2028so transparent overlay compositions render visible contentmix-blend-mode: lighten— dark backgrounds blend away, bright content shows throughStacked on #986.
Test plan