refactor(core): extract maxEndTime+serialize to parsers/test-utils.ts (TU)

[vanceingalls]

Summary

• Extracts maxEndTime and serialize helpers that were duplicated inline in T1 (htmlParser.roundtrip.test.ts) and T2 (stableIds.test.ts) into a shared packages/core/src/parsers/test-utils.ts
• Both test files now import from test-utils.ts; generateHyperframesHtml is still imported directly in T1 for the includeScripts: true path that isn't abstracted

Also fixes the lefthook fallow hook to unset GIT_DIR, GIT_INDEX_FILE, and GIT_WORK_TREE before running — these variables are set by git in worktree hook context and prevent fallow's internal temp-worktree creation.

Test plan

• bun run --cwd packages/core test -- htmlParser.roundtrip.test.ts stableIds.test.ts — all existing T1/T2 tests pass
• Pre-commit hook (lefthook) runs fallow without the worktree error in a git worktree context

[vanceingalls]

• test(core,studio): add T3+T7 hfId targeting stubs (spec for R1) #1267
• test(core): add T6a GSAP parser golden baselines (Recast/Babel snapshot) #1263
• test(core): add T10 PreviewAdapter contract stubs (spec for R7) #1262
• refactor(core): extract maxEndTime+serialize to parsers/test-utils.ts (TU) #1261 👈 (View in Graphite)
• test(studio): add T5c review-fix gaps in manualEditsDomPatches characterization #1259
• test(studio): add T5b rotation+motion build-patches characterization #1258
• test(studio): add T5a pathOffset+boxSize build-patches characterization #1257
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miguel-heygen]

Clean refactor with two well-scoped changes:

parsers/test-utils.ts — maxEndTime and serialize extracted from two existing test files. The fixed compositionId: "test-comp" is the key detail: prevents Date.now() churn in snapshot comparisons, necessary for deterministic golden output in T6a.

lefthook.yml fallow hook fix — env -u GIT_DIR -u GIT_INDEX_FILE -u GIT_WORK_TREE prefix is the right fix for the worktree context issue. Without it, fallow inherits the worktree's GIT_DIR and can misidentify the repo root when running in a detached worktree. Adding test-utils.ts to tsconfig.json excludes matches the established pattern for test helpers that aren't part of public package exports.

One confirmation: htmlParser.roundtrip.test.ts and stableIds.test.ts lose their local serialize/maxEndTime definitions and import from test-utils.ts. The -18/-17 deletions are all dead local copies — no behavior change.

Approved.

[james-russo-rames-d-jusso]

Round-1 review of the TU refactor — maxEndTime + serialize extraction. Different shape from the T5* PRs since this is a refactor, not a test addition. Walked the delta (+42/-37, 5 files) with the lens of (1) clean extraction, (2) caller updates, (3) location/naming, (4) imports / tree-shakeability, (5) tsconfig surface, (6) coverage on the extracted utils. Plus one concern about PR scope.

Verified the duplication was real before posting: grep -nE "function maxEndTime|function serialize\(parsed" packages/ returns exactly the two file-level definitions that this PR deletes (stableIds.test.ts:19,24 and htmlParser.roundtrip.test.ts:15,20), nothing else. The "extracted from T1 and T2" claim is accurate — no stragglers in hyperframes.test.ts or elsewhere.

What's right

Extraction is clean and structurally equivalent. The two helpers were verbatim-identical in T1 and T2 (I diffed the deletions — same Math.max, same compositionId: "test-comp" literal, same parsed.styles ?? undefined coalesce). New test-utils.ts:13-16,23-28 reproduces both functions and keeps the doc comment about Date.now() churn masking instability — which is load-bearing for a future reader because it explains WHY compositionId is fixed in the test. Preserving the reason was the right call.

Both call sites updated. htmlParser.roundtrip.test.ts:12 switched to import { maxEndTime, serialize } from "./test-utils.js"; stableIds.test.ts:16 to import { serialize } from "./test-utils.js" (only serialize used downstream — maxEndTime was inlined-then-unused after the extract, dropped correctly). No callers stranded on the old inline helpers.

Named exports are tree-shakeable. export function maxEndTime / export function serialize rather than a default object. ✓

tsconfig exclude is the right escape. packages/core/tsconfig.json adds "src/parsers/test-utils.ts" to the production-build exclude list. The file lives under src/ but isn't a .test.ts and isn't a production module either — without the exclude, it would land in dist/ as a test helper that imports from ../generators/hyperframes.js. Excluding it keeps the public surface clean. Vitest type-checks via its own config (the **/*.test.ts it picks up has type-aware imports of test-utils.ts at test time), so this doesn't break the type-check loop. The Typecheck CI job is green on this SHA, confirming.

Concerns

[concern — two unrelated changes bundled in one PR] lefthook.yml adds env -u GIT_DIR -u GIT_INDEX_FILE -u GIT_WORK_TREE before bunx fallow audit. The fix itself is correct — running pre-commit hooks inside a git worktree does set those env vars and they do break tools that try to create their own internal worktrees. But this has nothing to do with maxEndTime/serialize extraction. Two unrelated fixes bundled means a reviewer who wants to revert the refactor (e.g. if the tsconfig exclude ends up breaking some downstream tool) has to manually un-revert the lefthook change.

Not blocking — both diffs are small enough to grok at once — but worth noting the pattern. If you're already in a worktree-aware sweep, follow-up PRs that target build/CI infra cleanly separate from product-code refactors land more reviewable.

[nit — test-utils.ts naming, weak signal it's test-only] The file is named test-utils.ts (no .test. infix), and the doc comment says "Not part of the public package exports — consumed only by *.test.ts files". That's the intent; the enforcement is the tsconfig exclude alone. Two stronger conventions:

• Move to packages/core/src/parsers/__test-utils__/index.ts (matches the __goldens__/ directory pattern that lands in #1263 for similar reasons — fallow / oxfmt etc. already gitignore-style-match on __*__).
• Or rename to _test-utils.ts with underscore prefix (some tsconfigs auto-exclude _*.ts from build).

Either makes "this is a test-only file" visible without having to read tsconfig.json. Not blocking — the current shape works — but worth considering before more test-utils.ts files accrete and the convention has to scale.

[nit — no unit coverage on the extracted utils themselves] maxEndTime([]) returning 0 and maxEndTime([{ startTime: 1, duration: 2 }, { startTime: 3, duration: 1 }]) returning 4 are exercised through T1/T2 end-to-end but not directly. For a 4-line helper that's fine; if test-utils.ts ever grows past trivial helpers (e.g. a fixture builder with branching), give it its own test-utils.test.ts.

Plan-doc alignment

TU = "extract shared parser test utilities to enable T6+." Scope is exactly that. The placeholder in the doc-comment // T1, T2, T6… reserves the consumer set, which lines up with #1263 also adding parser tests (though T6a uses a different golden-snapshot pattern, not these helpers).

Verdict

Clean small refactor with the right ergonomics. Two unrelated-changes-bundled concern is the only "I'd push back" item; the rest are convention nits. Leaving as a comment.

— Review by Rames D Jusso

The base branch was changed.