Skip to content

fix(cli): keep render filename timestamp local#2470

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/render-filename-local-date
Jul 15, 2026
Merged

fix(cli): keep render filename timestamp local#2470
miguel-heygen merged 2 commits into
mainfrom
fix/render-filename-local-date

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Summary

  • generate default render output dates and times from the same local calendar
  • cover the local-evening/next-UTC-day boundary reported from a resumed render
  • apply the shared timestamp to both single and batch default filenames

Why

The render command combined a UTC date from toISOString() with a local time from toTimeString(). 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

  • regression test demonstrated the old output: expected 2026-07-14_22-55-06, received 2026-07-15_22-55-06
  • pnpm --filter @hyperframes/cli exec vitest run src/utils/renderOutputTimestamp.test.ts
  • pnpm exec oxfmt --check packages/cli/src/commands/render.ts packages/cli/src/utils/renderOutputTimestamp.ts packages/cli/src/utils/renderOutputTimestamp.test.ts
  • pnpm exec oxlint packages/cli/src/commands/render.ts packages/cli/src/utils/renderOutputTimestamp.ts packages/cli/src/utils/renderOutputTimestamp.test.ts

Full 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 --output paths are unchanged. Only auto-generated render filenames change, and only their date component now matches the already-local time component.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 time

So 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/621 share the same timestamp variable — 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 :30 offset) would be more robust against a future accidental UTC swap — e.g. someone refactoring the helper to toISOString() slices again. Non-blocking. Follow-up worth a couple lines.

  • 🟢 Callsite isn't snapshot-tested. If someone later replaces formatRenderOutputTimestamp(now) in render.ts:614 with 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.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-local Date getters (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 combined toISOString() (UTC calendar) with toTimeString() (local clock), which is exactly what produces the tomorrow-date/today-time drift near local midnight.
  • packages/cli/src/commands/render.ts:616-621batchOutputTemplate and outputPath now share the same timestamp variable, 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 — sets process.env.TZ = "America/Los_Angeles", restores it in finally, 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 padStart regression 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: BLOCKED is 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 james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Helper hoisted to @hyperframes/core — moved from packages/cli/src/utils/renderOutputTimestamp.{ts,test.ts} to packages/core/src/utils/, exported at packages/core/src/index.ts:234. CLI render.ts:79 now imports it from @hyperframes/core.
  2. Second site closed — studio-server/src/routes/render.ts:110-114. The studio-server's POST /render route was carrying the exact same mixed-calendar pattern (now.toISOString().slice(0, 10) + now.toTimeString().slice(0, 8).replace(/:/g, "-")) to build jobIds, so studio-launched renders on late-local-evening PDT would also have produced next-day jobId strings. Now consumes formatRenderOutputTimestamp(now) from the shared helper.
  3. fallow-ignore-next-line code-duplication suppressions removed from both callsites — no longer duplicated because they share the helper.
  4. 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).
  5. 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 at render.ts:614 and studio-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ïve String(getMonth() + 1) implementation. Regression guard. ✓
  • @hyperframes/core was the correct move. parseFps is 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.ts had 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.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts now holds formatRenderOutputTimestamp(now), using local getters (getFullYear / getMonth / getDate / getHours / getMinutes / getSeconds) with a shared padStart(2, "0").
  • packages/core/src/index.ts:234 re-exports it.
  • CLI: packages/cli/src/commands/render.ts:79 imports formatRenderOutputTimestamp from @hyperframes/core; the twin at line 615 now feeds both outputPath and batchOutputTemplate from the same shared timestamp.
  • Studio-server: packages/studio-server/src/routes/render.ts:7 imports from @hyperframes/core; the R1 twin at 110-114 collapses to formatRenderOutputTimestamp(now) at line 111.
  • Grepped both files at head — zero remaining toISOString / toTimeString calls. 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().slice returns only the two files touched by this PR (the search index still points at main; both are refactored at this head).
  • Freshness: mergeable_state=blocked because approvals are still pending; regression-shards 1-8 are in_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

@miguel-heygen miguel-heygen merged commit 017183a into main Jul 15, 2026
54 checks passed
@miguel-heygen miguel-heygen deleted the fix/render-filename-local-date branch July 15, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants