feat: add SocialPost and CropPreview compositions for API rendering#3
Open
sidneyswift wants to merge 3 commits intomainfrom
Open
feat: add SocialPost and CropPreview compositions for API rendering#3sidneyswift wants to merge 3 commits intomainfrom
sidneyswift wants to merge 3 commits intomainfrom
Conversation
Move SocialPost and CroppedVideo compositions from the content-creation-app into the main remotion project. This consolidates all Remotion compositions into a single registry so the render-video API endpoint can render any composition by ID. New compositions: - SocialPost: Center-crop 16:9→9:16, audio overlay, TikTok-style captions - CropPreview: Simple center-crop for validation Also adds TikTokSans-Regular.ttf font for caption rendering.
📝 WalkthroughWalkthroughThe changes introduce two new video composition components (CroppedVideo and SocialPost) for generating 9:16 portrait social media videos from 16:9 landscape sources, register them in the root composition, and expand AGENTS.md documentation to describe the dual-role architecture supporting both local development and hosted API rendering workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/components/SocialPost.tsx (4)
47-47: Dead variablecaptionOpacity— remove it.
const captionOpacity = 1is always1and its only use isopacity: captionOpacity, which is equivalent toopacity: 1. There's no animation wired up to it.♻️ Proposed fix
- // Caption visible immediately - const captionOpacity = 1; ... - opacity: captionOpacity, + opacity: 1,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SocialPost.tsx` at line 47, Remove the dead constant captionOpacity from the SocialPost component and replace any usage of opacity: captionOpacity with the literal 1 (or simply remove the opacity style if default is fine); specifically delete the const captionOpacity = 1 declaration and update the style where opacity: captionOpacity is referenced (in the SocialPost JSX) so no unused variable remains.
70-72:<Sequence from={0}>wrapping<Audio>is a no-op — remove it.
from={0}is the default value forSequence. The wrapper adds no semantic timing offset;<Audio>with itsstartFromprop can stand alone.♻️ Proposed fix
- {!hasAudio && audioSrc && ( - <Sequence from={0}> - <Audio src={staticFile(audioSrc)} volume={0.85} startFrom={audioStartFrame} /> - </Sequence> - )} + {!hasAudio && audioSrc && ( + <Audio src={staticFile(audioSrc)} volume={0.85} startFrom={audioStartFrame} /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SocialPost.tsx` around lines 70 - 72, The <Sequence from={0}> wrapper around the Audio element is redundant; remove the Sequence wrapper so the Audio component stands alone. Locate the JSX where Sequence and Audio are used (the Sequence component wrapping <Audio src={staticFile(audioSrc)} volume={0.85} startFrom={audioStartFrame} />) and replace it with the Audio element by itself, preserving the props (src={staticFile(audioSrc)}, volume={0.85}, startFrom={audioStartFrame}); no other timing changes or imports should be needed.
56-66: Addmuted={!hasAudio}toOffthreadVideoto prevent double-audio.When
hasAudio: false, audio is managed explicitly via the<Audio>component. If the source video unexpectedly contains an audio track,OffthreadVideowill include it, producing double-audio in the render. Making the mute state conditional onhasAudiois a cheap defensive guard.♻️ Proposed fix
<OffthreadVideo src={videoUrl} + muted={!hasAudio} style={{ ... }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SocialPost.tsx` around lines 56 - 66, OffthreadVideo may play embedded audio causing double-audio when you also render <Audio>; update the OffthreadVideo usage to set its muted prop based on hasAudio (i.e., muted={!hasAudio}) so the video is muted when audio is handled via the Audio component; locate the OffthreadVideo component instance in SocialPost (the one using props videoUrl, offsetX, scaledWidth) and add the conditional muted prop to it.
44-44: Hardcoded30fps inaudioStartFrame— useuseVideoConfig().fps.If the composition's fps ever changes (e.g., 60fps preview),
audioStartSeconds * 30produces the wrong frame offset. The comment "audio is 30fps in Remotion" is also a misnomer — frame timing is driven by the composition fps, not a fixed audio rate.♻️ Proposed fix
-import { AbsoluteFill, OffthreadVideo, Audio, Sequence, staticFile } from "remotion"; +import { AbsoluteFill, OffthreadVideo, Audio, Sequence, staticFile, useVideoConfig } from "remotion"; export const SocialPost: React.FC<SocialPostProps> = ({ ..., audioStartSeconds = 0 }) => { + const { fps } = useVideoConfig(); - // Convert audio start offset from seconds to frames (audio is 30fps in Remotion) - const audioStartFrame = Math.round(audioStartSeconds * 30); + // Convert audio start offset from seconds to frames + const audioStartFrame = Math.round(audioStartSeconds * fps);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SocialPost.tsx` at line 44, The code uses a hardcoded 30fps when computing audioStartFrame (const audioStartFrame = Math.round(audioStartSeconds * 30)), which breaks when composition fps changes; replace the magic 30 with the composition fps from useVideoConfig() by calling useVideoConfig(), destructuring fps, and computing audioStartFrame = Math.round(audioStartSeconds * fps); also update/remove the misleading comment about "audio is 30fps in Remotion" so timing reflects the composition fps.AGENTS.md (1)
49-49: Manual dual-repo sync is an operational risk — consider a CI safeguard.The requirement to hand-copy components to
tasks/src/remotion/every time one changes here means a missed sync silently deploys stale compositions to the API renderer. Consider adding a CI step (e.g., a diff check or a shared package/symlink) that fails the pipeline if the two copies diverge. At minimum, the deployment checklist in this file is a good stopgap until that automation exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 49, The AGENTS.md note highlights a risky manual sync of remotion compositions to the tasks repo; add an automated safeguard: create a CI job that compares the composition files in this repo against the duplicate in the tasks repository (or the shared package if you move them) and fail the pipeline on any diff, or refactor to a single shared package/monorepo artifact consumed by both repos and update build steps accordingly; update AGENTS.md to record the new CI check or shared-package requirement so future PRs must satisfy the check before merging.src/components/CroppedVideo.tsx (1)
20-21: UseuseVideoConfig()for composition dimensions instead of magic numbers.
1280and720are hardcoded. If this component is ever embedded in a composition with different dimensions, the crop framing will be silently wrong.useVideoConfig()makes the math adaptive at zero cost.♻️ Proposed fix
-import { AbsoluteFill, OffthreadVideo } from "remotion"; +import { AbsoluteFill, OffthreadVideo, useVideoConfig } from "remotion"; export const CroppedVideo: React.FC<CroppedVideoProps> = ({ videoUrl }) => { + const { width, height } = useVideoConfig(); - const scaledWidth = 1280 * (16 / 9); - const offsetX = -(scaledWidth - 720) / 2; + const scaledWidth = height * (16 / 9); + const offsetX = -(scaledWidth - width) / 2; ... - height: 1280, + height,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/CroppedVideo.tsx` around lines 20 - 21, Replace the hardcoded numbers by using useVideoConfig(): import and call useVideoConfig() to get {width, height}, then compute scaledWidth and offsetX from those values (e.g. const scaledWidth = width * (16/9) and const offsetX = -(scaledWidth - width)/2 or, if your crop logic expects height, use height in place of the second operand), replacing the magic 1280 and 720 in the scaledWidth and offsetX calculations to make the crop adaptive to composition size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/SocialPost.tsx`:
- Around line 50-51: The crop math for scaledWidth and offsetX is duplicated
from CroppedVideo; remove the duplicated calculations in SocialPost and either
(preferred) compose the existing <CroppedVideo> component for the video layer
instead of re-implementing the math, or extract the shared logic into a small
utility (e.g., getCropStyle(width,height)) that returns scaledWidth and offsetX
and call that from both CroppedVideo and SocialPost; update usages of
scaledWidth and offsetX to use the composed component or the utility.
- Around line 12-15: The font load is currently async and unsynchronized with
Remotion, causing SSR frames to use the fallback; wrap the load with Remotion's
delayRender/continueRender/cancelRender flow: call delayRender() before
creating/loading the FontFace (reference FontFace and
staticFile("TikTokSans-Regular.ttf")), then in the fontFace.load().then(...) add
the loaded font to document.fonts and call continueRender(handle) where handle
is the value returned by delayRender(); in the .catch(...) call
cancelRender(handle) and log the error instead of silencing it so failed font
loads cancel the render and surface the error.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 49: The AGENTS.md note highlights a risky manual sync of remotion
compositions to the tasks repo; add an automated safeguard: create a CI job that
compares the composition files in this repo against the duplicate in the tasks
repository (or the shared package if you move them) and fail the pipeline on any
diff, or refactor to a single shared package/monorepo artifact consumed by both
repos and update build steps accordingly; update AGENTS.md to record the new CI
check or shared-package requirement so future PRs must satisfy the check before
merging.
In `@src/components/CroppedVideo.tsx`:
- Around line 20-21: Replace the hardcoded numbers by using useVideoConfig():
import and call useVideoConfig() to get {width, height}, then compute
scaledWidth and offsetX from those values (e.g. const scaledWidth = width *
(16/9) and const offsetX = -(scaledWidth - width)/2 or, if your crop logic
expects height, use height in place of the second operand), replacing the magic
1280 and 720 in the scaledWidth and offsetX calculations to make the crop
adaptive to composition size.
In `@src/components/SocialPost.tsx`:
- Line 47: Remove the dead constant captionOpacity from the SocialPost component
and replace any usage of opacity: captionOpacity with the literal 1 (or simply
remove the opacity style if default is fine); specifically delete the const
captionOpacity = 1 declaration and update the style where opacity:
captionOpacity is referenced (in the SocialPost JSX) so no unused variable
remains.
- Around line 70-72: The <Sequence from={0}> wrapper around the Audio element is
redundant; remove the Sequence wrapper so the Audio component stands alone.
Locate the JSX where Sequence and Audio are used (the Sequence component
wrapping <Audio src={staticFile(audioSrc)} volume={0.85}
startFrom={audioStartFrame} />) and replace it with the Audio element by itself,
preserving the props (src={staticFile(audioSrc)}, volume={0.85},
startFrom={audioStartFrame}); no other timing changes or imports should be
needed.
- Around line 56-66: OffthreadVideo may play embedded audio causing double-audio
when you also render <Audio>; update the OffthreadVideo usage to set its muted
prop based on hasAudio (i.e., muted={!hasAudio}) so the video is muted when
audio is handled via the Audio component; locate the OffthreadVideo component
instance in SocialPost (the one using props videoUrl, offsetX, scaledWidth) and
add the conditional muted prop to it.
- Line 44: The code uses a hardcoded 30fps when computing audioStartFrame (const
audioStartFrame = Math.round(audioStartSeconds * 30)), which breaks when
composition fps changes; replace the magic 30 with the composition fps from
useVideoConfig() by calling useVideoConfig(), destructuring fps, and computing
audioStartFrame = Math.round(audioStartSeconds * fps); also update/remove the
misleading comment about "audio is 30fps in Remotion" so timing reflects the
composition fps.
Comment on lines
+12
to
+15
| if (typeof globalThis.FontFace !== "undefined") { | ||
| const fontFace = new FontFace("TikTok Sans", `url(${staticFile("TikTokSans-Regular.ttf")})`); | ||
| fontFace.load().then((f) => { document.fonts.add(f); }).catch(() => {}); | ||
| } |
There was a problem hiding this comment.
Font load is not synchronized with Remotion's render pipeline — SSR frames may use the fallback font.
The canonical Remotion pattern for font loading requires calling delayRender() before the async op and continueRender(waitForFont) inside .then() so Remotion waits for the font before screenshotting any frame. Without it, the render starts immediately and TikTok Sans is almost certainly not yet in document.fonts when frame 0 is captured — every frame will render in the system-ui fallback.
Additionally, .catch(() => {}) silences errors entirely. If the async task fails and you cannot recover, cancelRender() should be called to cancel the render instead of silently falling back to the wrong font.
🐛 Proposed fix
-import { AbsoluteFill, OffthreadVideo, Audio, Sequence, staticFile } from "remotion";
+import { AbsoluteFill, OffthreadVideo, Audio, Sequence, staticFile, delayRender, continueRender, cancelRender } from "remotion";
-if (typeof globalThis.FontFace !== "undefined") {
- const fontFace = new FontFace("TikTok Sans", `url(${staticFile("TikTokSans-Regular.ttf")})`);
- fontFace.load().then((f) => { document.fonts.add(f); }).catch(() => {});
-}
+if (typeof globalThis.FontFace !== "undefined") {
+ const waitForFont = delayRender("Loading TikTok Sans");
+ const fontFace = new FontFace("TikTok Sans", `url('${staticFile("TikTokSans-Regular.ttf")}') format('truetype')`);
+ fontFace
+ .load()
+ .then((f) => { document.fonts.add(f); continueRender(waitForFont); })
+ .catch((err) => cancelRender(err));
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/SocialPost.tsx` around lines 12 - 15, The font load is
currently async and unsynchronized with Remotion, causing SSR frames to use the
fallback; wrap the load with Remotion's delayRender/continueRender/cancelRender
flow: call delayRender() before creating/loading the FontFace (reference
FontFace and staticFile("TikTokSans-Regular.ttf")), then in the
fontFace.load().then(...) add the loaded font to document.fonts and call
continueRender(handle) where handle is the value returned by delayRender(); in
the .catch(...) call cancelRender(handle) and log the error instead of silencing
it so failed font loads cancel the render and surface the error.
Comment on lines
+50
to
+51
| const scaledWidth = 1280 * (16 / 9); | ||
| const offsetX = -(scaledWidth - 720) / 2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
DRY violation — crop math is identical to CroppedVideo.tsx.
scaledWidth and offsetX are copy-pasted from CroppedVideo.tsx lines 20-21. Either compose the existing <CroppedVideo> component for the video layer, or extract the math to a shared utility. As per coding guidelines: "Extract shared logic into utilities (DRY)."
Option A — compose <CroppedVideo> (preferred, eliminates all duplication):
♻️ Proposed fix (Option A)
+import { CroppedVideo } from "./CroppedVideo";
export const SocialPost: React.FC<SocialPostProps> = ({ videoUrl, ... }) => {
- const scaledWidth = 1280 * (16 / 9);
- const offsetX = -(scaledWidth - 720) / 2;
return (
<AbsoluteFill style={{ backgroundColor: "#000", overflow: "hidden" }}>
- <OffthreadVideo
- src={videoUrl}
- style={{ position: "absolute", top: 0, left: offsetX, width: scaledWidth, height: 1280, objectFit: "cover" }}
- />
+ <CroppedVideo videoUrl={videoUrl} />
{/* audio and caption layers unchanged */}
</AbsoluteFill>
);
};Option B — extract to a utility if composing the component is impractical:
♻️ Proposed fix (Option B)
// src/utils/cropMath.ts
export function getCropStyle(width: number, height: number) {
const scaledWidth = height * (16 / 9);
const offsetX = -(scaledWidth - width) / 2;
return { scaledWidth, offsetX };
}Then use getCropStyle(720, 1280) in both components.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/SocialPost.tsx` around lines 50 - 51, The crop math for
scaledWidth and offsetX is duplicated from CroppedVideo; remove the duplicated
calculations in SocialPost and either (preferred) compose the existing
<CroppedVideo> component for the video layer instead of re-implementing the
math, or extract the shared logic into a small utility (e.g.,
getCropStyle(width,height)) that returns scaledWidth and offsetX and call that
from both CroppedVideo and SocialPost; update usages of scaledWidth and offsetX
to use the composed component or the utility.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds two new compositions to the Remotion project that are used by the
POST /api/video/renderendpoint (PR 222 in api repo) for server-side video rendering via Trigger.dev.New Compositions
Other Changes
TikTokSans-Regular.ttffont for caption stylingFontFaceAPI usage withtypeof globalThis.FontFacecheck for Node.js compatibility (required for server-side rendering in Trigger.dev)AGENTS.mdwith documentation on local vs hosted usage, deployment checklist, composition design guidelines, and roadmap (Option A: pre-built library → Option B: template-driven)Related PRs
POST /api/video/renderendpointrender-videoTrigger.dev task with Remotion rendering logicTested
Successfully rendered a CropPreview composition via the API endpoint on preview deployment — video uploaded to Supabase Storage and accessible via signed URL.
Summary by CodeRabbit
New Features
Documentation