fix(cli): preserve snapshot timestamp precision

[miguel-heygen]

Summary

• preserve millisecond-level precision in snapshot --at progress labels and output filenames
• trim floating-point noise so computed timestamps remain readable
• cover explicit sub-tenth timestamps and floating-point artifacts with regression tests

Tests

• bun run --cwd packages/cli test -- src/commands/snapshot.test.ts (26 passed)
• bun run --cwd packages/cli typecheck
• focused oxfmt --check and oxlint
• real CLI repro: snapshot --at 1.12 --describe false now reports 1.12s and writes frame-00-at-1.12s.png

[james-russo-rames-d-jusso]

Reviewed at 4e805f8e. Windows checks in progress; other CI green.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Number(time.toFixed(3)) is the right shape for the stated goal. toFixed(3) caps precision at milliseconds (which the PR body promises); Number(…) then trims trailing zeros so 1.10 → "1.1" reads cleanly. FP-noise cases like 0.30000000000000004 collapse to 0.3 without losing intent. Sub-millisecond precision drops, which is what a "snapshot label" wants — this isn't a scientific timestamp, it's a filename.

🟢 Applied consistently at both call sites — the intra-loop timeLabel at the snapshot-capture site AND the outer label used in the run-summary line. No third call site missed (spot-checked with time\.toFixed).

🟢 formatSnapshotTimestamp is exported for direct testability — the test hits it directly rather than round-tripping through the CLI. Both regression cases (regular 1.12 retention, fp-noise 0.30000000000000004 cleanup) pin the two failure modes the PR body cites.

🟢 Scope is tight. Only presentation logic changes; resolveSnapshotVideoFrameTime / computeSnapshotTimes / tailFrameTime are untouched, so underlying time computation stays exact. The rounding lives at the display boundary where it belongs.

Verdict framing

Small, well-scoped formatting fix. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

R1 — Position: RIGHT. Approve; findings are follow-ups.

The mechanism (Number(time.toFixed(3))) is a well-known trim-fp-noise idiom, locale-invariant (spec-defined . decimal in Number.prototype.toFixed), and produces filesystem-safe strings for every realistic snapshot time. formatSnapshotTimestamp is applied to both the progress label (timeLabel) and the filename (via timeLabel) and the atTimestamps list label — so no label-vs-filename divergence, which was the specific risk from the field report. Verified end-to-end at head 4e805f8e.

Extrapolation exercise (node -e '(t)=>Number(t.toFixed(3))+"s"' at head):

input	output	note
1.12	1.12s	reported case, fixed
0.30000000000000004	0.3s	fp-noise trimmed
1.125	1.125s	3-decimal boundary preserved
1.1235	1.123s	rounds to ms (banker's-style)
1.5000	1.5s	trailing zeros collapsed
2	2s	⚠ behavior change: was 2.0s
0	0s	⚠ behavior change: was 0.0s
0.0001	0s	⚠ silent sub-ms collapse
-1.5	-1.5s	filename gets double-dash but stays filesystem-safe
Infinity / NaN	Infinitys / NaNs	nonsense but non-throwing

Findings (non-blocking)

F1 — sibling .toFixed(1) sites still emit lower-precision. snapshot.ts lines 491, 506, 525 (diagnostic warnings — missed video-frame extraction, video-frame injection failure, --zoom no-visible-box) still print ${time.toFixed(1)}s. If a user runs --at 1.12 and one of those warnings fires, the warning line reads 1.1s while the label + filename read 1.12s. Same class of bug as the one this PR fixes, one indirection over. Recommend a follow-up that either routes those through formatSnapshotTimestamp or explicitly documents why the diagnostic surface intentionally rounds harder. Line 314's .toFixed(2) in the end-of-timeline note is a distinct precision choice that reads intentional.

F2 — regression parameterization is thin. The new it.each covers 2 rows: the reported 1.12 case and one fp-noise case. It does not lock in:

• integer input (2 → 2s) — a real behavior change from .toFixed(1)'s 2.0s; any downstream shell/regex parser expecting a decimal point in frame-*-at-*.png names now breaks silently on integer --at values.
• 3-decimal exact (1.125) — the precision-ceiling boundary.
• sub-ms input (0.0001 → 0s) — currently silently collapses to zero.
• 1.5000 → 1.5s — trailing-zero collapse.

Add rows for those (they all currently pass — you're locking in the answer, not changing it) so future refactors can't regress them.

F3 — non-finite guard. formatSnapshotTimestamp(NaN) returns \"NaNs\" (verified). Snapshot times flow through resolveSnapshotVideoFrameTime / computeSnapshotTimes upstream, which should reject non-finite input — but the formatter itself has no Number.isFinite guard. Trivial defense: if (!Number.isFinite(time)) return \"?s\" (or throw). Cosmetic; only matters if a caller ever routes user-supplied Infinity/NaN in.

Freshness

• head 4e805f8ebfef2084d665d4506ccabb7283b94229
• mergeable: MERGEABLE, mergeStateStatus: BLOCKED (needs approval)
• required checks: green
• reviews: none prior at head
• envelope clean (no Co-Authored-By: Claude, no 🤖 Generated with [Claude Code])

Not blocking on any of F1–F3. Right fix for the reported bug, minimal blast radius, both label + filename converge on one formatter.

— Via

[vanceingalls]

Formalizing APPROVE stake per Magi's request — my prior COMMENTED review at this head already stated RIGHT verdict with three non-blocking follow-ups (sibling .toFixed(1) at snapshot.ts:491/506/525, thin it.each, missing Number.isFinite guard). Follow-ups don't block. Fix is scope-correct for the reported symptom.

— Via