fix(timeline): carry rounded timecode seconds - #236
Conversation
📝 WalkthroughWalkthroughChangesThe timeline formatters now share rounded time decomposition. The left panel and transport displays use the shared formatting utilities. Regression tests cover rollover, invalid input, and large finite values. Timeline formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lib/ai-edition/timeline/format.ts`:
- Around line 13-20: Update splitRoundedTime to avoid multiplying the full
duration by 10: compute whole minutes and the remaining seconds from safe first,
round only the remainder, and carry one minute when the rounded remainder
reaches 60. Preserve zero handling and return finite totalMinutes and seconds
for all finite inputs, including values near Number.MAX_VALUE.
🪄 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: 9e1208b7-b946-4551-b08a-ab11bf8f19eb
📒 Files selected for processing (2)
src/lib/ai-edition/timeline/format.test.tssrc/lib/ai-edition/timeline/format.ts
89e07a8 to
545043d
Compare
af50e14 to
f2b87c0
Compare
f2b87c0 to
193a728
Compare
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Good catch, and the fix is factored the way I'd have wanted it. splitRoundedTime does the carry once, and — the part that actually matters — formatSeconds now derives hours from totalMinutes instead of re-flooring the raw value. Without that second half formatSeconds(3599.96) would still land on 59:60.0 even with a rounded seconds field. You also stayed inside the boundary the header comment draws and left formatHms/formatTimePadded alone, which is right.
The one thing I want settled before merge is scope. The identical floor-then-round shape lives in two more places this PR doesn't touch, and one of them is the readout users stare at all day:
src/components/ai-edition/TransportBar.tsx:9(formatTC) — feeds the live playhead at line 180.src/components/ai-edition/LeftPanel.tsx:39(formatTimecode).
Details inline. format.ts is only two of the three copies, so merging this alone means the bug you fixed is still visible in the transport bar. I'm happy either way — sweep them into this PR, or land this and open a follow-up. Say which and I'll merge.
format.ts was only two of the three places that floored the minute field off the raw value while rounding the second field off it separately, so `0:60.0` stayed renderable in the two this PR had not reached. TransportBar's `formatTC` was `formatSec` verbatim — same output for every input, junk guard written differently — so it just goes, and the transport bar imports the shared one. That is the copy that mattered: it feeds the live playhead readout, so the bad string showed up there once a minute during playback, which is the place a user is most likely to actually see it. LeftPanel's `formatTimecode` is a genuinely different shape (`h:mm:ss.t`, hours always shown), so it keeps formatting itself rather than being forced into one of format.ts's two shapes. It now shares `splitRoundedTime`, which is what this PR added and the only part that must not be re-derived. Fixed a second bug while there: `padStart(3, "0")` never padded anything, because `(5).toFixed(1)` is already three characters — a single-digit second rendered `0:00:5.0`. Also tightened the two `Number.MAX_VALUE` cases. `not.toMatch(/Infinity|NaN/)` passes against the pre-carry implementation too, so it pinned nothing; asserting the exact string means a regression is actually caught.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ai-edition/LeftPanel.tsx (1)
44-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a colocated regression test for
formatTimecode.
format.test.tsverifiessplitRoundedTime, but it does not verify the component-specifich:mm:ss.tassembly in Line 44-51. A regression in the hour/minute calculation orpadStart(4)could reintroduce0:00:60.0or0:00:5.0while the current tests still pass. Addsrc/components/ai-edition/LeftPanel.test.tsxwith Vitest and jsdom, or moveformatTimecodeintosrc/lib/ai-edition/timeline/format.tsand test it there.🤖 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/LeftPanel.tsx` around lines 44 - 51, Add a colocated Vitest/jsdom regression test for the component-specific formatTimecode function in LeftPanel.tsx, covering hour/minute assembly and single-digit seconds padding so outputs never regress to forms like 0:00:60.0 or 0:00:5.0. Keep the existing splitRoundedTime tests unchanged and verify representative formatted timecodes through the exported or otherwise testable formatTimecode symbol.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.
Nitpick comments:
In `@src/components/ai-edition/LeftPanel.tsx`:
- Around line 44-51: Add a colocated Vitest/jsdom regression test for the
component-specific formatTimecode function in LeftPanel.tsx, covering
hour/minute assembly and single-digit seconds padding so outputs never regress
to forms like 0:00:60.0 or 0:00:5.0. Keep the existing splitRoundedTime tests
unchanged and verify representative formatted timecodes through the exported or
otherwise testable formatTimecode symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c7aa9128-02f7-4bd3-8bf7-5e01534fedc1
📒 Files selected for processing (4)
src/components/ai-edition/LeftPanel.tsxsrc/components/ai-edition/TransportBar.tsxsrc/lib/ai-edition/timeline/format.test.tssrc/lib/ai-edition/timeline/format.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/ai-edition/timeline/format.ts
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Threads cleared and the sweep is in (c3f3dea), CI green. Approving.
The carry factoring was right from the start — deriving hours from totalMinutes rather than re-flooring the raw value is the half that makes formatSeconds(3599.96) land on 1:00:00.0. What this needed was reach: format.ts was two of the three copies, and the one it did not touch feeds the live playhead.
3 participants
Summary
Related issue
No linked issue; found while exercising timeline boundary values.
Type of change
Release impact
Desktop impact
Screenshots / video
Not applicable; unit-level timecode correction.
Testing
Summary by CodeRabbit