fix(cli): keep render filename timestamp local#2470
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at cd1741992a014bcefe2edaf42a1c7f3deb07d2cd.
What this does
Fixes a mixed-calendar filename timestamp for the default render output. Pre-PR at render.ts:614-615:
const datePart = now.toISOString().slice(0, 10); // UTC date
const timePart = now.toTimeString().slice(0, 8).replace(/:/g, "-"); // LOCAL timeSo on late local evenings whose UTC crosses midnight, the filename mixed a next-day UTC date with the current local time — e.g. Jul-14 22:55 PDT rendered as 2026-07-15_22-55-06. The extracted helper formatRenderOutputTimestamp(now) at renderOutputTimestamp.ts:1-6 now uses getFullYear() / getMonth() / getDate() / getHours() / getMinutes() / getSeconds() — all local — so date and time agree.
Test at renderOutputTimestamp.test.ts:6-16 pins the exact regression: TZ=America/Los_Angeles + new Date("2026-07-14T22:55:06-07:00") (which is Jul-15 in UTC) → "2026-07-14_22-55-06". Old code would have produced "2026-07-15_22-55-06".
Verification
- All local getters:
getFullYear,getMonth()+1,getDate,getHours,getMinutes,getSeconds— consistent local calendar for both halves. ✓ - Second-precision matches pre-PR (
toTimeString().slice(0, 8)was HH:MM:SS). No milliseconds regression. ✓ - Zero-padding on all fields via
pad(value)→String(value).padStart(2, "0"). Correct for months 1-9, days 1-9, hours 0-9, etc. ✓ - Batch template + single-file paths at
render.ts:618/621share the sametimestampvariable — no divergence between batch runs and single runs. ✓ - CI green at head. ✓
Concerns
-
🟢 Single-timezone regression coverage. The test pins one moment in one TZ. A parametric run across a few timezones (Pacific/Auckland, UTC, Europe/London with DST, Asia/Kolkata's
:30offset) would be more robust against a future accidental UTC swap — e.g. someone refactoring the helper totoISOString()slices again. Non-blocking. Follow-up worth a couple lines. -
🟢 Callsite isn't snapshot-tested. If someone later replaces
formatRenderOutputTimestamp(now)inrender.ts:614with a UTC alternative, only manual review would catch it — the helper's unit test would still pass. Tolerable given the helper is one call from one file; a brittle callsite snapshot would probably be more noise than signal. Note-only.
Verdict framing
Clean single-purpose fix backed by a targeted regression that pins the exact mixed-calendar day-boundary case. LGTM from my side.
There was a problem hiding this comment.
R1 review at head cd174199. One-line: correct root-cause fix on the CLI, but the sibling site in studio-server has the identical UTC-date + local-time pattern and stays broken.
[Race note] Rames's review posted ~42s before mine; I discovered his review after posting and edited this body to cite the divergence explicitly.
Strengths
packages/cli/src/utils/renderOutputTimestamp.ts:1-6— swaps to all-localDategetters (getFullYear/getMonth+1/getDate/getHours/getMinutes/getSeconds) so the whole timestamp comes from a single local calendar. That's the right root cause: the old code combinedtoISOString()(UTC calendar) withtoTimeString()(local clock), which is exactly what produces the tomorrow-date/today-time drift near local midnight.packages/cli/src/commands/render.ts:616-621—batchOutputTemplateandoutputPathnow share the sametimestampvariable, so a batch index sibling can no longer land under a different date than the single-output filename. That's the specific inconsistency called out in the source feedback thread and it's closed.packages/cli/src/utils/renderOutputTimestamp.test.ts:5-15— setsprocess.env.TZ = "America/Los_Angeles", restores it infinally, and pins the byte-for-byte expected output. A future accidental revert to a UTC-slice would trip immediately. Good positive-pin discipline.
blocker — Rule 2: sibling site with the identical failure mode is untouched
packages/studio-server/src/routes/render.ts:110-114 at main (and unchanged at the PR head) still has this exact pattern:
// fallow-ignore-next-line code-duplication ← author already flagged the twin
const now = new Date();
const datePart = now.toISOString().slice(0, 10); // UTC calendar
const timePart = now.toTimeString().slice(0, 8).replace(/:/g, "-"); // local clock
const jobId = `${project.id}_${datePart}_${timePart}`;jobId flows into outputPath = join(rendersDir, \${jobId}${ext}`)on line 118 and into therenderJobsmap key, so studio-server users hitting the render route near local midnight get the same tomorrow-UTC-date/today-local-time drift the CLI just fixed — this time in the persisted jobId. The// fallow-ignore-next-line code-duplication` on line 110 is dispositive: the author of the CLI code was already aware this exact pattern lives in studio-server. Fixing one twin and leaving the other stale means the class is not closed.
Concrete path: move renderOutputTimestamp.ts to a package both consume (studio-server already depends on @hyperframes/parsers and @hyperframes/core — @hyperframes/core is the natural home), then import from both packages/cli/src/commands/render.ts and packages/studio-server/src/routes/render.ts, and drop the now-obsolete fallow-ignore comments at both call sites.
important — regression coverage does not parameterize across every branch of the helper
renderOutputTimestamp.test.ts:11 fires a single fixture 2026-07-14T22:55:06-07:00. The helper has six padStart(2, "0") sites (Y-M-D-H-M-S). This fixture only actually exercises padStart on month (getMonth()+1 = 7 → "07") and seconds (getSeconds() = 6 → "06"). Day (14), hour (22), and minute (55) are already two-digit, so a regression that quietly dropped padStart from those three sites (e.g. someone "simplifying" to ${now.getDate()}) would still make the existing test pass. Add a second fixture with a single-digit day, hour, and minute — e.g. new Date("2026-01-05T04:07:03-08:00") at TZ=America/Los_Angeles expected 2026-01-05_04-07-03. That closes the parameterization gap across every branch the fix touches.
nit — stale comment if the class is closed
packages/cli/src/commands/render.ts:614 — the // fallow-ignore-next-line code-duplication was suppressing the duplication warning against the studio-server twin. If you take the blocker fix and lift the helper into @hyperframes/core, both call sites become one-liners with no duplication to ignore; drop the comment at both sites. If you don't, leave it.
Divergence from Rames
Rames posted an LGTM (state=COMMENTED with approve-prose) noting two non-blocking follow-ups: (1) parametric TZ coverage across Pacific/Auckland, UTC, Europe/London, Asia/Kolkata, and (2) no callsite snapshot test. Neither of those catches what I'm calling out here. Specifically:
- Rames did not flag the studio-server twin at
routes/render.ts:110-114, which is the actual class-not-closed miss on this PR. My blocker on that site is the gap between his review and the correctness bar Miguel called for. - Our regression-coverage findings sit at different levels of abstraction: Rames wants more timezones; I want more single-digit fixtures. TZ variation would catch the wrong-calendar failure class again; single-digit day/hour/minute would catch a
padStartregression that TZ variation would not. Both are worth adding — the second fixture I proposed is the tighter branch-coverage argument.
Notes
- CI is clean on this CLI-only slice;
mergeStateStatus: BLOCKEDis the reviewer gate only, no failing required check. - Two reviews on the PR at head
cd174199: Rames (COMMENTED, LGTM prose) and this one (CHANGES_REQUESTED).
Verdict: REQUEST CHANGES
Reasoning: The CLI fix is correct in isolation, but packages/studio-server/src/routes/render.ts:110-114 still runs the exact toISOString() UTC-date + toTimeString() local-time recipe this PR replaces; the author's own fallow-ignore code-duplication comment on that block confirms the twin was known. One shared helper in @hyperframes/core closes both sites; the regression test also wants a second fixture to actually cover the day/hour/minute padStart branches.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 0b6b18e30 — R1 (cd174199) → R2 delta is +10 / -10 across 5 files.
R2 delta
Closes my R1 🟢 on single-timezone regression coverage and closes a latent second-site bug I hadn't flagged.
- Helper hoisted to
@hyperframes/core— moved frompackages/cli/src/utils/renderOutputTimestamp.{ts,test.ts}topackages/core/src/utils/, exported atpackages/core/src/index.ts:234. CLIrender.ts:79now imports it from@hyperframes/core. - Second site closed —
studio-server/src/routes/render.ts:110-114. The studio-server'sPOST /renderroute was carrying the exact same mixed-calendar pattern (now.toISOString().slice(0, 10)+now.toTimeString().slice(0, 8).replace(/:/g, "-")) to buildjobIds, so studio-launched renders on late-local-evening PDT would also have produced next-dayjobIdstrings. Now consumesformatRenderOutputTimestamp(now)from the shared helper. fallow-ignore-next-line code-duplicationsuppressions removed from both callsites — no longer duplicated because they share the helper.- Single-digit test branch added via
it.each:"late local evening"—2026-07-14T22:55:06-07:00→"2026-07-14_22-55-06"(the original day-boundary regression)."single-digit calendar and clock fields"—2026-01-05T04:07:03-08:00→"2026-01-05_04-07-03"(pins zero-padding on months 1-9, days 1-9, hours 0-9, mins 0-9, secs 0-9).
- Doc comment added on the helper: "Format a render/job timestamp from one local calendar and clock." Makes the invariant explicit for future callers.
Verification
- Both callsites now identical — same
now = new Date(); formatRenderOutputTimestamp(now)shape atrender.ts:614andstudio-server/render.ts:114. Consistency between CLI and studio-launched renders. ✓ - Zero-padding is exercised.
pad("5")→"05"— the single-digit test would have failed against a naïveString(getMonth() + 1)implementation. Regression guard. ✓ @hyperframes/corewas the correct move.parseFpsis already exported alongside it at the same barrel (studio-server/render.ts:7), so callers use one core-utils import path. ✓- CI: mergeable, all completed jobs SUCCESS on current head at read time. ✓
R1 concerns status
- 🟢 Single-TZ regression coverage → CLOSED via the it.each pattern with two branches.
- 🟢 Callsite not snapshot-tested → SUPERSEDED by the fact that there are now two callsites both using the shared helper — a snapshot on either would only pin the helper's output, which the unit test already does.
- Bonus:
studio-server/render.tshad the same latent bug and is now closed at the same time.
Verdict framing
Clean lift-and-share R2 that closes the original R1 comment plus an additional callsite I didn't flag. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
R2 verify at head 0b6b18e3 — delta from R1 (cd174199) is a single commit that lifts the timestamp recipe into @hyperframes/core and adds a second regression fixture. All three R1 findings are resolved and I saw nothing new to raise.
Verdict
APPROVE — the extrapolation blocker (F1), the parameterization gap (F2), and the stale suppression (F3) are all cleaned up in one coherent move.
R1 findings — status
F1 — ✅ RESOLVED (extrapolation blocker). The helper was hoisted:
packages/core/src/utils/renderOutputTimestamp.tsnow holdsformatRenderOutputTimestamp(now), using local getters (getFullYear/getMonth/getDate/getHours/getMinutes/getSeconds) with a sharedpadStart(2, "0").packages/core/src/index.ts:234re-exports it.- CLI:
packages/cli/src/commands/render.ts:79importsformatRenderOutputTimestampfrom@hyperframes/core; the twin at line 615 now feeds bothoutputPathandbatchOutputTemplatefrom the same sharedtimestamp. - Studio-server:
packages/studio-server/src/routes/render.ts:7imports from@hyperframes/core; the R1 twin at 110-114 collapses toformatRenderOutputTimestamp(now)at line 111. - Grepped both files at head — zero remaining
toISOString/toTimeStringcalls. No new callsites bypass the helper.
F2 — ✅ RESOLVED (regression parameterization). renderOutputTimestamp.test.ts now runs it.each over two fixtures:
2026-07-14T22:55:06-07:00→ covers the late-evening / next-UTC-day boundary (month + seconds padStart).2026-01-05T04:07:03-08:00→ covers every remaining single-digit branch (day 05, hour 04, minute 07, seconds 03, month 01).
Dropping padStart from any of the five zero-prefixed sites would now fail. TZ scoping is preserved via the process.env.TZ try/finally block.
F3 — ✅ RESOLVED (nit). The stale // fallow-ignore-next-line code-duplication above const now = new Date(); at the previous cli render.ts:614 is gone. The remaining fallow-ignore comments in cli render.ts (complexity, unused-export) and studio-server render.ts (code-duplication at 189 / 210) are on unrelated blocks.
Adversarial pass
- No new callsites in the diff bypass the helper — CLI has one call site, studio-server has one call site, both go through
@hyperframes/core. - DST / TZ: the helper reads local wall-clock via
Date.prototype.get*— during a DST transition it produces the wall clock the user sees, which is the intended filename semantic. No UTC/local mix remains. - Repo-wide code search for
toTimeString().slicereturns only the two files touched by this PR (the search index still points at main; both are refactored at this head). - Freshness:
mergeable_state=blockedbecause approvals are still pending; regression-shards 1-8 arein_progress. All completed required checks (Preflight, CodeQL, Perf: *, CLI: npx shim across OSs, SDK smoke, Studio smoke, Tests on windows) are green. No CI-red signal at this head.
— Via
Summary
Why
The render command combined a UTC date from
toISOString()with a local time fromtoTimeString(). Near local midnight this produced filenames such as a July 15 date beside a July 14 local-time sibling, making resumed render output confusing.Source feedback: https://heygen.slack.com/archives/C0BGC335AQY/p1784095063194819
Verification
2026-07-14_22-55-06, received2026-07-15_22-55-06pnpm --filter @hyperframes/cli exec vitest run src/utils/renderOutputTimestamp.test.tspnpm exec oxfmt --check packages/cli/src/commands/render.ts packages/cli/src/utils/renderOutputTimestamp.ts packages/cli/src/utils/renderOutputTimestamp.test.tspnpm exec oxlint packages/cli/src/commands/render.ts packages/cli/src/utils/renderOutputTimestamp.ts packages/cli/src/utils/renderOutputTimestamp.test.tsFull CLI typecheck was not a valid local signal in the sparse worktree because its shared package symlinks resolve to the older main checkout; CI will run against a coherent workspace.
Risk
Low. Explicit
--outputpaths are unchanged. Only auto-generated render filenames change, and only their date component now matches the already-local time component.