fix(timeline): size pills and clips in px, not in fractions of the timeline - #233
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe timeline now uses pixel-based geometry for zoom-sensitive pill controls, snapping, ruler calculations, and resize limits. Clip cards use absolute duration-based positioning with gutter subtraction. Regression tests cover pill and clip geometry across zoom levels. ChangesTimeline geometry
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant V4Timeline
participant pillAffordance
participant TimelineDOM
User->>V4Timeline: change zoom
V4Timeline->>V4Timeline: calculate pxPerSec
V4Timeline->>pillAffordance: evaluate duration and pxPerSec
pillAffordance-->>V4Timeline: return compact and roomForLabel
V4Timeline->>TimelineDOM: render pill and clip geometry
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/ai-edition/v4/V4Timeline.geometry.test.tsx (1)
87-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
as anyon the mockedtlprop adds a newanytype in a.tsxfile.The coding guidelines require TypeScript strict mode and forbid new
anytypes in.tsxfiles. Thebiome-ignorecomment suppresses the linter but does not satisfy the guideline. Sincetlonly needs a handful of properties for this suite, a narrowerPartial<TimelineApi>cast (or an explicit test-only type picking the used members) would type-check the mock withoutany.As per coding guidelines, "Use TypeScript strict mode and do not add new
anytypes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ai-edition/v4/V4Timeline.geometry.test.tsx` around lines 87 - 98, Replace the `tl as any` cast in the `V4Timeline` test render with a narrower type-safe cast, such as `Partial<TimelineApi>` or a test-only type containing the members used by the suite. Remove the corresponding `biome-ignore` comment and keep the mocked timeline behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ai-edition/v4/EditorShellV4.module.css`:
- Around line 1360-1380: Update the color keyword in the .lanePillCompact:hover
.lanePillHandle rule to the lowercase spelling required by Stylelint’s
value-keyword-case rule, preserving the existing color-mix behavior.
In `@technical-documentation/architecture/editor-shell.md`:
- Around line 136-148: Update the V4Timeline checklist references to match the
current symbols and ranges: use the pill computation around interface LanePill
at line 324, the tlLane render rows beginning around line 1469, and the kind
union around line 203. Preserve the checklist content while replacing the stale
pre-PR line ranges with current machine-usable references.
---
Nitpick comments:
In `@src/components/ai-edition/v4/V4Timeline.geometry.test.tsx`:
- Around line 87-98: Replace the `tl as any` cast in the `V4Timeline` test
render with a narrower type-safe cast, such as `Partial<TimelineApi>` or a
test-only type containing the members used by the suite. Remove the
corresponding `biome-ignore` comment and keep the mocked timeline behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89a34d68-a05f-444f-9b6b-50b9ffbb4405
📒 Files selected for processing (4)
src/components/ai-edition/v4/EditorShellV4.module.csssrc/components/ai-edition/v4/V4Timeline.geometry.test.tsxsrc/components/ai-edition/v4/V4Timeline.tsxtechnical-documentation/architecture/editor-shell.md
… refs Two findings from the automated review on aa8d3bb: - 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-PR line numbers in V4Timeline.tsx, and this branch moved them by ~130 lines. Recomputed against the current file: :461-509 for the pill call site, :1478-1488 for the lane render block, :332 for the `kind` union. (They were already drifting before this branch — 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.
Collaborator
Author
|
Thanks — two of the three applied in 327e06f, replies in-thread for the other two. Nitpick on the
tl={tl as unknown as ReturnType<typeof useTimeline>}The prop keeps its real type at the call site, the suppression is gone, and the file is clean under Summary
CI green on |
16 tasks
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.
Summary
Reported on a 30-minute recording: agent-placed pills render much 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))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
They follow from the width instead of fighting it, with 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), since how short a region may be is a data question and how precisely you can aim at one is the zoom's business.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 — the numbers below are what the DOM reports.
Zoomed all the way out (canvas 1496 px):
Zoomed 30× (canvas 44 820 px): pills span 5 → 80 px and keep their true durations; 2 of them now have room for a label. The old floor would have drawn 672 px here — a 4-second trim covering half the screen, and 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.Math.max(1.5, …), the outside handles,total * 0.012, the 0.2 s floor,flex) turns the matching test red — e.g.expected 1.5 to be close to 0.0555…,expected ['', '', ''] to deeply equal ['0%', '33.33…%', '50%'].vitest run src/components/ai-edition/v4/→ 19/19.tsc --noEmit,biome check,check-docsclean.Note: jsdom's CSS parser silently drops
max(1px, calc(…)), which is why the clip width floor lives in CSS asmin-widthrather than inline.Summary by CodeRabbit
Bug Fixes
Tests
Documentation