fix(core): treat unparsable timing attributes as missing instead of NaN

[calcarazgre646]

Problem

Element-level timing attributes go through parseFloat unguarded in the timing compiler and the HTML parser, so a typo like data-start="abc" propagates as NaN with no signal to the user:

• compileTag (timingCompiler.ts) computes data-end as start + parseFloat(durationStr) and serializes the result into the compiled HTML: the output literally contains data-end="NaN". The same unguarded reads exist in injectDurations, extractResolvedMedia (start/mediaStart; duration was already guarded), clampDurations, and the unresolved-div scan.
• parseHtml (htmlParser.ts) computes duration = Math.max(0, parseFloat(dataEnd) - start) — and Math.max(0, NaN) is NaN, so a garbage data-end lands in the timeline model as a NaN duration. updateElementInHtml writes data-end="NaN" back into the document when data-start is garbage.

Neither lint nor compile reports anything: the linter has no numeric check for element timing attributes, while composition-level duration is already validated (validateCompositionHtml errors on non-finite data-composition-duration, and extractCompositionMetadata guards with isFinite). Element timing attrs missed the same policy.

Change

Unparsable is treated as missing, which all of these paths already know how to handle:

• New parseTimingAttr(value, fallback) helper in timingCompiler.ts (the same policy extractResolvedMedia already applies to data-duration), used at every timing-attribute read in both files.
• compileTag additionally normalizes the compiled output: an unparsable data-start is rewritten to 0, and unparsable data-end/data-duration are stripped so the element takes the existing recompute-or-resolver path. Downstream consumers (parser, runtime) never see the garbage values.
• parseHtml falls back to its existing defaults (start 0, the 5s default duration).

No behavior change for valid inputs.

Tests

• Compiler: unparsable data-start normalized to 0 with data-end computed from it; unparsable data-duration dropped and the element marked unresolved; unparsable data-end stripped and recomputed; unparsable data-media-start resolved to 0; direct parseTimingAttr cases (including "Infinity"). All assert the compiled output contains no NaN.
• Parser: unparsable data-start parses as 0; unparsable data-end falls back to the 5s default instead of NaN; updateElementInHtml recomputes data-end from 0 instead of writing NaN.
• Full core suite: 1175 passed. bun run build green.

[vanceingalls]

Clear with minor nits

The case is straightforward, the deduction sound, and the evidence on the floor accounts for every footprint in the snow. A typo in a data-* timing attribute was sailing past parseFloat and being re-serialized as the literal string "NaN", where it then poisoned every downstream reader with all the discretion of a fox in a chicken coop. The remedy — a small, well-named parseTimingAttr(value, fallback) helper that mirrors the Number.isFinite policy extractResolvedMedia had already adopted for data-duration — is the long-term shape: one guard, applied uniformly at every read in the compile and parse layers, with a deliberate normalization-or-strip at every write so the garbage never reaches the document. Tests pin the cardinal points (abc, oops, Infinity, the Math.max(0, NaN) propagation in the parser, the updateElementInHtml write path), all 1175 core suites pass, and CI sits green across CodeQL, perf, regression, and the Windows render shard.

A few observations worth registering. None blocking.

Findings

1. Dispatch chain — read-side consumers in packages/core not unified onto parseTimingAttr.

The helper is exported, but five sibling readers continue to defend against garbage with the older parseFloat(...) || 0 idiom rather than adopting the new canonical guard:

• packages/core/src/runtime/adapters/css.ts:96 — Number.parseFloat(entry.el.getAttribute("data-start") ?? "0") || 0
• packages/core/src/studio-api/helpers/sourceMutation.ts:298, 301, 302, 365 — same || 0 pattern across data-start, data-end, data-duration, data-playback-start
• packages/core/src/studio-api/helpers/sourceMutation.ts:366-367 — a hand-rolled Number.isFinite(rateRaw) ? rateRaw : 1 that is structurally parseTimingAttr with a non-zero fallback

The audit is reassuring: each of these sites is read-only and NaN || 0 = 0, so garbage already silently collapses to 0 at every consumer — no NaN escapes into the runtime. So this is not the band-aid pattern (no contract drift, no surface bug), but it is the missed unification opportunity: a future reader will wonder why two idioms coexist for the same job, and the next pair of eyes that needs to widen the policy (say, to clamp negatives, or to telemeter unparsable hits) will have to touch six call sites in two idioms instead of one. A follow-up PR that ports the || 0 and the hand-rolled Number.isFinite site onto parseTimingAttr would close the door fully and is cheap. Mentioning in the description that "the read-side || 0 idiom at css.ts and sourceMutation.ts already neutralizes the same garbage and was intentionally left as a follow-up" would also do the job.

2. Small footgun — getAttr's discovery regex ([^"']+) and the strip regex ([^"']*) disagree on empty values.

In timingCompiler.ts, getAttr at line 60-63 uses [^"']+ (one-or-more), so data-end="" returns null from the getter. The unparsable-strip at line 124 uses [^"']* (zero-or-more), which would match the empty-value attribute — but it never runs, because the prior getAttr call returned null and the dataEndStr !== null guard short-circuits. Practical consequence: an empty data-end="" survives compile-time intact. The parser at htmlParser.ts:190 catches it via parseTimingAttr's parseFloat("") = NaN branch and falls back to start + 5, so the NaN never escapes into the timeline model — but the compiled HTML itself still carries the malformed attribute. Small; bounded by the downstream guard; worth a one-line follow-up either way (either widen the getter to [^"']* so empty values are detected and stripped, or accept that empty values are a no-op at compile and rely on parse-side fallback). I lean toward widening the getter for symmetry; the cost is one character.

3. parseFloat("Infinity") === Infinity — covered by the helper, but worth a note.

The unit test at timingCompiler.test.ts:246 correctly asserts that parseTimingAttr("Infinity", 3) falls back to 3 because Number.isFinite(Infinity) === false. Good. The same logic guards parseFloat("1e1000") (which JavaScript silently coerces to Infinity). Whether intentional or coincidental, it is the right behavior — a literal Infinity in data-end should not be honored as a clip ending in the far future. The author may want to add a one-line comment to the helper noting that the Number.isFinite check is also what rejects "Infinity" and "NaN" as string-literals, which are the two finite-looking failure modes a parser-only reader might miss.

4. Test coverage gaps — minor.

The headline cases are well-covered. Three composite shapes are tacit-only:

• Both data-start="abc" AND data-end="oops" on the same element. By composition of the existing logic, data-start normalizes to 0, data-end strips, the element marks unresolved with start=0. No explicit test asserts the composition; the coverage relies on each helper test plus the reader's compositionality argument.
• clampDurations with a garbage data-start. The path at timingCompiler.ts:303 now uses parseTimingAttr, so a previously-NaN-poisoned data-end will now recompute cleanly to the clamp value. Useful change, untested.
• injectDurations with a garbage data-start. Same shape at line 238. Tested for the clean case at timingCompiler.test.ts:112-127; the unparsable-data-start branch is not explicitly exercised.

The composition is correct; the additional cases would be one-liners. Optional.

Stale-base check

The PR branches from ef186139. Current main advances to 513819ee. Between them, only 6f677292 (PR #1413, the Miguel-led packages/core simplification) touched the same files (htmlParser.ts only). The regions are disjoint — #1413 modified the UHD-resolution constants near line 139 and the addElementToHtml helper around line 612; #1206 modifies the parseHtml element walk at lines 182-191 and updateElementInHtml at line 532. No squash hazard. A pre-merge rebase will conflict-free.

CI

All required checks green at d34c592c: CI build/lint/typecheck/test/format/fallow-audit, CodeQL (the prior \s* ReDoS finding was resolved by the bounded-prefix fix in the head commit), perf load/fps/scrub/drift/parity, preview-regression, 8 regression shards, Windows render verification, CLI smoke, semantic PR title.

Verdict

Clear with minor nits at d34c592c. None of the four findings are band-aid-bar issues — the dispatch chain is intact, the regex strip is sound, and the parser fallback is the right shape. Re-verify if HEAD moves before stamp.

Review by Via