Skip to content

fix frameRendering with all extensions#478

Open
ExtraBinoss wants to merge 2 commits intowebadderallorg:mainfrom
ExtraBinoss:fix/global-extension-frame-renderer-path
Open

fix frameRendering with all extensions#478
ExtraBinoss wants to merge 2 commits intowebadderallorg:mainfrom
ExtraBinoss:fix/global-extension-frame-renderer-path

Conversation

@ExtraBinoss
Copy link
Copy Markdown
Collaborator

@ExtraBinoss ExtraBinoss commented May 9, 2026

Summary

Fix frame renderer integration to use extension-host registered frames globally

Why

The current behavior can look like frame rendering is tied to the built-in borders-frames extension, but the intended architecture is extension-agnostic.

Both export paths (FrameRenderer and ModernFrameRenderer) should resolve frames from the shared extension host registry so any active extension that registers frames via api.registerFrame(...) works consistently.

This PR clarifies and aligns that behavior so frame rendering is not perceived as custom logic for a single extension.

What this changes

  • Ensures frame lookup in both renderer pipelines is based on extensionHost.getFrames().
  • Keeps support for both frame types:
  • draw callback frames (resolution-independent rendering)
  • file/dataURL-backed frame assets
  • Preserves screenInsets usage so preview and export layout remain consistent.

Expected impact

  • Any extension with ui permission that registers frames can be rendered in preview and export. - No special-case dependency on public/builtin-extensions/borders-frames.
  • More predictable extension API behavior for third-party frame packs.

Type of Change

  • Bug Fix

Video (wherever possible):

export-lighnting-frame_DjT5v1JT.mp4

Testing Guide

Any extension with a rendering in the canvas, like my border extension.

Checklist

  • I have performed a self-review of my code.
  • I have added any necessary screenshots or videos.
  • I have linked related issue(s) and updated the changelog if applicable.

Thank you for contributing!

Summary by CodeRabbit

  • New Features
    • Added device frame overlay rendering during video export.
    • Frame overlays now correctly scale and pan with the video scene, maintaining visual consistency.
    • Improved memory management for frame resources during export cleanup.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 9, 2026

📝 Walkthrough

Walkthrough

This PR adds device frame overlay support to the video renderer. Frame assets are configured via config.frame, loaded during initialization, positioned relative to the layout mask with optional insets, and rendered with scene transforms (zoom/pan) to stay synchronized with the video content during export.

Changes

Device Frame Overlay Implementation

Layer / File(s) Summary
Frame State Fields
src/lib/exporter/modernFrameRenderer.ts
New instance fields track frame container, sprite, image source, draw callback, insets, and rasterization cache.
Scene Integration
src/lib/exporter/modernFrameRenderer.ts
frameContainer is created and attached to cameraContainer during Pixi initialization.
Asset Loading and Resolution
src/lib/exporter/modernFrameRenderer.ts
setupFrame() resolves frame configuration from extension host, loads images with CORS, or stores procedural draw callbacks with computed insets.
Layout Updates
src/lib/exporter/modernFrameRenderer.ts
updateFrameLayout() computes frame geometry from mask and insets, rasterizes or creates sprite texture, and positions overlay under camera container.
Scene Transform Application
src/lib/exporter/frameRenderer.ts, src/lib/exporter/modernFrameRenderer.ts
drawFrame() accepts optional sceneTransform and wraps drawing with applyCanvasSceneTransform; both temporal and normal render paths pass current scale/position.
Resource Cleanup
src/lib/exporter/modernFrameRenderer.ts
destroy() includes frame sprite texture in teardown, nulls container, and resets all frame fields to prevent leaks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

Checked

🐰 A frame appears, transformed and bright,
Positioned just right, with zoom and with sight,
Device frames now dance with the scene,
The finest overlay ever seen!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is partially related to the changeset—it mentions frame rendering but is vague about the actual fix (making it extension-agnostic rather than fixing an error). Consider a more descriptive title like 'Use extension host registry for frame rendering in both renderer pipelines' to clarify the main architectural change.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description covers all major template sections including summary, motivation, type of change, video, testing guide, and checklist, providing sufficient context for reviewers.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@ExtraBinoss ExtraBinoss marked this pull request as ready for review May 9, 2026 20:38
Copilot AI review requested due to automatic review settings May 9, 2026 20:38
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/lib/exporter/frameRenderer.ts (1)

1419-1448: ⚡ Quick win

Redundant dynamic import of extensionHost.

extensionHost is already imported at the top of the file (line 58). The dynamic import here is unnecessary and adds overhead.

♻️ Proposed fix
 	private async setupFrame(): Promise<void> {
 		const frameId = this.config.frame;
 		if (!frameId) return;

-		const { extensionHost } = await import("@/lib/extensions/extensionHost");
 		const frames = extensionHost.getFrames();
 		const frame = frames.find((f) => f.id === frameId);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/exporter/frameRenderer.ts` around lines 1419 - 1448, The setupFrame
method currently does a redundant dynamic import of extensionHost; remove the
await import line and use the already-imported extensionHost (referenced at top
of the file) directly when calling extensionHost.getFrames(), leaving the rest
of setupFrame logic unchanged (keep using frame.screenInsets, frame.draw,
frame.filePath, frameImage, frameDraw as-is); ensure no other references expect
a locally destructured extensionHost variable and update any such usage to the
module-level extensionHost.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 1419-1448: The setupFrame method currently does a redundant
dynamic import of extensionHost; remove the await import line and use the
already-imported extensionHost (referenced at top of the file) directly when
calling extensionHost.getFrames(), leaving the rest of setupFrame logic
unchanged (keep using frame.screenInsets, frame.draw, frame.filePath,
frameImage, frameDraw as-is); ensure no other references expect a locally
destructured extensionHost variable and update any such usage to the
module-level extensionHost.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 44a4b905-9102-493b-9f6c-261552416a04

📥 Commits

Reviewing files that changed from the base of the PR and between 62a1ed2 and 5c0342c.

📒 Files selected for processing (2)
  • src/lib/exporter/frameRenderer.ts
  • src/lib/exporter/modernFrameRenderer.ts

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.

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.

2 participants