Skip to content

feat(lint): font loading + invalid capture path composition rules#989

Open
ukimsanov wants to merge 1 commit into
feat/capture-pipeline-improvementsfrom
feat/lint-rules
Open

feat(lint): font loading + invalid capture path composition rules#989
ukimsanov wants to merge 1 commit into
feat/capture-pipeline-improvementsfrom
feat/lint-rules

Conversation

@ukimsanov
Copy link
Copy Markdown
Collaborator

What

Two new composition lint rules catching real failure modes from the website-to-video eval. Total lint suite goes from 148 to 151 tests; all 151 pass.

3 of 5 in the pipeline-quality stack. Stacked on #988.

Why

Both rules were chosen because the failure modes recurred across multiple eval rounds:

  • Google Fonts CDN — Sub-agents reflexively wrote <link href="https://fonts.googleapis.com/css2?..."> in compositions. Works locally; fails in sandboxed/offline renders, adds latency, and isn't deterministic.
  • Missing @font-face — CSS used a font-family with no declaration and no entry in the auto-bundled set. Text silently fell back to system-ui. Visual fidelity loss across multiple v2 videos (Mercury fonts, GeistMono, "Inter Variable").
  • ../capture/ paths — Sub-compositions live in compositions/ but are served with the project root as their base URL. <img src="../capture/logo.svg"> works on disk but 404s in Studio and renders. Four of six framer sub-agents got this wrong; mercury had four of five.

How

packages/core/src/lint/rules/fonts.ts (new)

  • google_fonts_import — warning. Detects <link> to fonts.googleapis.com and @import url(...fonts.googleapis.com...). Fix hint points to root-relative capture/assets/fonts/...woff2 with a local @font-face declaration.
  • font_family_without_font_face — warning. Checks every used family against @font-face declarations + the auto-bundled font set. Fix hint also points to root-relative capture/assets/fonts/.

packages/core/src/lint/rules/composition.ts

  • invalid_capture_path — error. Counts ../capture/ matches across the entire file (HTML + CSS + inline scripts) and reports the total. Registry source files and installed blocks are exempted via the existing isRegistrySourceFile / isRegistryInstalledFile guards.

Tests

  • fonts.test.ts (new) — 11 tests covering Google Fonts in <link> and @import, undeclared families, auto-bundled families staying clean.
  • composition.test.ts — three new cases for invalid_capture_path: <img> triggers, multi-occurrence url()s counted, root-relative capture/ clean.

WiringhyperframeLinter.ts runs the new fonts rules alongside the existing rule set.

Test plan

  • Unit tests added/updated — 151/151 passing (was 148).
  • Manual testing performed — typecheck clean; lint on a sub-composition with ../capture/ paths produces the expected error.
  • Documentation updated (if applicable) — fix hints in the rules themselves point at the correct root-relative paths; skill prose in PR feat(studio): consolidate into single OSS-ready NLE editor #4 of the stack updates step-5-build.md with the rule-aware guidance.

Copy link
Copy Markdown
Collaborator Author

ukimsanov commented May 20, 2026

ukimsanov added a commit that referenced this pull request May 20, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
ukimsanov added a commit that referenced this pull request May 20, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
ukimsanov added a commit that referenced this pull request May 20, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
ukimsanov added a commit that referenced this pull request May 20, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
ukimsanov added a commit that referenced this pull request May 20, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
@ukimsanov ukimsanov requested a review from Copilot May 20, 2026 23:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

Clean lint rules. google_fonts_import catches both and @import. font_family_without_font_face correctly skips generics, producer-bundled fonts, and font-family inside @font-face blocks. invalid_capture_path scoped correctly — only flags ../capture/ traversal. Tests comprehensive (12 cases).

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 at 06e71a39. Magi covered the test count (148 → 151) + producer-bundled-font / generic-family skip lists. Additive:

  • invalid_capture_path rule scope — counts ../capture/ matches across HTML + CSS + inline scripts and reports total. Registry source files + installed blocks exempted via existing isRegistrySourceFile / isRegistryInstalledFile guards. Worth confirming: are there legitimate top-level project files (e.g., a tools/ script) that reference ../capture/ for off-line preview reasons? If yes, they'd hit this rule. Counter-evidence: this is a lint rule (not a build error), so it surfaces as feedback rather than gating, mitigating false-positive damage.

  • google_fonts_import rule fix-hint — points to capture/assets/fonts/...woff2 (root-relative). One thing to verify against hf#988's capture pipeline: does capture/assets/fonts/ actually land at the root-relative path the fix hint claims, OR is it under capture/extracted/fonts/? Cross-PR consistency between the rule's fix-hint and the capture pipeline's actual output directory will save users an "I followed the hint and it 404'd" moment.

  • font_family_without_font_face rule — auto-bundled font set check is solid. The fallback-to-system-ui silent failure was a real eval-blocker; this catches it at lint time. ✓

  • Test coverage — 11 new tests for fonts + 3 for the capture-path rule. Both rules have explicit positive AND negative cases (e.g., auto-bundled families stay clean) — that's the right shape for a lint rule.

Stack base correctly set to feat/capture-pipeline-improvements.

— Rames Jusso

ukimsanov added a commit that referenced this pull request May 21, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
Two new composition lint rules catching failure modes that recurred
across the 11-round website-to-video eval. Both ship with vitest
coverage; total lint suite goes from 148 to 151 tests.

**`fonts.ts` (new) — two warnings**

- `google_fonts_import`: composition loads fonts from
  `fonts.googleapis.com` via `<link>` or `@import url(...)`. External
  font requests fail in sandboxed/offline renders and add latency.
  Fix hint points to root-relative `capture/assets/fonts/...woff2`
  with a local `@font-face` declaration.
- `font_family_without_font_face`: CSS uses a font-family that
  isn't declared with `@font-face` and isn't in the auto-bundled
  font set (Inter, JetBrains Mono, etc.). Text would silently fall
  back to system-ui — the visual fidelity loss the eval kept hitting.
  Fix hint points to the captured woff2 files.

**`composition.ts` invalid_capture_path (new) — one error**

Sub-compositions live in `compositions/` but get served with the
project root as their base URL. `<img src="../capture/...">` works
on disk but 404s in Studio and renders. Errors with a fix hint
saying replace `../capture/` with root-relative `capture/`.
Three vitest cases: `<img>` triggers, multi-occurrence url()s are
counted, root-relative paths stay clean. Registry source files and
installed blocks are exempted.

**Wiring**

`hyperframeLinter.ts` runs the new fonts rules alongside the existing
rule set; the composition rule was added inline so it picks up
automatically.
@ukimsanov ukimsanov force-pushed the feat/capture-pipeline-improvements branch from 29baca1 to eeacacc Compare May 21, 2026 01:13
ukimsanov added a commit that referenced this pull request May 21, 2026
Three concrete bugs found while auditing PR #991:

1. html-in-canvas-patterns.md (#1 in catalog, 3D Rotation with Bloom):
   The code example used `new THREE.EffectComposer(renderer)` UMD-style
   namespace access while the ESM imports right below pull them in as
   bare named imports. Three.js r150+ removed the UMD `examples/js/`
   globals, so as written the example throws `TypeError:
   THREE.EffectComposer is not a constructor`. Switched to the bare
   names matching the imports. THREE.Vector2 stays as-is — Vector2 is
   on the THREE namespace.

2. techniques.md (#5, Lottie Animation): The CDN path
   `@lottiefiles/dotlottie-web/dist/dotlottie-player.js` returns 404.
   `@lottiefiles/dotlottie-web` is the JavaScript SDK, not a web
   component — its `main` is `dist/index.cjs`. The web-component
   package is `@lottiefiles/dotlottie-wc` and the custom element is
   `<dotlottie-wc>`, not `<dotlottie-player>`. Updated both.

3. techniques.md (5 occurrences across Lottie / lottie-web /
   Video / @font-face examples): asset paths used the `../capture/`
   pattern that PR #989's `invalid_capture_path` lint rule emits an
   error for. Replaced all with root-relative `capture/...`. PRs #989
   and #991 are no longer self-contradictory.
@ukimsanov ukimsanov requested a review from Copilot May 21, 2026 02:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@ukimsanov ukimsanov requested a review from miguel-heygen May 21, 2026 03:21
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

Re-reviewed after rebase (e8d2da6e).

Same content as before — just rebased onto the updated stack. Five files, all additive: fonts.ts (150 lines), fonts.test.ts (121 lines), composition.ts (invalid_capture_path rule), composition.test.ts (3 test cases), hyperframeLinter.ts (wiring). No content changes from the previously approved version.

Still good. Ship it.

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.

4 participants