fix(video_compose): derive the Remotion theme from real playbook keys - #474
Open
bbudaedu wants to merge 1 commit into
Open
fix(video_compose): derive the Remotion theme from real playbook keys#474bbudaedu wants to merge 1 commit into
bbudaedu wants to merge 1 commit into
Conversation
`_build_theme_from_playbook` documents itself as reading "a playbook's actual
color values [...] not picked from a preset menu", but three of the keys it
reads are not in the playbook schema, so each silently fell back to a
hardcoded default for every playbook:
typo.get("heading") schema key is `headings`
palette.get("muted_text") schema key is `muted`
motion.get("pace") `pace` is an identity field; motion carries
pacing_rules, not a pace enum
Effect, before -> after:
flat-motion-graphics headingFont Inter -> Space Grotesk
muted #6B7280 -> #64748B
spring 20/120 @0.4s -> 12/80 @0.3s (pace: fast)
minimalist-diagram headingFont Inter -> IBM Plex Sans
anime-ghibli headingFont Inter -> Noto Serif JP
Every playbook rendered its headings in Inter regardless of what it declared,
and no playbook's declared pace ever reached the spring config.
This is the Remotion counterpart of the HyperFrames style-bridge defect in
issue calesthio#306. That bridge lives in lib/hyperframes_style_bridge.py and is
untouched here, as are the open PRs against it (calesthio#296, calesthio#307).
Two adjacent things deliberately left alone:
- `palette.get("surface", bg)` also names a key the schema does not define,
but the fallback to the background color is an explicit, sensible default
rather than a dropped value. Giving playbooks a real surface color would be
a schema addition, not a bug fix.
- The pace branch still handles only `fast` and `slow`, so `deliberate` and
`rapid` land on the moderate default. Extending that mapping is a motion
tuning decision, not part of reading the right key.
Note on ordering: Root.tsx's resolveTheme currently prefers a named preset
over themeConfig, which meant this derived theme was discarded whenever the
playbook name matched a Remotion preset — masking the defect. Fixing
resolveTheme before this would have made every video lose its heading
typeface, muted color and motion pacing at once.
Expectations in the new tests come from the playbook YAML rather than a
hardcoded table, so a playbook that changes its typeface keeps them honest.
Verified: 4 cases fail on the unfixed tree; full suite goes 964 -> 977 passed
with no regressions.
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
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.
The defect
VideoCompose._build_theme_from_playbookdocuments itself as:Three of the keys it reads are not in
schemas/styles/playbook.schema.json, so each silently fell back to a hardcoded default for every playbook:typo.get("heading")headingsInterpalette.get("muted_text")mutedmutedTextColorwas always#6B7280motion.get("pace")identity.paceBefore → after:
This is the Remotion counterpart of the HyperFrames style-bridge defect in #306. That bridge lives in
lib/hyperframes_style_bridge.pyand is untouched here, as are the open PRs against it (#296, #307).Note on ordering
Root.tsx'sresolveThemecurrently prefers a named preset overthemeConfig, and four of the five playbook names collide with Remotion preset names. So the derived theme was being discarded exactly where these defaults would have shown — which masked this bug.Fixing
resolveThemefirst would have made every video lose its heading typeface, muted color and motion pacing in one step. This PR has to land first.Deliberately left alone
palette.get("surface", bg)also names a key the schema does not define, but the fallback to the background color is an explicit, sensible default rather than a dropped value. Giving playbooks a real surface color would be a schema addition, not a bug fix.fastandslow, sodeliberateandrapidland on the moderate default. Extending that mapping is a motion tuning decision, not part of reading the right key.Coverage
tests/contracts/test_remotion_theme_playbook_keys.pytakes its expectations from the playbook YAML rather than a hardcoded table, so a playbook that changes its typeface keeps the test honest rather than making it stale.Verification
make lint: passed.The 3 skips are
anime-ghibli, which cannot yield a theme on currentmainfor the reason #468 fixes; they start running once that lands.Independent of #468, #469, #472 and #473 — no overlapping files.
🤖 Generated with Claude Code