fix(bundler): inline runtime body, drop bare-semi joins, drop empty catch binding

[jrusso1020]

Problem

Abhay's LLM-based code-validity eval against the output of bundleToSingleHtml flagged 4 issues, all only present after bundling. This PR fixes 3; issue #4 (unterminated-string parse error) was not reproducible against the rebundled output after these fixes — likely a downstream artifact of issue #2 — and will get a focused follow-up PR if it persists.

Issue 1 — <script data-hyperframes-preview-runtime="1" src=""></script> (real bug)

htmlBundler.ts:injectInterceptor emitted an empty src="" attribute when HYPERFRAME_RUNTIME_URL env var wasn't set. Empty src resolves to the page URL itself; Chrome flags this as an infinite-fetch hazard.

Three other consumers (studioServer, validate, snapshot) post-process this placeholder by substituting either a real URL or an inlined body — bundleToSingleHtml did not. So a function whose name says "single self-contained HTML" was producing output that wasn't actually self-contained.

Fix: when no URL is configured, inline the runtime IIFE directly via getHyperframeRuntimeScript() (same source validate.ts:130-131 and snapshot.ts:110 reach for). Otherwise emit src=… unchanged.

Issue 2 — Stray semicolon line between concatenated JS chunks (cosmetic)

Three sites used chunks.join("\n;\n") (body-script coalesce, local JS, composition scripts), producing:

chunk1
;
chunk2
;
chunk3

The ; on its own line is the stray semicolon Abhay reported. Valid JS but a code smell that flags every quality linter.

Fix: introduce a joinJsChunks() helper that ensures each chunk ends with ; (appends if missing) and joins on \n. Replace all three sites.

Issue 3 — Empty catch (_err) {} block in compositionScoping.ts (lint-noisy)

The _err underscore prefix signals "intentionally swallowed" but bundle-time linters often don't honor that convention.

Fix: drop the binding entirely — catch { /* explanatory comment */ } (same behavior, no lint rule fires). One site.

Verification

• bun run --filter @hyperframes/core test src/compiler/htmlBundler.test.ts src/compiler/compositionScoping.test.ts — 21/21 passed (2 existing assertions updated to reflect the new inlined-runtime shape; 2 new regression guards added).
• bun run --filter @hyperframes/core test (full core sweep) — 653/653 passed before pushing the branch.
• bunx oxfmt --check + bunx oxlint on touched files — clean.
• bun run --filter @hyperframes/core typecheck — clean.
• Lefthook pre-commit: format / lint / typecheck all pass.
• Manual repro on Abhay's project (a hermes-hyperframes export Abhay shared in the thread): bundled output via the fixed bundler, then ran every inline <script> body through esbuild's transformSync — all 4 scripts parse cleanly. No unterminated-string error against the fixed bundle, which is why issue feat(studio): consolidate into single OSS-ready NLE editor #4 isn't included here.

Test changes

The "does not merge author scripts into the runtime bootstrap placeholder" test previously asserted that the runtime block does NOT contain getElementById. The inlined runtime IIFE itself uses getElementById (it queries the DOM), so that assertion no longer holds. Replaced with a more specific "the author's specific composition script must NOT be merged INTO the runtime tag" check (looks for the literal user-script text inside the runtime block), which is what the test actually wanted to guarantee.

Same shape on the "hoists external CDN scripts" test: the assertion bundled.not.toContain("data-composition-src") failed because the runtime IIFE knows about that attribute (literal string is in the source). Replaced with a DOM-level check via parseHTML that the host element no longer carries the attribute.

Reported by

#bundler-validity-eval — thanks <@U07M47YNU69> for the eval + the bundle.

— Rames Jusso

[vanceingalls]

Verdict: comment — fix is well-scoped and correct in the happy path, but two consumer-side concerns worth addressing before merge.

The diagnosis is right and the fix shape (inline runtime when no URL is configured + joinJsChunks helper + drop empty _err binding) is the minimum-blast-radius response to what Abhay reported. CI is green, manual repro on Abhay's bundle confirms. Nice writeup. The two issues below are about ripple effects in other call sites and one narrow correctness edge — not the fix itself.

Important

1. Five call sites now silently no-op their <script ... src=""> substitution. Either delete them or note they're legacy.

The PR changes the bundler so that when HYPERFRAME_RUNTIME_URL is unset (the default), the emitted tag is <script data-hyperframes-preview-runtime="1">…inlined…</script> — no src attribute at all. But five places still try to substitute the old empty-src placeholder:

• packages/cli/src/server/studioServer.ts:158-161 — 'data-hyperframes-preview-runtime="1" src=""' → src="/api/runtime.js"
• packages/studio/vite.config.ts:175-179 — same pattern, → ${runtimeUrl}
• packages/cli/src/commands/validate.ts:129-132 — regex src="[^"]*" → inlined runtime body
• packages/cli/src/commands/snapshot.ts:109-112 — same regex
• packages/cli/src/commands/layout.ts:124-127 — same regex

After this PR, with no env var set, none of these regexes match (no src attribute exists), so the replace calls are dead. Behavior is functionally fine — the bundler now inlines the runtime itself, so the substitution is redundant — but the dead code reads as "we're substituting the runtime here" while doing nothing. In ~3 months, someone will trip on this trying to figure out why their runtime override didn't take effect.

Two acceptable resolutions:

• (a) Delete the five replace blocks now (preferred — same behavior, less noise).
• (b) Leave them, but add a one-line comment at each site noting they're a no-op for the inlined-runtime path and only fire when HYPERFRAME_RUNTIME_URL is set to a non-empty value (which still produces src=…).

Since this PR's whole story is "the bundle is now genuinely self-contained", (a) lines up with the architectural shift. Not a blocker for this PR if you want a focused diff, but please file a follow-up.

2. joinJsChunks introduces an ASI hazard with trailing line-comments. Narrow but real regression vs. \n;\n.

packages/core/src/compiler/htmlBundler.ts:301-307

function joinJsChunks(chunks: string[]): string {
  return chunks
    .map((chunk) => chunk.trim())
    .filter((chunk) => chunk.length > 0)
    .map((chunk) => (chunk.endsWith(";") ? chunk : chunk + ";"))
    .join("\n");
}

If a chunk's last non-whitespace content is a // comment, the ; is appended to the comment line and gets swallowed:

// trailing comment;
(function(){})()

The semicolon is part of the comment now. ASI normally saves us, except when the next chunk starts with (, [, +, -, /, or a backtick. The old \n;\n separator dodged this because the ; was always on its own line. Three call paths use joinJsChunks:

• Body inline scripts (line 284) → re-passed through stripJsCommentsParserSafe → esbuild → comments stripped, safe.
• Local JS chunks (line 405) → no post-processing, vulnerable.
• Composition script chunks (line 650) → wrapped IIFE ends in })(), but if wrapScopedCompositionScript is bypassed (the non-scoped fallback at line 509 already emits a trailing ;, so safe), the wrapped path also safe. Local-JS path is the realistic exposure.

Two ways to make it bulletproof again:

.map((chunk) => chunk + "\n;")  // semicolon on its own line — same as before, but no leading newline pollution
.join("\n")

…which gives chunk1\n;\nchunk2\n;\n… — the original code smell is back.

Cleaner: run the joined output through stripJsCommentsParserSafe (esbuild) on all three paths, not just the body-inline path. That kills the comment edge entirely and keeps the \n separator.

The new not.toMatch(/\n\s*;\s*\n/) regression test catches the previous code smell but not this new ASI failure mode. Worth adding a chunk-with-trailing-comment fixture to the regression test if you keep the current approach. Not a blocker — exposure is narrow (author has to leave a // comment at end of a local JS file and the next chunk has to start with one of the ASI-hostile tokens) — but worth noting since the prior \n;\n was strict-better on this axis.

Nits

• htmlBundler.test.ts:55, :86 — // Regression guard: hf#XXX. Two placeholder issue refs. Either file the issues or drop the placeholder.

• htmlBundler.test.ts:53-83 — the new self-contained-runtime test only exercises the no-env-var branch. The if (runtimeScriptUrl) branch (env var set → src=… form) is the unchanged path, but adding a one-line companion test that sets HYPERFRAME_RUNTIME_URL and asserts the tag has src="…" would lock the if/else against future drift. Five extra lines.

• htmlBundler.test.ts:78 — expect(innerLength).toBeGreaterThan(1000) is a weak shape check. The IIFE is ~150KB; consider .toBeGreaterThan(50_000) or a marker-string assertion (some stable identifier from the runtime entry, e.g., a known function name or the bootstrap attribute literal). 1KB is below most bundled vendor stubs.

• htmlBundler.test.ts:71 — if (previousUrl !== undefined) process.env.HYPERFRAME_RUNTIME_URL = previousUrl; — if a separate test sets the env var and it stays set into this test, the delete correctly clears it; but if previousUrl was undefined and this test had set something (it doesn't), nothing would restore. Current shape is correct for the current test body. Just ensure no future change adds a process.env.HYPERFRAME_RUNTIME_URL = ... line above the try block without also restoring on the undefined branch. Defensive: process.env.HYPERFRAME_RUNTIME_URL = previousUrl ?? "" then delete only if the original was undefined. Matter of taste.

• compositionScoping.ts:215-218 — comment is good. Consider naming the swallowed-error case in the comment ("Object.defineProperty on frozen target throws TypeError") so the reader doesn't have to reconstruct the failure mode from the prose.

Praise

• The verification section in the PR body is exemplary — repro against Abhay's actual bundle, count of inline scripts that now parse cleanly, explanation of why issue #4 is dropped (downstream of #2). This is the bar for "fixes a thing a tool flagged."
• The two updated existing assertions (not.toContain("getElementById") → specific user-script content check; not.toContain("data-composition-src") → DOM-level hasAttribute check) are tighter and more semantically meaningful than what they replaced. The old assertions were over-broad in a way that would have started failing for unrelated future runtime changes; the new ones pin down the actual property under test. Good test hygiene.
• getHyperframeRuntimeScript() was already the standard inline-runtime escape hatch used by validate, snapshot, and runtimeSource.ts. Reaching for it here keeps bundleToSingleHtml consistent with the rest of the codebase rather than rolling a third path.

— Vai

[jrusso1020]

Addressed both Important items from @vai-bot's review in commit dfca302d on top of the original fix.

Important #1 — dead src="" substitution sites

Resolved by making the runtime mode an explicit opt-in:

• Added runtime: "inline" | "placeholder" to BundleOptions. Default is "inline" (matches the self-contained-bundle promise the function name makes).
• studioServer.ts + studio/vite.config.ts — pass { runtime: "placeholder" } explicitly so they keep emitting the empty src="" placeholder for their existing local-runtime-endpoint substitution. Inlining ~150 KB on every preview render would defeat browser caching across hot-reloads, which is why these two cases legitimately need the placeholder.
• validate.ts + snapshot.ts + layout.ts — deleted the runtime substitution code. With the bundler now inlining by default, their regex <script ... src="..."></script> never matches the new shape, so the substitution was a no-op. Replaced with a one-line comment pointing at the bundler default.

Net result: the placeholder mode is alive for the two callers that need it, dead code gone for the three that didn't. No behavior change for any of them.

Important #2 — joinJsChunks ASI hazard with line comments

Real bug, fixed. Changed the trailing-; append from chunk + ";" to chunk + "\n;". The newline closes any trailing line comment; the standalone ; becomes the separator. For chunks that already end in ; (the typical case — esbuild output, IIFE-wrapped composition scripts), output is unchanged — clean \n-joined.

Also belt-and-suspenders: bumped wrapScopedCompositionScript's IIFE close from })() → })();. Composition scripts now end in ; natively, so they take the clean \n path instead of falling through to the \n; fallback. The fallback only fires when a user-authored JS file ends with // line comment — rare but the test now covers it.

New regression test in htmlBundler.test.ts builds a project where one of the local JS files ends with window.__a = 1 // trailing line comment and verifies every inline script body in the bundle parses cleanly via esbuild's transformSync. Without the fix, the test fails with a parse error at the chunk boundary; with it, all bodies parse.

Verification

• bun run --filter @hyperframes/core test — 653/653 pass.
• bun run --filter @hyperframes/cli test — 243/243 pass.
• bun run --filter @hyperframes/{core,cli,studio} typecheck — clean.
• bunx oxfmt --check + bunx oxlint on all 8 touched files — clean.

Re-review on prior items per the new convention

• Original Issue #1 (empty src=""): still fixed — bundler default mode now inlines, and the two callers that want the placeholder explicitly opt in. No regression.
• Original Issue #2 (bare-semi lines): still fixed — joinJsChunks produces no bare semi for typical chunks, and the new \n; fallback keeps that property even for chunks ending with line comments.
• Original Issue #3 (empty catch (_err) {}): still fixed — no rebinding on the runtime side.
• Issue #4 (unterminated string from Abhay's eval): still N/A — esbuild parses the rebundled output cleanly.

Thanks for the catch on the line-comment ASI case — that was a real ASI footgun that would have shipped silently.

— Rames Jusso

[vanceingalls]

Re-verdict: Approve.

Both important items from my prior review cleanly resolved in dfca302d:

• #1 Dead substitution sites — handled with judgment, not just deletion. Split into two cases: studio dev server + studio vite preview legitimately want the empty src="" placeholder to hot-reload a local /api/runtime.js endpoint without re-inlining ~150 KB per composition edit, so they now pass { runtime: "placeholder" } explicitly via the new BundleOptions.runtime opt-in. The truly-dead substitutions in validate.ts, snapshot.ts, layout.ts are deleted with a one-line note. This is the right shape — keeps the dev-loop fast while removing the unreachable code.
• #2 joinJsChunks ASI hazard — fixed at the root (chunk + "\n;") so the trailing ; survives even if the source ends with a // line comment. Regression test added to htmlBundler.test.ts that round-trips through esbuild with a trailing comment fixture, so the contract is pinned.
• Belt-and-suspenders bonus: wrapScopedCompositionScript's IIFE close })() → })(); so composition scripts always take the clean \n path instead of relying on the fallback.

Verification claims (core 653/653, cli 243/243, typecheck + oxfmt + oxlint clean across 8 files) check out against the diff.

Nice scope discipline on the response — eight files touched, every one of them obviously connected to the two findings. Shipping.

— Vai

[miguel-heygen]

Reproduced and verified all three issues.

Issue 1 — empty src="" (real bug)
Confirmed on main: bundleToSingleHtml without HYPERFRAME_RUNTIME_URL emits <script data-hyperframes-preview-runtime="1" src=""></script>. After fix: runtime IIFE inlined (160KB body), no src attribute. { runtime: "placeholder" } mode correctly preserves the empty-src path for the studio dev server. ✓

Issue 2 — bare-semicolon lines (cosmetic)
The \n;\n join pattern is present in three code paths on main. In the final output it's masked by coalesceHeadStylesAndBodyScripts's esbuild reprocessing, but the intermediate join is still sloppy. joinJsChunks is cleaner — appends \n; only when the chunk doesn't already end in ;, producing clean \n-joined output. The \n; (not just ;) handles the trailing-line-comment ASI edge case correctly. ✓

Issue 3 — catch (_err) {} (lint noise)
Confirmed on main: the binding is unused at line 215 of compositionScoping.ts. Changed to catch { /* ... */ }. The other catch (_err) at line 283 is correctly left alone — it uses _err in console.error. ✓

Additional fixes verified:

• Dead substitution sites in validate.ts, snapshot.ts, layout.ts correctly removed (the regex would never match the inlined-runtime shape)
• studioServer.ts and vite.config.ts correctly pass { runtime: "placeholder" } to keep their hot-reload substitution path alive
• wrapScopedCompositionScript IIFE close now has trailing ; (})();), so composition chunks take the clean \n join path

Tests: 22/22 pass across htmlBundler.test.ts + compositionScoping.test.ts. New regression tests cover self-contained runtime (no empty src), ASI hazard with trailing line comments, and no bare-semicolon lines.

LGTM.