fix(timeline): size pills and clips in px, not in fractions of the timeline - #234
Merged
Merged
Conversation
…meline Reported on a 30-minute recording: agent-placed pills render far wider than the effect they stand for, and the mismatch changes with the zoom. Three screen-space rules were written as a fraction of `total`, which is a DURATION in disguise — it scales with the recording, so what looked right on a one-minute clip was nonsense on a podcast: - `Math.max(1.5, pctOf(dur))` floored every pill at 1.5% of the timeline: 27 s on a 30-minute project, 58 s on the 65-minute one this was measured against, at every zoom level. Touching pills merged into one block and no pill could be read as a duration. - `total * 0.012` as the drag snap radius was a 21-second magnet, so an edge jumped to a clip boundary it was nowhere near — the more so the longer the recording, and the worse the further you zoomed in to place it precisely. - `.tlClips`'s flex `gap: 6px` was a fixed pixel amount inserted into a proportional layout: each junction pushed what followed 6px right while every clip shrank to pay for it, so a clip's left edge missed its own start time (+2px on clip 2, +6px on clip 3 of a three-clip timeline) while the pills and ruler above it sat at the true position. Constant in px means it was worth 5 s and 15 s of a 30-minute recording zoomed out, and a fraction of a second zoomed in — that changing ratio is what reads as "the pills move when I zoom". Everything on the canvas is now positioned by `pctOf` (clips included, now absolutely positioned instead of flex), and everything that must be a fixed SCREEN size goes through `pxPerSec`: `pillAffordance`, `PILL_SNAP_PX`, `CLIP_GUTTER_PX`. The only floor left on a width is 1px, in CSS. Handles follow from the width rather than fighting it. Above 18px (two 6px handles + a grabbable body) they sit inside the pill as before; below it they mount outside, with the gap on each side belonging to the pill's own hit strip — so a 1px pill still offers ~9px to move and 6px per side to resize, at every zoom. Nothing becomes unreachable at any size. The chrome re-flows mid-drag without disturbing the gesture: deltas come from the pointer and the listeners live on `window`. The flat 0.2 s minimum region is gone too (it refused the last fifth of a second however far you zoomed in); the floor is now the storage grid, 1 ms, since how SHORT a region may be is a data question and how PRECISELY you can aim at one is the zoom's business. Measured in the app on the reported project (65 min, 15 agent-placed trims): zoomed out each trim was drawn at 22.4px (58.5 s) and now sits at its true 0.45–6.94 s; zoomed 30x the old floor would have been 672px. Clip 2 now starts at exactly 50% of the canvas instead of +3px. Every test in V4Timeline.geometry.test.tsx was ablated — reverting each constant turns the matching one red.
…tant Review finding on the commit before it. `.lanePillCompact` turns overflow visible — it has to, its handles hang outside the box — so a compact pill that rendered a label would spill it across the lane with nothing to clip it. That can only happen if PILL_CONTENT_MIN_PX drops below PILL_HANDLES_MIN_PX, which nothing enforced: two independent numbers with an invisible dependency between them. `roomForLabel` now derives from `compact` itself, so the guarantee holds whatever those numbers become. Also spells out why the snap radius is 0 while the panel is still unmeasured.
… refs
Two findings from the automated review on the main-line version of this change:
- the mocked `tl` was cast to `any` behind a biome-ignore, which AGENTS.md rules
out ("don't add new `any`"). Cast through `unknown` to the real
`ReturnType<typeof useTimeline>` instead: the prop keeps its type and the
suppression goes away.
- the "add a region kind" checklist pointed at pre-change line numbers in
V4Timeline.tsx, and this work moved them by ~130 lines. Recomputed against
this branch: :463-511 for the pill call site, :1504-1512 for the lane render
block, :334 for the `kind` union. (They were already drifting beforehand —
check-docs does not verify line numbers.)
The third finding, `currentColor` → `currentcolor` for stylelint's
value-keyword-case, does not apply here: the repo has no stylelint at all (CI's
Lint job is `biome check`, green on both spellings), and the convention in
website/src/css/custom.css is `currentColor`. Changing it would leave the only
lowercase spelling in the codebase.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Same change as #233, retargeted onto
release/v1.8.0— #233 went tomainby mistake.The three commits cherry-pick onto this branch without conflict, and were re-verified here rather than assumed:
tsc --noEmitclean,vitest run src/components/ai-edition/v4/19/19,check-docsOK. The only edit versus themainversion is the checklist's line references, recomputed against this branch'sV4Timeline.tsx(:463-511,:1504-1512,:334instead of:461-509,:1478-1488,:332) — the waveform work on this branch shifts them by +2 and +26.Summary
Three screen-space rules were written as a fraction of
total, which is a duration in disguise — it scales with the recording, so what looked right on a one-minute clip was nonsense on a podcast:Math.max(1.5, pctOf(dur))total * 0.012gap: 6pxEverything on the timeline canvas is now positioned by
pctOf— clips included, absolutely positioned instead of flex — and everything that must be a fixed screen size goes throughpxPerSec:pillAffordance,PILL_SNAP_PX,CLIP_GUTTER_PX. The only floor left on a width ismin-width: 1px, in CSS.Handles
One threshold, at 18 px (two 6 px handles + a grabbable body):
A 1 px pill still offers ~9 px to move and 6 px per side to resize, at every zoom — nothing becomes unreachable at any size. The chrome re-flows mid-drag without disturbing the gesture (deltas come from the pointer; the listeners live on
window). The flat 0.2 s minimum region is gone too — it refused the last fifth of a second however far you zoomed in; the floor is now the storage grid (1 ms).Related issue
n/a — reported directly.
Type of change
Release impact
Desktop impact
Screenshots / video
Measured in the running app on the reported project (65 min, 2 clips, 15 agent-placed trims) rather than eyeballed.
Zoomed all the way out (canvas 1496 px):
Zoomed 30× (canvas 44 820 px): pills span 5 → 80 px and keep their true durations. The old floor would have drawn 672 px here — a 4-second trim covering half the screen, all 15 merging into one red band.
Testing
src/components/ai-edition/v4/V4Timeline.geometry.test.tsx(new, 5 tests): pill width = its duration at two zoom levels; handles reachable below their own width; grow / shrink / snap through a real drag; clip left edges anchored to their start time and agreeing with the pill above them; the gutter coming off each clip's own width.tscandcheck-docsclean.mainversion of the change.