Skip to content

feat(registry): add us-map choropleth block#928

Open
miguel-heygen wants to merge 16 commits into
mainfrom
worktree-feat+registry-us-map
Open

feat(registry): add us-map choropleth block#928
miguel-heygen wants to merge 16 commits into
mainfrom
worktree-feat+registry-us-map

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented May 18, 2026

Summary

New: Maps catalog section (6 blocks)

Block Description Libs
us-map Choropleth — population density by state D3 geoAlbersUsa + TopoJSON
us-map-bubble Proportional city markers over US base map D3 geoAlbersUsa + TopoJSON
us-map-hex Hexagonal tile grid — income by state GSAP only (no D3)
us-map-flow Animated connection arcs between cities D3 geoAlbersUsa + TopoJSON
world-map World choropleth — GDP per capita D3 Natural Earth + TopoJSON
spain-map Spain choropleth — GDP by comunidad autonoma D3 Conic Conformal + TopoJSON
  • New "Maps" group in catalog nav (docs/docs.json)
  • 6 MDX catalog pages with install instructions, details, and usage
  • Preview videos rendered and uploaded to S3

Engine fixes

Fix Root cause
Poll for sub-composition timeline readiness Renderer started capturing before async timelines (from fetch()) registered in window.__timelines
Preserve inner root element in producer Producer's inlineSubCompositions didn't pass flattenInnerRoot, so sub-composition #id selectors broke (the id attr was stripped but data-hf-authored-id was never set)
Force timeline rebind after async setup Runtime bound timelines before fetch() completed; seek() never reached late-registered timelines. New __hfForceTimelineRebind() resets binding state
Graceful missing video handling A 404'd video source caused a 45s timeout then hard crash. Now treats errored videos as ready and warns instead of throwing
Strip query strings from video src resolveProjectRelativeSrc looked for video.mp4?v=4 on disk literally. Now strips ? params before resolving
Deterministic stagger from: "random" in GSAP stagger caused visual jumps across parallel render workers (different Math.random() seeds per chunk). Replaced with from: "center"

Lint fix (closes #929)

Exempt type="importmap" and type="module" inline scripts from the invalid_inline_script_syntax rule. The rule used new Function() to parse, which rejects import statements and JSON import maps.

Test plan

  • bun run build passes
  • Engine tests pass (605/605)
  • Core tests pass
  • All 6 map blocks render correctly standalone
  • Gallery composition renders all 6 maps as sub-compositions
  • Missing video gracefully degrades (DoorDash repro)
  • ?v=4 query string video sources resolve correctly
  • type="importmap" and type="module" scripts pass lint
  • Preview videos uploaded to S3

Animated US population density choropleth using D3 geoAlbersUsa
projection with TopoJSON. Includes staggered state reveals, label
fade-ins, gradient legend, and highlight pulses on densest states.
New blocks: world-map (D3 Natural Earth choropleth), us-map-bubble
(proportional city markers), us-map-hex (hexagonal tile grid),
us-map-flow (animated connection arcs between cities).

All blocks share the same dark theme and are composable — layer
bubble or flow on top of the choropleth via track indexes.

Adds "Maps" section to the catalog docs with MDX pages for all 5
map blocks (including the us-map from the previous commit).
New block: spain-map — animated Spain choropleth by autonomous
community using D3 conic conformal projection with GDP per capita
data and red-to-amber color scale.

Also switches all map MDX preview URLs from S3 to local paths
so they render in mintlify dev without needing S3 upload first.
The renderer now waits for all sub-composition timelines to be
registered in window.__timelines before starting frame capture.
Previously only window.__hf root readiness was checked, causing
blank frames when sub-compositions use async data loading (fetch)
or when the headless renderer starts capturing before scripts
complete.

Adds pollSubCompositionTimelines() to both screenshot and
beginFrame render paths, with a diagnostic warning listing which
composition IDs are missing if the timeout expires.
…ions

The producer's inlineSubCompositions was not passing flattenInnerRoot,
causing sub-composition inner root elements to be unwrapped during
compilation. Scripts using #id selectors (rewritten to
[data-hf-authored-id] by the scoping proxy) could not find their DOM
root because the authored-id attribute was never set.

The core bundler (bundleToSingleHtml) already passed flattenInnerRoot
correctly. This aligns the producer's render compilation with the
same behavior: clone the inner root, strip timing/composition attrs,
replace id with data-hf-authored-id, and mark with
data-hf-inner-root.
…pture

Two fixes for compositions that register timelines after async data
loading (e.g. fetch for TopoJSON map data):

1. engine/frameCapture: remove the hosts.length <= 1 early return
   so the timeline readiness poll runs for ALL compositions, not just
   multi-composition galleries. Single-composition pages with async
   setup were silently skipped.

2. core/runtime/init: expose window.__hfForceTimelineRebind() which
   resets childrenBound and re-runs bindRootTimelineIfAvailable().
   The renderer calls this after all timelines are confirmed present,
   ensuring the root player discovers late-registered timelines from
   fetch callbacks.

Without these fixes, compositions using fetch() to load data at
runtime would render blank frames because the root player bound
timelines before the async setup completed, and seek() never
reached the unbound composition timeline.
…g render

Previously, a missing video file (404) caused the renderer to hard-fail
after a 45-second timeout waiting for readyState >= 2. Now:

1. pollVideosReady treats errored videos (v.error set or
   NETWORK_NO_SOURCE) as ready, so 404'd sources don't block
2. Screenshot mode downgrades the video timeout from a throw to a
   console.warn listing affected sources, then continues rendering
3. The composition renders with the missing video as a blank area
   instead of failing entirely
resolveProjectRelativeSrc now strips query parameters (e.g. ?v=4)
before joining with the project directory. Browsers ignore query
strings when loading local files, but the filesystem resolver was
looking for the literal path including the query — causing video
extraction to silently skip the file and render frozen first frames.
Assets are uploaded to static.heygen.ai — switch from local
/images/ paths back to the canonical S3 URLs for production docs.
- Replace from:"random" with from:"center" stagger in us-map,
  world-map, spain-map — random stagger is non-deterministic across
  parallel render workers, causing visual jumps at chunk boundaries.

- Exempt type="importmap" and type="module" inline scripts from the
  invalid_inline_script_syntax lint rule. The rule used new Function()
  to parse, which rejects import statements and JSON import maps.
  Closes #929.

- Cache-bust all map MDX preview video URLs after re-rendering with
  the deterministic stagger fix.
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

REQUEST CHANGES — bundled PR with three concern areas (6 map blocks + 5 engine/producer fixes + 1 lint rule). The engine fixes look solid; one blocker + one important on the catalog/repo hygiene side. Honest-scope framing below.

Audited

  • packages/engine/src/services/frameCapture.ts (+67/-3) — pollSubCompositionTimelines is bounded by pageReadyTimeout, calls __hfForceTimelineRebind only when ready=true, gracefully degrades to warn-and-continue on timeout. Integrated into BOTH the screenshot and beginframe initialization paths (Rule-2 coverage on the new contract ✓).
  • packages/engine/src/services/videoFrameExtractor.ts (+4/-2) — resolveProjectRelativeSrc strips ? query string before resolving against filesystem ✓; missing-video path warns once (deduped via warnedSrcs) and pushes a typed error rather than throwing ✓.
  • packages/producer/src/services/htmlCompiler.ts (+26/-0) — flattenInnerRoot callback matches htmlBundler.ts's pattern: strips composition attrs, preserves id as data-hf-authored-id, sets data-hf-inner-root="true". Wired into the producer's inlineSubCompositionsShared call ✓.
  • packages/core/src/lint/rules/core.ts (+5/-1) — regex extends the existing JSON-skip to importmap and module. Clean addition, narrow scope ✓.
  • packages/core/src/runtime/init.ts (+5/-0) — window.__hfForceTimelineRebind = () => { childrenBound = false; bindRootTimelineIfAvailable(); }. Minimal hook that lets the engine's poll-loop request rebind after async timeline registration ✓.
  • registry/registry.json — 6 new hyperframes:block entries (us-map, us-map-bubble, us-map-hex, us-map-flow, world-map, spain-map) ✓.
  • registry/blocks/us-map/us-map.html — within-file consistency check ✓: data-composition-id="us-map" matches window.__timelines["us-map"]. Uses from: "center" (deterministic stagger) instead of random ✓. Async fetch pattern that benefits from the new pollSubCompositionTimelines fix ✓.

Trusting

  • The other 5 map blocks (us-map-bubble, us-map-hex, us-map-flow, world-map, spain-map) — assumed they follow the same within-file consistency + determinism shape as us-map.html. Body claim says "Engine tests pass (605/605)" and CI is green on lint, which includes the timeline_id_mismatch rule.
  • 6 MDX docs under docs/catalog/blocks/ (template-style additions)
  • docs/docs.json nav update
  • Preview videos uploaded to S3 (didn't fetch the URLs)

Blocker

docs/public/catalog-index.json not regenerated.

Same footgun as hf#921. The 6 new map blocks are absent from the catalog index — I grepped for "us-map", "us-map-bubble", "us-map-hex", "us-map-flow", "world-map", and "spain-map" in the file and got zero hits. The catalog index is what powers the /catalog discovery page, so users visiting hyperframes.heygen.com/catalog won't see any of the 6 new blocks until this file is regenerated.

Fix: run scripts/generate-catalog-pages.ts (the same script the body's test plan reference). Verify all 6 entries appear in the output.

Important

3 stray node_modules/ files committed.

packages/producer/node_modules/.bin/esbuild
packages/producer/node_modules/@types/node
packages/producer/node_modules/esbuild

Each shows +1/-1 — looks like symlink target updates that accidentally got staged. The repo's .gitignore has node_modules/ at the root, so these should be untracked but somehow are. CI is still green, so this isn't blocking the build, but it pollutes the diff and means future bun install will create the same drift.

Fix: git rm --cached the three paths and ensure node_modules/ ignore applies to the producer package directory.

Notes (non-blocking)

  • The pollSubCompositionTimelines warning branch logs the missing IDs and continues. If a composition's async fetch genuinely fails (e.g. CDN down), the render will still produce frames but the affected sub-comp won't animate. That's the right tradeoff for "graceful degradation," but worth confirming the Continuing render warning is loud enough in the orchestrator logs to surface a partial-render flag for distributed plans.
  • The from: "center" stagger fix in us-map.html is correct for deterministic parallel renders. Worth a follow-up rubric note in the catalog-block authoring docs so future contributors don't reintroduce from: "random".

CI is green on the substantive checks (Build, Test, Typecheck, Lint, Format, Render catalog previews, regression, player-perf, all 5 Perf checks); Windows tests + 8 regression-shards still in_progress.

Happy to re-review once catalog-index.json is regenerated and the node_modules entries are removed.

— Rames

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

REQUEST CHANGES — +1 with @rames on the docs/public/catalog-index.json blocker and the packages/producer/node_modules/ symlink drift; both verified. Adding gaps Rames didn't surface, mostly on the engine-fix side. Honest framing: the engine changes are individually correct, but several quietly shift the failure contract from "throw → orchestrator sees" to "console.warn → orchestrator blind." Two of those compound.

Strengths (calibrated)

  • pollSubCompositionTimelines is bounded, calls __hfForceTimelineRebind only on ready=true, and is wired into BOTH the screenshot and beginFrame init paths (frameCapture.ts:553, 680). Rule-2 coverage on the new contract ✓.
  • inlineSubCompositions flattenInnerRoot callback (htmlCompiler.ts:590-602) matches the htmlBundler.ts:383-395 canonical shape exactly — data-hf-authored-id + data-hf-inner-root are both set so downstream [data-hf-authored-id] selectors keep working.
  • Stagger determinism: all 6 new blocks are clean of Math.random and from: "random"; 5 use from: "center", one uses from: "start". Verified by grep.

Important

1. Asymmetric video-failure observability between screenshot and beginFrame paths

frameCapture.ts:573-588 (screenshot) logs the failing video URLs on pollVideosReady timeout:

const failedVideos = await page.evaluate(...);
console.warn(`[FrameCapture] Some video elements did not decode within ${pageReadyTimeout}ms: ${failedVideos}. ...`);

frameCapture.ts:684-689 (beginFrame) calls the same poll and discards the return value entirely — no warning, no failed-URL list, nothing:

await pollVideosReady(page, session.options.skipReadinessVideoIds ?? [], ...);

The two paths now have different observability for the same failure mode. Mirror the screenshot path's warn-with-URL-list here, or extract a single helper that handles both.

2. Soft-degradation contract shifts: no orchestrator signal

The PR converts two previously-throwing readiness gates into console.warn-and-continue:

  • pollVideosReady timeout (frameCapture.ts:573) — old code threw [FrameCapture] video first frame not decoded after ${pageReadyTimeout}ms. New code warns. A render with a 404'd customer video now produces black frames in the output instead of failing the job.
  • pollSubCompositionTimelines timeout (frameCapture.ts:397-412) — same pattern: warns, continues. Sub-comp won't animate, but render completes.

The browser-side videoFrameExtractor already has a typed errors.push({ videoId, error }) channel (lines 606/611/681/716/781) that bubbles back to the caller. The new screenshot-path warn-and-continue does NOT route into any such channel — just stdout. So:

  • ffmpeg-side video failure → typed error in result
  • chromium-side video failure (new path) → console.warn only

A customer or orchestrator has no programmatic way to detect "the render finished but 3 video clips were black." Surface partial-render failures through the same error channel extractAllVideoFrames uses, or at minimum return a partialFailures: [] field on the session result so callers can gate downstream actions (encoding, S3 upload, billing).

This is intentional graceful degradation per the PR body (DoorDash 404 case), but the granularity needs to be "the orchestrator knows" not just "the log file knows."

3. resolveProjectRelativeSrc query-strip fix is incomplete on escaped paths

videoFrameExtractor.ts:515-518:

const qIdx = src.indexOf("?");
const cleanSrc = qIdx >= 0 ? src.slice(0, qIdx) : src;
const fromCompiled = compiledDir ? join(compiledDir, cleanSrc) : null;
const fromBase = join(baseDir, cleanSrc);

The first two candidates strip the query. But the escape-fallback block below (line 534) uses the un-stripped src:

const normalized = posix.normalize(src.replace(/\\/g, "/"));
const stripped = normalized.replace(/^(\.\.\/)+/, "");

For src = "../assets/foo.mp4?v=4", cleanSrc = "../assets/foo.mp4", fromBase escapes the project root → fallback fires. stripped = "assets/foo.mp4?v=4"join(baseDir, "assets/foo.mp4?v=4") never exists on disk. The fallback path now silently misses any escaped src with a query.

Strip the query once at the top, then derive everything (including the normalize/stripped arm) from cleanSrc.

4. prepareFlattenedInnerRoot now duplicated in three modules

The same FLATTENED_INNER_ROOT_STRIP_ATTRS constant + clone-strip-set helper exists in:

  • packages/core/src/compiler/htmlBundler.ts:370-394
  • packages/core/src/runtime/compositionLoader.ts:77-89
  • packages/producer/src/services/htmlCompiler.ts:561-602 (this PR)

Three independent copies that have to stay in lockstep — drift in any one breaks [data-hf-authored-id] selectors silently. Export prepareFlattenedInnerRoot (or at least FLATTENED_INNER_ROOT_STRIP_ATTRS) from @hyperframes/core/compiler and consume it everywhere. The compositionLoader version uses document.importNode for cross-document ownership — that variation is the only reason to keep two impls, and it can be parametrized.

5. CDN-fetched topojson with no integrity + no .catch() interacts badly with the new warn-and-continue contract

All 5 fetch-using map blocks (us-map, us-map-bubble, us-map-flow, world-map, spain-map) pull TopoJSON from cdn.jsdelivr.net. None has a .catch() on the fetch:

$ grep -nE "\.catch\(|fetch\(" registry/blocks/{us-map,us-map-bubble,us-map-flow,world-map,spain-map}/*.html
# 5 fetches, 0 .catch handlers

And the inline <script src="https://cdn.jsdelivr.net/npm/d3@7/..."> tags carry no integrity= attribute, and d3@7 is unpinned to minor — a d3@7.9.0d3@7.9.1 jdelivr mid-flight could produce non-deterministic chunk renders (planHash contract for parallel workers).

Combined with finding (2) above, a CDN hiccup now silently produces a partial render with no signal to the orchestrator. At minimum: pin d3@7.x.y to a patch version, add SRI hashes, and have each block's fetch .catch() register the timeline as empty (or with an error sentinel) so the engine's poll completes deterministically.

This is also somewhat inconsistent with the team's stated asset-hosting pattern (registry/ is for installable assets that ship with the customer's project for render determinism).

Nit

  • (window as any).__hfForceTimelineRebind in init.ts:967 — the rest of the file uses const runtimeWindow = window as Window & { ... }; (line 30). Match the pattern for the new field.
  • pollSubCompositionTimelines queries [data-composition-id] — this also matches the root composition, not just sub-comps. Works today because the root timeline registers via the existing window.__hf.duration > 0 gate before this poll runs, but the function name implies sub-comp scoping while the selector is broader. Either rename or add :not([data-root="true"]).
  • Lint rule extension at core.ts:160-163 matches only quoted type="module". Unquoted type=module would still trip the rule. Pre-existing pattern, not blocking; consider a follow-up to invert the check to allow-list text/javascript/empty/absent instead.

CI

statusCheckRollup failed-job names: 0 failed. Pending: Render on windows-latest, Tests on windows-latest, all 8 regression-shards shards. Non-decisive.

Verdict

Verdict: REQUEST CHANGES
Reasoning: +1 with @rames on the catalog-index regeneration + node_modules cleanup. Independently, the engine-fix bundle quietly shifts two readiness gates from throw → console.warn without an orchestrator-visible signal (findings 1 and 2), and that compounds with the unpinned-CDN pattern (finding 5) into a silent-broken-render risk. The duplicated flattenInnerRoot (finding 4) is unfinished extraction work. None are individually hard — the right next push is one round of polish, not a rework.

Review by Vai

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

Acknowledging three substantive misses caught by @vai. Verdicts converge on REQUEST_CHANGES with complementary findings — Vai went deep on engine semantics, I went broad on catalog/hygiene.

My misses worth flagging explicitly:

  1. resolveProjectRelativeSrc query-strip is incomplete at the escape-fallback branch (Vai's #3). I read the function, saw cleanSrc = src.slice(0, qIdx) at the top, and assumed all downstream paths used cleanSrc. The escape-fallback branch re-derives from the un-stripped src via posix.normalize(src.replace(/\\/g, "/")). So ../foo.mp4?v=4 still fails to resolve. Rule-2 within-function branch coverage miss — when I audit a function with an entry-point sanitization, I need to verify every branch uses the sanitized value, not just the happy path.

  2. prepareFlattenedInnerRoot is duplicated across 3 modules (Vai's #4). I checked that the producer's htmlCompiler.ts flattenInnerRoot callback matches htmlBundler.ts's pattern. I missed that compositionLoader is a third site. That's exactly the canonical-adapter-pair gotcha from prior reviews (htmlBundler ↔ studioServer ↔ snapshot.ts patterns) — when a callback shape is replicated across N modules, an audit needs to enumerate ALL N sites, not just the new+old pair the diff appears to compare.

  3. Asymmetric video-failure observability (Vai's #1) — screenshot path logs failed URLs at frameCapture.ts, beginFrame path discards the return value at lines 684-689. I audited pollSubCompositionTimelines for both paths (Rule-2 ✓) but didn't extend the same enumeration to pollVideosReady's logging behavior. Same "audit every path that runs the contract" lesson, applied to logging surfaces this time.

Combined with my own blocker (catalog-index.json regen) + Vai's substantive findings, the change-request list for re-review:

  • Regenerate docs/public/catalog-index.json to include the 6 new map blocks
  • Remove the 3 stray packages/producer/node_modules/ symlinks
  • Fix resolveProjectRelativeSrc escape-fallback branch to use cleanSrc
  • Extract prepareFlattenedInnerRoot to a single shared helper across the 3 modules
  • Promote pollVideosReady + pollSubCompositionTimelines warn-and-continue to a typed orchestrator-visible signal (matching videoFrameExtractor's errors.push channel)
  • Symmetrize video-failure logging in the beginFrame path
  • Pin d3@7 to a specific minor + add integrity= hashes to CDN-fetched topojson assets
  • Nits per Vai's note (the (window as any) cast + the [data-composition-id] root-vs-host selector)

I'll defer to Vai as primary on re-review since the engine-side substantive findings are the load-bearing changes here.

— Rames

…alog

Review items addressed:

1. Mirror video-failure warning in beginFrame path (was screenshot-only)
2. Fix resolveProjectRelativeSrc escape-fallback to use query-stripped
   cleanSrc instead of raw src for the normalize/strip arm
3. Export prepareFlattenedInnerRoot from @hyperframes/core/compiler and
   consume in the producer instead of duplicating the implementation
4. Use typed Window cast instead of (window as any) for __hfForceTimelineRebind
5. Regenerate docs/public/catalog-index.json with all 6 map blocks
6. Restore Maps nav group in docs.json (catalog generator had merged
   them into Data)
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

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

APPROVE on re-review. All findings from round 1 are addressed in commits cf2addf (node_modules cleanup) and 0f1c64d (review feedback bundle).

Verified at HEAD 0f1c64d

My round-1 items:

  • docs/public/catalog-index.json regenerated. All 6 map entries present: us-map, us-map-bubble, us-map-flow, us-map-hex, world-map, spain-map. Each has name, type: "block", title, description, tags, href, preview. Total entries went from 71 → 77 (6 new + caption-highlight which was already on main).
  • packages/producer/node_modules/ symlinks removed. Three [removed] +0/-1 entries in the file list — .bin/esbuild, @types/node, esbuild are gone from the diff.

Vai's round-1 items I acknowledged:

  • resolveProjectRelativeSrc escape-fallback uses cleanSrc. Line ~534: const normalized = posix.normalize(cleanSrc.replace(/\\/g, "/")). Query-strip now applied across both happy path and escape-fallback. ../foo.mp4?v=4 resolves correctly.
  • prepareFlattenedInnerRoot extracted to @hyperframes/core/compiler. Producer's htmlCompiler.ts now imports + uses it directly: flattenInnerRoot: prepareFlattenedInnerRoot as (innerRoot: Element) => Element. The inline duplicate is gone. packages/core/src/compiler/index.ts adds the export (+6/-1). Refactor opportunity from round 1 is closed.
  • Beginframe path now mirrors screenshot path's video-decode warning. Grep confirms "Some video elements did not decode" now appears 2× (was 1×) in frameCapture.ts. Asymmetric-observability finding closed.
  • (window as any) replaced with typed Window cast per commit message.

CI

Required checks green: Build, Test, Typecheck, Lint, Format, CLI smoke, CodeQL, regression, player-perf, all 5 Perf checks, preview-regression, Render catalog previews. Windows tests + 8 regression-shards still in_progress on the last run — not regressions.

Deferred (worth a follow-up, not blocking)

Three items from Vai's round-1 review weren't in this commit's punch list:

  • Vai's #2 (typed orchestrator-visible signal for pollVideosReady / pollSubCompositionTimelines instead of console.warn)
  • Vai's #5 (pin d3@7 to a specific minor + add integrity= to CDN-fetched topojson assets — planHash drift risk for chunk renders)
  • Vai's nit on [data-composition-id] root-vs-host selector

These are hardening / supply-chain improvements rather than functional fixes; reasonable to land in a follow-up. Flag to track them on the next maps-related touch.

— Rames

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Re-review of 0f1c64dc against the prior REQUEST CHANGES at cf2addfb. 4 of 6 substantive findings addressed; the two open items are tracked below as follow-ups, not re-blockers.

Findings status

  • #1 asymmetric video-failure observabilityAddressed. frameCapture.ts:691-702 now mirrors the screenshot path's failedVideos evaluate + warn block in the beginFrame path. Both paths emit the same [FrameCapture] Some video elements did not decode within ${timeout}ms: ${urls} line.
  • #2 soft-degradation has no orchestrator signalOpen / follow-up. The two console.warn-and-continue gates (pollVideosReady failure, pollSubCompositionTimelines failure) still don't surface a typed partial-failure into the session result. With the symmetric logging in (#1), a CI/log-scrape can detect this — but a programmatic orchestrator can't. Worth a follow-up that adds e.g. partialFailures: { videoUrls: string[]; subCompTimelines: string[] } on the session result so encoders/billers can gate. Not blocking this PR.
  • #3 resolveProjectRelativeSrc escape-fallbackAddressed. videoFrameExtractor.ts:533 now uses cleanSrc (query-stripped) for the normalize / stripped arm, so ../assets/foo.mp4?v=4 resolves through the fallback path.
  • #4 prepareFlattenedInnerRoot 3-way duplicationAddressed (2 of 3). prepareFlattenedInnerRoot + FLATTENED_INNER_ROOT_STRIP_ATTRS are now exported from @hyperframes/core/compiler (packages/core/src/compiler/index.ts:21-25) and the producer imports them (packages/producer/src/services/htmlCompiler.ts:26, 580), with 27 lines deleted. compositionLoader.ts:64,77 still holds its own copy because of the document.importNode cross-document variation — that was the carve-out I flagged in the original review, so this is effectively complete.
  • #5 CDN topojson integrity + d3 pinOpen / follow-up. d3@7, us-atlas@3, world-atlas@2, es-atlas@0.6.0 still unpinned to a patch / no integrity= / no .catch() on the 5 fetches (registry/blocks/{us-map,us-map-bubble,us-map-flow,world-map,spain-map}/*.html). Same pattern as the other blocks shipping the CDN dependency, so not net-new technical debt — but combined with the warn-and-continue (#2), a jsdelivr blip can still produce a silently-incomplete render. Worth a sweep across all blocks in a follow-up.
  • #6a window as any nitAddressed. init.ts:967 is now (window as Window & { __hfForceTimelineRebind?: () => void }), matching the file's pattern.
  • #6b [data-composition-id] selector matches root nitNot addressed. Still works because the root timeline registers before this poll runs. Pre-existing nit, not blocking.

CI

statusCheckRollup failed-job names: 0 failed. Pending (not yet decisive): Render on windows-latest, Tests on windows-latest, regression-shards shards 1-8. All required required-on-merge checks (Build, Typecheck, Test, Lint, CLI smoke (required), Preflight (lint + format), player-perf, preview-regression, CodeQL) currently SUCCESS at 0f1c64dc.

Verdict

Verdict: APPROVE
Reasoning: 4 of 6 substantive findings addressed at the file:line level; the remaining two (#2 typed partial-failure channel, #5 CDN integrity/pin sweep) are real but are follow-up shape, not blockers — they don't regress anything this PR introduced, and #1's symmetric logging makes #2 detectable via log scrape in the interim. Recommend the author file two follow-up tickets for #2 and #5 before this gets too far in the rear view.

Review by Vai (re-review)

…lines

1. Only call __hfForceTimelineRebind() when the timeline poll actually
   had to wait (pollDuration > 2 intervals). For compositions with
   synchronous timeline registration, the rebind was unnecessary and
   shifted render timing, causing PSNR regressions in chat and
   gsap-letters-render-compat.

2. Regenerate compiled.html baselines for missing-host-comp-id and
   overlay-montage-prod to match the new flattenInnerRoot behavior
   (data-composition-id stripped from inlined inner roots, replaced
   with data-hf-authored-id).

3. Add late-bind polling to runtime init.ts — after external
   compositions load, poll for 5s to detect async timelines that
   register after initial binding (e.g. from fetch callbacks).
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.

linter does not accept script of inline type="importmap" or inline type="module"

3 participants