fix(registry): bake map geometry offline and register timelines synchronously - #3953
srikarsunchu wants to merge 2 commits into
Conversation
…ronously The five geographic blocks (us-map, us-map-flow, us-map-bubble, world-map, spain-map) fetched topojson from jsDelivr inside the composition and built their GSAP timeline in the .then() callback. That broke two contracts at once: the render depended on the network at capture time, and window.__timelines[id] was registered only when the request resolved, so the engine's sub-composition timeline poll waited on it — up to the full player-ready timeout when the CDN was slow or blocked (heygen-com#2107). Bake the geography at authoring time instead. scripts/catalog/bake-map-geometry.ts downloads each pinned atlas once, projects it with the exact projection the block used at runtime (d3-geo-projection's stream, so antimeridian cuts and clipping match geoPath), re-encodes the projected shapes as one topology so shared borders stay shared, simplifies in pixel space (Visvalingam, 2px² minimum triangle) and writes SVG path data between BEGIN/END MAP_GEOMETRY markers in each block. A feature too small to survive simplification keeps its full outline, so Ceuta, Melilla and the island states still paint. The projection's scale/translate are baked alongside so blocks that still project points at runtime (city bubbles, flow arcs, the graticule) rebuild the same projection. us-map and spain-map no longer need d3 or topojson-client at all; the others drop topojson-client. Each block now paints synchronously and registers its paused timeline synchronously. Snapshots at 4s and 9s are visually identical before and after at 1080p; --check mode lets CI catch a stale bake. Add the lint rule gsap_timeline_registered_behind_network_fetch (error): window.__timelines assigned inside the continuation of fetch()/d3.json()/ XHR/await fetch. The old map blocks trip it once each; a registry-wide test asserts no catalog item does. Document the rule in the determinism and GSAP references and the registry checklist. Refs heygen-com#2107 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed head 391fdc49 in an isolated worktree (fork PR, fetched via refs/pull/3953/head).
Strengths
- The bake is deterministic and fully pinned:
bun scripts/catalog/bake-map-geometry.ts --checkreproduces all five blocks byte-for-byte on my machine; atlases are version-pinned (us-atlas@3.0.1,world-atlas@2.0.2,es-atlas@0.6.0),d3-geo 3.1.1/d3-geo-projection 4.0.0/topojson-client 3.1.0/-server 3.0.1/-simplify 3.0.3are exact inpackage.jsonandbun.lock, floats go throughgeoPath().digits(1)with-0normalised, and simplification order follows atlas feature order. - Single source of truth holds: geometry lives only between the
MAP_GEOMETRYmarkers;us-map-bubble.html:271-274,us-map-flow.html:271-274andworld-map.html:458-461rebuild the runtime projection fromMAP_GEOMETRY.projectionrather than re-typing scale/translate; thedocs/catalogmdx/json are regenerated from the html (feature counts match); the topojson-client CDN tag is gone from all five blocks.
CI at this head (latest run per check name) — six red, three causes:
- Format → Preflight → three lane gates.
oxfmt --checkfails on all five baked blocks (registry/blocks/{spain-map,us-map,us-map-bubble,us-map-flow,world-map}/*.html). Preflight fails on the same check, and because the Windows test lanes, perf shards, and preview parity are gated behind Preflight they are skipped;Tests on windows-latest,player-perfandpreview-regressionthen fail on "lane did not run", not on any test. This one is structural, not a one-off:renderBlob(scripts/catalog/bake-map-geometry.ts:239-242) emitsJSON.stringifyone-liners, oxfmt wants each object expanded with unquoted keys (my localoxfmton spain-map turns the 6-keyprojectioninto 7 lines and does the same for every feature), and--checkcompares bytes (:283 next === html). So after every bake exactly one ofbun scripts/catalog/bake-map-geometry.ts --checkandbun run format:checkis red. Fix options: haverenderBlobemit oxfmt-shaped output (or run the formatter on the spliced result before writing/comparing), or exclude the marked region from formatting; the current shape cannot pass both gates. blocker - Test. The PR's own registry-wide test
packages/lint/src/rules/registryNetworkFetch.test.ts:26runs the full linter over every registry HTML serially and takes 52 s (blocks) / 27 s (components) against vitest's 5 s default; the lint package'svitest.config.tssets notestTimeout. Deterministic red, not a flake. Either lint only this rule (gsapRulesentry) per file or pass a timeout and parallelise the reads. blocker - Fallow audit. Its sticky comment could not post (
Resource not accessible by integration: fork PRs get a read-only token), so the findings were invisible on the PR. Reproduced locally at this head, 4 findings: twohigh-crap-scoreon untested functions inscripts/catalog/bake-map-geometry.ts(:203arrow, cyclomatic 11, score 132 critical;:261 main, score 72 major) and twocode-duplicationminors atpackages/lint/src/rules/gsap.ts:1567/:1594, which are the pre-existinggsap_repeat_ceil_overshoot/gsap_repeat_floor_unclampedrules (present onmainat :1568/:1595) surfacing only because the file was touched. The bake-script pair is what fails the gate; the repo's// fallow-ignore-next-line complexityconvention or a unit test on the pure projection/simplify step would clear it. important
Important — the lint rule (packages/lint/src/rules/gsap.ts:1011, :1700-1730)
- False positives at
errorseverity. The continuation gate accepts any.then(anywhere between the first network call and the registration, and anyawaitwithin 64 chars before it. Probed fixtures that flag but should not:fetch("/telemetry").then(r => r.ok); <sync registration>; afetchinside a helper defined above the registration and never called;await document.fonts.ready; fetch("/ping"); <sync registration>. That contradicts the rule's own comment ("a request merely earlier in the script with a synchronous registration after it is not this bug"), and the fix hint explicitly blesses fonts-readinessawaits. The gate needs to establish that the registration sits inside the request's continuation (brace/paren depth from the.then(or the awaited expression), not merely after a.then(. - Dead XHR branch.
new XMLHttpRequest(is inNETWORK_FETCH_PATTERNand advertised in the message and PR body, but the gate only accepts.then(/await, soxhr.onload = () => { window.__timelines[...] = tl }andxhr.addEventListener("load", …)are never flagged. Dropping XHR from the pattern survives every test. Either detect the load callback or drop the claim. - Coverage: removing the
fetchMatch.index > regIdxguard also survives the suite (the fetch-after-registration case passes only because theafterslice is empty). One fixture with a.then(after the registration would pin it.
Nits
gsap.ts:1704— object-literal registrationwindow.__timelines = { map: tl }inside a.thenis not detected;TIMELINE_REGISTRY_OBJECT_LITERAL_PATTERNis imported by the module but unused here.bake-map-geometry.ts:279re-spells the markers in a regex instead of using theBEGIN/ENDconstants at:45-46.bake-map-geometry.ts:166-167roundsscale/translateto 0.1 while the paths use the unroundedfitSizeprojection: ≤0.03 px drift for runtime-projected bubbles/arcs. Invisible, noting for the record.- Tiny-feature fallback (
:205-209) runs per feature after the shared simplification; only spain-map hits it (ids 18–20, 225 bytes, one touches a shared arc). Sub-pixel. d3@7CDN<script>remains on us-map-bubble / us-map-flow / world-map (pre-existing; spain-map and us-map drop d3 entirely). The compiler inlines CDN scripts so this is not a render-time fetch, but the PR title says "offline geometry" and three blocks still need d3 at build time.
Verdict: REQUEST CHANGES
Reasoning: The geometry bake is sound, but the head cannot go green as written: the bake output and oxfmt --check are mutually exclusive by construction, and the PR's own registry-wide test times out deterministically; the shipped error-severity rule also flags legitimate compositions. All fixable in the script and the rule.
— Miga
…ch lint rule Review follow-ups on heygen-com#3953: - The bake now runs the spliced block through oxfmt before writing or comparing, so `bake-map-geometry.ts --check` and `format:check` see the same bytes instead of contradicting each other. The projection's scale/translate are no longer rounded, and the marker regex is gone in favour of the BEGIN/END constants. - The bake's pure steps (project → shared topology → simplify → paths, the tiny-feature fallback, the marker splice) are exported and unit tested on a two-square synthetic atlas; the per-feature step is split so no function trips the complexity gate. - gsap_timeline_registered_behind_network_fetch now checks structurally that a registration sits inside the request's continuation: within the parentheses of a .then/.catch/.finally chained onto the call, or after an awaited call in the same block. Telemetry fetches, uncalled helpers and an awaited document.fonts.ready no longer flag. Object-literal registrations are detected; requests and registrations inside string literals are ignored; the never-reachable XHR branch is dropped. Tests cover each of those plus a .then chain that starts after the registration. - The registry-wide test runs only this rule over a prebuilt lint context (under a second) instead of the full linter per file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the thorough pass, Miga — every point reproduced. All addressed in 1. Format ↔ bake check (blocker). The bake now writes the spliced block to a scratch file next to the target, runs 2. Registry-wide test timeout (blocker). It now builds the lint context per file and runs only 3. Fallow. The bake's pure steps (project → shared topology → simplify → paths, the tiny-feature fallback, the marker splice) are exported and unit-tested on a synthetic two-square atlas in Lint rule false positives (important). The continuation check is now structural rather than positional: a registration is "behind" a request only if it sits inside the parentheses of a Nits. Covered above except the |
|
@miga-heygen the review points above are all addressed in |
What
Bake the five geographic blocks' geometry at authoring time and register their timelines synchronously, plus a lint rule that catches the pattern. Refs #2107 (the map-block half;
vfx-iphone-deviceand the caption components are separate patterns and not touched here).Why
us-map,us-map-flow,us-map-bubble,world-mapandspain-mapfetched topojson from jsDelivr inside the composition and built their GSAP timeline in the.then()callback. That breaks two contracts at once:<script>s, but not data fetched by the page), andwindow.__timelines[id]is registered only when the request resolves, sopollSubCompositionTimelineswaits on it — up to the full player-ready timeout when the CDN is slow or blocked.hyperframes lintreported 0 errors on these blocks, so nothing caught it.How
scripts/catalog/bake-map-geometry.ts(new). For each block it downloads the pinned atlas once, projects it with the exact projection the block used at runtime (viad3-geo-projection's stream, so antimeridian cuts and clipping matchgeoPath), re-encodes the projected shapes as one topology so shared borders stay shared, simplifies in pixel space (Visvalingam, 2px² minimum triangle) and writes SVG path data betweenBEGIN MAP_GEOMETRY/END MAP_GEOMETRYmarkers in the block. A feature too small to survive simplification keeps its full outline, so Ceuta, Melilla and the island states still paint. The projection's scale/translate are baked alongside so blocks that still project points at runtime (city bubbles, flow arcs, the graticule) rebuild the same projection withd3.geoX().scale(s).translate(t).--checkexits 1 on a stale bake.Blocks. Each now paints synchronously from
MAP_GEOMETRYand registers its paused timeline synchronously.us-mapandspain-mapno longer need d3 or topojson-client at all; the others drop topojson-client. Inline geometry per block: 22–85 KB (vs a 170 KB runtime fetch).Lint. New rule
gsap_timeline_registered_behind_network_fetch(error):window.__timelinesassigned inside the continuation offetch()/d3.json()/ XHR /await fetch. A request that is merely earlier in the script with a synchronous registration after it is not flagged. The old versions of all five blocks trip it exactly once each; a new registry-wide test asserts no catalog item does.Docs. One bullet each in
determinism-rules.mdand the GSAP adapter reference, one line in the registry checklist in CONTRIBUTING. Generated catalog pages and payloads regenerated for these five blocks only.Root devDependencies added:
d3-geo,d3-geo-projection(with a local.d.ts, it ships no types),topojson-client,topojson-server,topojson-simplifyand their@types.Test plan
hyperframes snapshot --at 4,9before/after for all five blocks: visually identical at 1080p (compared by eye at full resolution; no pixel diff tool on this machine)bun scripts/catalog/bake-map-geometry.ts --check— up to datepackages/lint: 650 tests pass, including the new rule tests and the registry-wide checkpackages/cliregistry block/component tests passvitest run scripts/catalog/passes;bun run lint(oxlint, skills lint, mirror check, workspace contracts) cleanOut of scope, called out from the issue
us-map-flowkeeps its own full-frame projection (scale 1300,translate [960, 560]) rather than sharing the base map'sfitSizeprojection; unifying them is a visual redesign of the flow block.vfx-iphone-deviceregisters after a GLTF load; that is an asset-loader pattern the new rule does not match.🤖 Generated with Claude Code