feat(registry): say what a promoted template's slots mean, not where they sit - #3562
Conversation
…they sit
Every declaration described its slot by position — appName was "small credit in the corner
of every banner" — and every editing contract described the supplied brand only by what it
may not touch, chatgpt-exchange going as far as "only the subject discussed inside the
conversation". An agent filling those slots has nothing telling it that the video is an
advertisement for that brand, and remixes shipped with an invented brand instead of the
real one.
Adds `portrays` to the 35 identity-bearing variables across seven templates: what the slot
stands for, as a list, kept orthogonal to `role` (which says which aspect of the
composition a knob affects, not what a value means to a viewer). Rewrites those variables'
descriptions to state meaning, and gives every TEMPLATE.md the affirmative sentence it was
missing. notes-reveal now records honestly that it declares no identity slot at all, which
is why a remix of it cannot carry a brand.
`portrays` is optional metadata: rendering ignores it, the promoted contract still projects
{id, type, role}, and packages that predate it keep working. Variable ids, types, roles,
labels, and defaults are untouched, so installs and renders see the same contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This was the one promoted template that declared no identity slot at all, and the previous commit could only record that honestly. A remix of it could not name the advertiser, could not carry its domain, and had nowhere to place its mark — so the ad it produced was unattributable no matter how good the copy was. The closing card ends at y=1708, leaving 212px of dotted scene. A sign-off strip lives there: `brandLogo` (portrays subject_logo) above `brandDomain` (portrays subject_domain), in the card's own plum and Courier Prime. Putting it outside the card means the hand-lettered headline and the checklist keep the geometry they were designed with — verified by `hyperframes check`, which reports the same 0 errors and the same warning counts as origin/main, with one more contrast check (367/367 AA). No name slot: this template has no text slot that can hold a bare brand name. `cardTop`'s marker line continues into the fixed words "OF ONE FILE", and a name set beside a wordmark reads twice — rendering it proved that before this was committed. The mark plus the domain carry the attribution, which is what the other end-card templates do. Rendered to check rather than assumed: the packaged default reads as the HyperFrames wordmark over hyperframes.heygen.com, and a resolved symbol mark (Notion's cube over notion.com) sits correctly in the same strip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g the card
Three defects, all visible in the reported gamma remix.
The seven body lines carried their characters as pre-split `<span class="ch">`
elements so the reveal animation could address them by id. That made the note
permanently unwritable: 46% of the 24.9s runtime, and the only place the note
says anything, was fixed copy about making videos with HyperFrames -- shipped
inside an advertisement for a Dutch DIY chain. The spans are now built at
runtime from seven declared variables, keeping the same ids, so the animation
is untouched and the copy is the brand's.
`cardMid` and `cardBottom` ("OF", "ONE FILE") were literals with no variable at
all, so a remix could only ever change the first of the card's three lines.
Every text slot sat at a fixed left edge on one line, guarded only by a
character cap -- the wrong unit. `cardTop` allowed 12 characters; "THE POWER"
(9) fits and "CREATE DECKS" (12) does not, which is why both reported gamma
remixes shipped with the closing headline over the edge of the card. Slots are
now fitted to their box at render time, shrinking type only as far as needed and
never past the designed size. The card headlines also carried
`data-layout-allow-overflow`, switching off the one check that would have caught
it; those waivers are gone. The note's title and body keep theirs, because
`#note-body` is translated across the whole scene and leaves the canvas by
design.
Timing is preserved rather than regenerated: each body line is revealed inside
the frame window the packaged rhythm already used, so `JUMPS` and the hand-tuned
`SCROLLS` curves stay valid and the composition is still 24.867s for any length
of copy. A line whose length matches the packaged copy keeps its rhythm frame
for frame.
Verified: rendered with the packaged defaults, SSIM 1.000000 across all 746
frames against the previous template -- the design is bit-identical. Rendered
with the gamma values that overflowed, and with a stress set of long and
one-word values: both stay inside the card and the note, both still 24.867s.
`hyperframes check` passes, with layout warnings down from 28 to 8 and no new
issue class. Note that `check` takes no variable values, so it guards the
template's defaults and the fit is what guards a remix.
A cap that admits a string still too wide at the floor font size puts the text back outside its box -- the exact bug the fit was added for. With the floor at 0.6 and the title cap at 40, a 40-character title still overflowed `#note-body` by 30px, because "Things nobody told me" is 21 characters and 900px at 88px, so 42.9px per character, and 40 characters need to reach about 0.45 of the design size to fit. Floor to 0.45 and the caps in: titles 34, body lines 52, cardTop and cardBottom 16. Every cap is now satisfiable with room to spare, and the common case stays far from the floor. Found by probing all eight promoted templates with values about 1.8x the length of their packaged defaults, which is the length a real remix produces; short sentinel values only ever prove a slot is wired. Defaults are unaffected: SSIM 1.000000 across all 746 frames, still 24.867s, `hyperframes check` still passes.
`prompt` is an editable slot in both templates, but `.comp-text` is a fixed-width single line with `white-space: pre` inside a composer that clips, and the packaged prompt already reaches the mic -- it has no headroom at all. A longer prompt therefore slid under the mic and the send button and lost its tail, with the caret hidden behind them: the remix read as having typed into nowhere, cut mid-word. Both now scroll the typed text left to keep the caret in view, the way a real single-line input does, on the very per-character times the reveal already uses. The limit is measured to the opaque round button rather than the mic, because the packaged prompt's caret already sits 2px past the mic's left edge in chatgpt-exchange -- measuring to the mic would scroll the packaged composition. Shrink-to-fit was the other option and it is worse: with no headroom it shrinks the packaged prompt too, and it still cannot absorb a prompt twice as long without unreadable type. It was tried, measured, and dropped. A prompt that fits emits no keyframes and the element is never handed to GSAP at all -- even a zero translate stamps a transform, which changes how the text rasterizes. claude-exchange puts this inside its existing `build()`, which already re-runs on `document.fonts.ready`, so it measures the final font. chatgpt-exchange builds once, so its scroll keyframes are kept in a list and rebuilt on the same event. Verified on the packaged defaults: claude-exchange SSIM 1.000000 across 1284 frames. chatgpt-exchange is not frame-deterministic -- rendering it twice unchanged gives 0.999959 with 249 frames under 0.9999, and before-vs-after gives 0.999967 with the same 249 -- so the change sits inside its own noise. Durations unchanged at 14.9s and 21.4s. `hyperframes check` reports byte-identical results to the unchanged templates.
…characters The three checklist rows were the only text slots with no width fit, so their character cap was doing the job a measurement should: it was set to 8 because that was the widest the packaged strings needed, and a gamma.com remix delivered "DIRECT MEE" and rendered "DIRECT M". Fit the labels and values like every other text slot -- dividing the label's scaleX(0.86) back out of its column, since fitWidth measures the untransformed width -- and return the cap to a backstop at 18. "DIRECT MEE" needs no shrink at all: it ends at x=694 against a tick at x=715, so the truncation was never about room. Measured: the packaged defaults still render at their authored 44px and 50px, and all three values clear the tick.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at head e299a982d48fe79869fc76483ab67920f3e92438. All 8 required contexts on main ran and passed, matched by exact name.
Strengths
portraysis the right shape for this problem. Prose descriptions can be improved indefinitely and still not be checkable; a machine-readable marker on each identity-bearing slot lets an eval ask "did the advertiser reach the screen?" without pattern-matching English.docs/concepts/variables.mdxdocumenting the eight values is what makes it usable by anything other than this PR.- The source-chip descriptions (
row1Chip/row2Chip/… inchatgpt-exchange) telling the agent a chip asserts where the ranking came from, so it should stay neutral when the comparison is written for the remix — that is a distinction between copy and claim that most slot metadata never makes. - Fixing
notes-revealrather than recording it as out of scope, and putting the sign-off strip in the 212px below the card so the hand-lettered headline keeps its designed geometry.
Findings
important — the byte-scope claim in the description is not accurate, and the exception is not notes-reveal. The description says: "Compositions are otherwise untouched: outside the data-composition-variables block the bytes are unchanged, and no slot was added, removed or renamed", with notes-reveal called out separately as the one composition change. Against the diff, two more templates change composition:
registry/blocks/chatgpt-exchange/chatgpt-exchange.html— 52 added lines outside the variables blockregistry/blocks/claude-exchange/claude-exchange.html— 42 added lines outside the variables block
Both add a new caret-following scroll in the composer (layoutCaretScroll, #cge-comp-text / #cge-blue-btn, CARET_PAD). The other five HTML files are clean — their changes are entirely slot metadata, which I checked line by line.
To be clear about severity: the code itself is careful, and I would not ask for it to be removed. It collects steps before touching typedEl so a prompt that fits emits no keyframes and the packaged render is bit-for-bit unaffected; it converts rects with the element's own ratio instead of reading --scale, so it survives a change to that; and the comment explains why the blue button rather than the mic is the real boundary. The problem is that a reader — human or bot — who takes the scope claim at face value will not review those 94 lines at all, and the verification list (hyperframes check 0 errors / 367 contrast, a notes-reveal render at t=23.5s, an agent grading run) contains nothing that exercises a long prompt in either exchange template. Correcting the claim and saying how the caret path was checked is the whole ask.
question — timeline mutation after first build. document.fonts.ready.then(layoutCaretScroll) re-runs the layout and re-adds tweens to tl after the composition has already built its timeline once. For a deterministic frame-by-frame render that is only safe if capture cannot begin before fonts.ready resolves. If the renderer does not guarantee that ordering, a capture could straddle the re-layout and land keyframes mid-sequence. You have rendered this, so you may already know the answer — worth stating it in the description either way, since the same pattern will get copied into the next template.
Verdict: APPROVE
Reasoning: The semantic-role change is well-designed and independently verified, and the undisclosed caret-scroll is good code. Approving on the merits; please fix the scope sentence before merge so the next reader is not told those 94 lines do not exist.
— Rames Jusso
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head e299a982d48fe79869fc76483ab67920f3e92438.
The portrays contract is a strong addition: it makes identity semantics machine-readable without changing rendering, and the eight documented values give downstream agents a stable vocabulary. I also agree with Rames that the PR body must disclose the substantial caret-scroll and notes-reveal behavior changes rather than describe this as metadata-only.
Blocking — registry/blocks/chatgpt-exchange/chatgpt-exchange.html:1419-1427: the font-ready relayout kills the registered master timeline for long prompts.
Timeline.to() returns the timeline (tl), not the child tween. The code pushes that return value into caretScrollTweens, so every array entry is the same master tl. layoutCaretScroll() runs synchronously and then unconditionally runs again from document.fonts.ready; when the prompt is long enough to create scroll steps, the second pass executes tw.kill() and kills window.__timelines["chatgpt-exchange"] itself before adding replacement steps. The default prompt does not overflow, which is why the packaged render and current green checks miss this exact remix path.
Please make the caret animation a nested timeline that can be cleared/rebuilt, or construct/store the actual child tweens and remove only those. Add a long-prompt regression that waits for document.fonts.ready and then seeks the registered timeline through the typing/collapse interval; asserting only the default composition will not distinguish this failure.
claude-exchange does not share this bug: its font-ready path clears and rebuilds the master timeline inside build() rather than storing the return value of tl.to() as a child.
Exact-head CI is green, but it does not exercise an overflowing prompt after fonts settle.
Verdict: REQUEST_CHANGES
Reasoning: The semantic metadata is good, but one of the added long-prompt protections invalidates the master GSAP timeline on the very input class it is intended to support.
— Magi
Dismissing my own approval: I would not approve this head today. Superseded by my re-review at the same head, which retracts it and carries the mechanism.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Retracting my earlier approval on this PR. I approved at e299a982 (review 5078820454) and that was wrong — I read the caret-scroll block as self-contained and did not check what tl.to() returns. I have dismissed that approval rather than leaving it standing, since a COMMENT does not remove it and it was still counting toward the gate.
@miguel-heygen's premise is correct, and I verified it independently rather than relaying it. One correction to the stated consequence, and one defect underneath it that I don't think has been named yet.
blocker — the caret-scroll teardown never removes anything, and detaches the composition from its parent
registry/blocks/chatgpt-exchange/chatgpt-exchange.html:1419 and :1422 push the return value of tl.to(...) into caretScrollTweens. In GSAP 3.14.2 — the build this file loads at :33 — Timeline.to() returns the timeline, for chaining; only gsap.to() returns a Tween. Measured against that exact dist file: tl.to(target, {...}, at) === tl → true. So the array holds N references to the master timeline created at :1167, and :1391's caretScrollTweens.forEach((tw) => tw.kill()) is tl.kill().
Animation.prototype.kill() is _interrupt(this): detach from parent, fire onInterrupt. It does not touch the timeline's own children. Two things follow.
1. The document.fonts.ready re-measure at :1427 never replaces the first pass — it layers on top of it. Replicating the two-pass bookkeeping verbatim: children go 1 → 4 → 7, zero removed. The pre-font tweens stay on the timeline and the post-font set is added alongside them. At a time only the first pass covered, the stale pre-font offset is what renders (measured x = -40 at a step the second pass does not emit). So the re-measure that exists to correct the pre-font metrics does not correct them.
2. The composition is orphaned. Direct seek of the killed timeline still maps correctly (measured 25 at seek(0.25), 90 at seek(0.90), ease: "none"), so a standalone render of this block is unaffected — which is part of why CI is green here and why this is easy to miss. But the timeline is no longer in its parent's child list, so:
- driven by a parent's seek, it freezes at its initial state — measured
0where an unkilled control renders50, withmaster.getChildren().includes(child)→false; play()on it is dead — measured still0after 400ms where an unkilled control completes at100. That is the pathpackages/core/src/runtime/init.tsactivateSiblingTimelinestakes on every registered non-master timeline, and the comment there already warns that a child which never receives a workingplay()sits at its initial CSS state.
Trigger: only when the first pass emits steps — an overflowing prompt, i.e. exactly the remix path the caret scroll was added for. The packaged prompt emits none, so the packaged composition and CI stay clean. Note document.fonts.ready resolves even when the faces are already loaded, and the .then() at :1427 is unguarded, so for any overflowing prompt this is deterministic rather than a font-timing race.
The sibling in this same PR already has the correct shape. registry/blocks/claude-exchange/claude-exchange.html:1234 puts the identical caret-scroll code inside build(), whose first statement at :1235 is tl.clear(), and guards the re-run with document.fonts.status !== "loaded". chatgpt-exchange has neither. Adopting that shape is the smallest fix.
Two alternatives, both verified against 3.14.2:
- capture the real tween —
tl.to(...)thentws.push(tl.recent());recent()returns aTween, killing those keeps the child count stable across passes and leaves the master parented; - give the caret its own nested timeline added once (
tl.add(caretTl, 0)) andcaretTl.clear()per layout; master child count stable, and the pass-1-only time then renders the pass-2 value (-26, not the stale-40).
Either way the regression worth pinning is: a prompt long enough to emit steps, re-layout after the faces settle, then assert the child count did not grow and the timeline still has a parent.
nit — carried over
The scope sentence in the PR body still reads as description-only for the two exchange blocks; chatgpt-exchange (+66/-9) and claude-exchange (+54/-6) both add composition behavior.
Audited: chatgpt-exchange.html and claude-exchange.html (caret-scroll paths end-to-end, at e299a982), GSAP 3.14.2 timeline/kill semantics (measured, not recalled), runtime/init.ts sibling activation.
Trusting: the .mdx catalog copy and the portrays metadata, which I read for consistency but did not re-derive.
Verdict: REQUEST CHANGES
Reasoning: A confirmed behavior defect on the exact path this PR adds, with the correct pattern already present in its sibling file; my prior approval is dismissed because I would not approve this today.
— Rames Jusso
…done `tl.to()` returns the TIMELINE, not the tween it created, so `caretScrollTweens` held 49 references to the master timeline. The fonts-ready rebuild then called `.kill()` on the master timeline 49 times and re-added its keyframes on top of the ones it meant to replace. Measured with an overflowing prompt: master timeline detached from the global timeline (tl.parent -> null) scroll tweens on #cge-typed: 49 before the rebuild, 98 after 49 timestamps left with two competing tweens on the same property GSAP's `Animation.kill()` interrupts and detaches rather than killing children, so the composition kept rendering under an explicit seek -- which is why the packaged-defaults SSIM check passed and never saw this. It is still wrong: the timeline is off the ticker, and the stale keyframes were measured against the fallback font. Only an overflowing prompt reaches it, i.e. exactly the remix the scroll exists for. The packaged prompt emits no steps, so nothing is created and nothing is killed. Now one nested child timeline holds the steps: a real object that can be killed and replaced, and killing it cannot reach `tl`. Children sit at the same absolute times and the nest is added at 0, so the motion is unchanged. Verified: overflowing prompt holds at 49 scroll tweens across the rebuild with no duplicated timestamps and the timeline stays attached. Packaged defaults against the pre-feature baseline -- SSIM 0.999998 over 894 frames, 13 frames below 0.9999 against a 249-frame noise floor for this template, duration 14.9s unchanged. Found by Magi in review of #3562. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both fixed at The
One correction to the mechanism, because it changes how bad this is: GSAP's The steps now live in one nested child timeline: a real object that can be killed and replaced, and killing it cannot reach Verified both directions:
The scope sentence. You were both right and it was worse than described: the claim also covered |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head d3359ee42b5e71531b87e7df6879338f574391f4.
The implementation defect from my prior review is fixed. The caret steps now live in one nested caretTl; the font-ready pass kills only that child, clears the transform only when a prior scroll existed, rebuilds the steps at the same absolute times, and re-adds the child at position 0. The master tl remains registered and parent-driven seeking is preserved. The PR body also now states the real three-template behavioral scope accurately.
Remaining important gate — the requested regression is not committed. The delta from the blocked head changes only chatgpt-exchange.html; no test or regression fixture exercises an overflowing prompt through document.fonts.ready and parent-driven seek. The current required suite renders the packaged prompt, which emits zero caret-scroll keyframes and therefore cannot distinguish the original failure from this fix. That is exactly how the broken implementation stayed fully green.
Please commit a browser/runtime regression with a prompt long enough to emit steps, wait for the font-ready rebuild, then assert at minimum that the registered master timeline remains parented and the caret child count/timestamps do not double before seeking through the typing and collapse interval. The manual measurement in the PR body is good evidence for this head, but it is not a guard against the next edit; this repo requires changed logic branches to have a test that can fail when production behavior regresses.
All required exact-head checks are green. This hold is only for the missing discriminating regression; the code fix itself is accepted.
Verdict: REQUEST_CHANGES
Reasoning: The timeline bug is repaired, but the only path capable of detecting it remains outside CI, despite the prior review explicitly requesting that regression and the default-render suite being structurally unable to cover it.
— Magi
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at d3359ee4. Posting this as a comment rather than an approval: @miguel-heygen's CHANGES_REQUESTED is pinned to this exact head, so it is a live block and not mine to stamp over. On the code I am satisfied — details below, including a measured reason the specific test being asked for would not do what it looks like it does.
The fix is correct, verified at the mechanism
I checked the new shape against the same harness that found the defect rather than against the diff's own reasoning. Replicating layoutCaretScroll verbatim on gsap@3.14.2:
- master child count stable across two passes with different metrics (
5 → 5) — the stale nest is genuinely discarded, where the old shape went4 → 7with nothing removed; - the master stays parented, so the sub-composition and
play()paths the oldtl.kill()severed are intact — under a parent-driven seek the value now propagates (-52) where the old shape froze at its initial state; - a time only the first pass covered now renders the second pass's value (
-26, not the stale-40), which is the bug thedocument.fonts.readyre-measure exists to prevent and previously did not; tl.add(caretTl, 0)preserves absolute times — a step authored at1.0reads-13.00att=1.04underease: "none";- master
totalDuration()unchanged, so nesting does not extend the composition.
This is also stronger than the tl.recent() alternative I offered. That one stays correct only while every future edit remembers to capture the return value; killing a nested child cannot reach its parent, so the property holds structurally. The comment at :1388-1394 records the trap for the next person, which is the part that outlives us both.
On the requested regression — the shape matters, and I measured it
A long-prompt + font-ready + parent-seek test committed against this repo's unit-test DOM would pass vacuously, on the broken code and the fixed code alike. Measured in happy-dom@20, the environment packages/player runs:
getBoundingClientRect()on the composer text returns{left: 0, width: 0}, andoffsetWidth/offsetLeftare0;- so
limitcomputes to-8, andif (!(limit > 0)) return;fires before any step is collected — zero caret keyframes regardless of prompt length; - and
document.fontsdoes not exist at all, so the second pass never runs.
Both preconditions for the defect are unreachable there. A green test in that environment would read as protection while asserting nothing, which is worse than the absence of one.
Where it could live is the render regression corpus — regression.yml renders named fixtures for real, in shards, which is the only place layout and document.fonts both exist. That is a new fixture plus shard budget rather than a unit test, so it is a reasonable follow-up ask but a heavy gate on a fix that is already structurally correct. Calibrating it as important, not a blocker.
CI at this head: 57 distinct checks, 0 not green.
Verdict: COMMENT — I would approve this on the merits; withholding the stamp only because a peer block is live at this head.
Reasoning: The defect is closed at the mechanism and verified by measurement; my one substantive difference is that the requested test shape cannot observe the bug it is meant to pin.
— Rames Jusso
Magi asked for a regression that makes the caret-scroll failure unable to return.
A render fixture cannot be that guard, and neither can a unit test. Both were
measured before choosing this:
- The unit-test DOM has no layout. In happy-dom `getBoundingClientRect()` is
{left: 0, width: 0} and offsetWidth/offsetLeft are 0, so the template's own
guard computes limit = -8 and returns before collecting a single keyframe,
whatever the prompt length. `document.fonts` does not exist there either, so
the rebuild never runs. Both preconditions are unreachable -- the test would
pass on the broken code and the fixed code alike. Rames flagged this; verified.
- A render fixture would not fail either. Rendering the long prompt through the
real pipeline on the broken vs fixed template gives SSIM 0.999994 over 894
frames with ZERO frames below 0.999. The renderer drives the composition by
explicit seek, and the duplicated keyframes are added last so they win, which
is exactly why `.kill()` on the master timeline stayed invisible. A golden mp4
would have cost a fixture and a shard and guarded nothing.
What is testable is the root cause, and it is a static property: `tl.to()` returns
the TIMELINE, `gsap.to()` returns a Tween, and the two read identically. So the
guard is a lint rule, which needs no DOM, fires on the source, and covers every
composition in the registry rather than this one template.
The rule keys on the timeline handles `collectTimelineVarNames` already finds,
which is what keeps `gsap.to()` and `Array.from()` out of it. Pushing a timeline
return into an array is flagged outright -- that is the collect-to-discard shape.
A bound return is flagged only once something tween-scoped is aimed at it, so a
pointless-but-harmless capture stays quiet.
Verified against the real files, not just synthetic ones: the pre-fix
chatgpt-exchange raises exactly one error and names the offending line; the fixed
file is clean. Across all 642 HTML files under registry/ and examples/ the rule
finds nothing, and lint output on the eight promoted blocks is identical to
origin/main error-for-error and warning-for-warning, so error severity costs no
existing template. 540 lint tests pass, 6 of them new.
Requested by Magi in review of #3562; unit-test DOM measurement by Rames Jusso.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regression added at A unit test cannot see the bug. Rames's happy-dom measurement is correct; I reproduced it:
A render fixture would not have failed either, which I did not expect and had to check before spending a golden mp4 on it. Long prompt, real pipeline, broken template vs fixed: The renderer drives the composition by explicit seek, and the duplicated keyframes are appended last so they win the tick. That is precisely why So the guard is the root cause, which is static and needs no DOM. Verified against the real files rather than synthetic ones:
This covers every composition in the registry instead of one template, and it fails in CI on the source, where the bug actually lives. If you still want the render-corpus fixture on top of this I'll add it, but I'd rather not check in a golden that cannot go red — say the word and I'll open it as a follow-up instead. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 458a6101d8c7a38ea9b34f28ee06efb8c76aef40.
The new guard satisfies the durability concern from my prior review, and it is the right kind of test for this failure.
I agree with the measured constraint: happy-dom has no usable layout metrics or document.fonts, so a nominal long-prompt unit test would return before producing a caret keyframe on both the broken and fixed implementations. A golden render is also the wrong oracle because explicit seek lets the duplicated last-added keyframes win, leaving near-identical pixels while live parent-driven playback is broken.
The lint rule instead pins the semantic root cause directly. It derives actual timeline handles through collectTimelineVarNames, rejects collection of tl.to() / from() / set() returns, and rejects a bound timeline return only when tween-scoped lifecycle methods are later aimed at it. It does not flag gsap.to(), ordinary chaining, Array.from(), or the nested-timeline fix. The six tests cover the original collect-and-kill shape, the bound-return shape, both valid alternatives, and two false-positive boundaries. Error severity is justified because the failure detaches a registered composition timeline.
I also verified the production fix remains the nested child shape, so the new rule and the repaired template agree. No blocker remains from my pass.
Required exact-head checks are still running with no completed failures; runtime contract and semantic title are already green. This approval is on code quality, not a merge instruction—wait for the remaining required contexts.
Verdict: APPROVE
Reasoning: The master-timeline defect is fixed structurally, and the committed lint rule is a non-vacuous, repo-wide regression guard that catches the precise API misuse which caused it.
— Magi
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Third-round review, at exact head 458a6101. Additive over @miguel-heygen's approval, which already covered the rule's adequacy, the six tests, why the runtime-test alternatives are vacuous here, and that the repaired template still uses the nested child. I re-derived the rule independently rather than checking its test names, and I'm adding a ground-truth control, a registry-wide sweep, and one coverage gap.
The rule is validated against ground truth — this is the part that matters. I ported the rule's logic and helpers verbatim (stripJsComments, escapeRegExp, collectTimelineVarNames) and first confirmed the port reproduces all six of this PR's own asserted outcomes, 6/6. Then I ran it against the actual pre-fix source at e299a982: it fires twice, at both caretScrollTweens.push(tl.to(...)) sites. Against the fix at this head: zero. A guard that fires on the specific code that shipped the defect, and not on its repair, is a different thing from a guard that fires on a paraphrase of it — last round the concern was precisely that a plausible-looking test here would pass identically on broken and fixed code. This one doesn't.
Strengths
gsap.ts:2168-2176keys the pattern oncollectTimelineVarNamesrather than on.to(. That's the design decision the rule lives or dies on: it's what keepsgsap.to()— which really does return a Tween — andArray.from()outside the match, instead of flagging every.to(in the file.- The bound-name branch (
:2180-2186) only reports once something tween-scoped is aimed at the alias, so ordinary chaining doesn't generate noise. Test 6 pins exactly that, and it's the test I'd have asked for. - The
fixHintprescribes the nested-timeline shape and says children keep their absolute times when the nest is added at 0. That second clause is the non-obvious half, and it's what makes the hint actionable instead of merely correct. - Blast radius is clean: I swept the rule over all 162 registry block HTML files at this head — 162/162 have a detected timeline (so the zero isn't a detector that saw nothing), 0 findings. No false positives across the registry, and no other latent instance of the pattern.
important — the detector cannot see the shape the fixHint prescribes
collectTimelineVarNames (gsap.ts:826) requires a declarator adjacent to the constructor:
/(?:const|let|var)\s+([A-Za-z_$][\w$]*)\s*=\s*gsap\.timeline\b/
The landed fix builds its nested timeline as let caretTl = null; (chatgpt-exchange.html:1395) and then caretTl = gsap.timeline(); (:1428) — a plain reassignment. So caretTl never enters timelineVars, and every rule keyed on that helper is blind to it.
Measured, not inferred: reintroducing the exact original collect-and-discard bug on the nested timeline — bad.push(caretTl.to(typedEl, {...}, at)) — yields 0 findings. The same mutation against a const-declared nested timeline yields 1. So the miss is specifically the declaration form, and it's the form this PR's own fix and fixHint use.
Why I'm not blocking on it: nothing is live (all 162 blocks are clean above), and widening the helper is not free — gsap_callback_dom_measurement (:2070, severity warning) shares it, so a wider regex newly registers reassigned timeline handles for that rule too and can surface new warnings across the registry. That deserves a deliberate change, not a one-line drive-by inside this PR. The cheap move here, if you want one, is a line in the rule comment noting that a reassigned timeline handle isn't covered — so the next author doesn't read a clean lint as proof of absence.
note — what the guard does and doesn't buy
The six tests run under the required Test context (bun run --filter '!@hyperframes/producer' test includes this package), so the rule's logic is pinned by CI. Its application to authored blocks is not: bun run lint is oxlint plus the check:* scripts and never calls the linter (and Lint isn't a required context anyway); registryBlocks.test.ts doesn't lint; registryComponents.test.ts covers registry/components and filters to media_missing_src; and the compile-time path (staticGuard.ts:19-21 consumed at htmlBundler.ts:814) only console.warns on an error-severity finding rather than failing the bundle. So this reliably catches the pattern for anyone running check, and it will not by itself turn a future PR red. That's pre-existing wiring shared by every rule in this file — not something this PR changed. I flag it only so "durable guard" isn't over-read.
CI at this head: 6 of the 8 required contexts are green. Test is still in progress — that's the one running these six tests — and regression / Tests on windows-latest are summary contexts that haven't been created yet while their matrices finish. Nothing is failing. So this is a code approval, not a merge signal.
Verdict: APPROVE
Reasoning: The fix I verified last round is intact, and the new rule is a real guard rather than a vacuous one — proven by firing twice on the exact pre-fix source, staying silent on the repair, and producing zero false positives across all 162 registry blocks. The detector gap is a coverage limit inherited from a shared helper, with nothing live behind it, so it belongs in a follow-up rather than gating this.
— Rames Jusso
3 participants
Why
A promoted template's variable descriptions told the remix agent where a slot sits on screen, not what it stands for. That is the wrong information for the one decision a remix has to get right: which slot carries the advertiser's identity.
Before, on
notification-cascade:Nothing there says whose name, whose domain, whose logo. Four remixes reported by a customer-facing team all shipped somebody else's identity in those slots — one of them shipped an invented brand, "Frame AI" at "the-ai-workspace.com", read out of Notion's own tagline. Another shipped an ad naming a company that does not appear anywhere on the page it was given.
After:
What changed
portraysmetadata, a machine-readable marker on each identity-bearing slot (subject_name,subject_domain,subject_logo,subject_tagline,competitor_name,authority_badge,recommended_position,host_identity). Documented indocs/concepts/variables.mdx. This is what lets a checker or an eval ask "did the advertiser reach the screen?" without pattern-matching prose.Scope, precisely. Five of the eight blocks are declaration-only:
ai-chat-reveal,message-thread-reveal,slack-notification-ad,share-sheet-carouselandnotification-cascadechange zero lines outside a variable declaration. Three change more, and each has its own section below —notes-reveal(+334/-125) gains a sign-off strip and two slots, andchatgpt-exchange(+78/-9) andclaude-exchange(+54/-6) gain a caret-following composer scroll. No slot is removed or renamed anywhere.An earlier revision of this description claimed the bytes outside
data-composition-variableswere unchanged everywhere butnotes-reveal. That was wrong, and it would have sent a reader straight past the exchange work — thanks to Rames for catching it.notes-reveal now has somewhere to say whose ad it is
The first draft of this PR recorded
notes-reveal's missing identity slots as out of scope. Second commit fixes it instead, because "this template cannot be attributed" is not a state a promoted ad template should stay in.The closing card ends at y=1708, leaving 212px of dotted scene. A sign-off strip lives there —
brandLogo(subject_logo) abovebrandDomain(subject_domain), in the card's own plum and Courier Prime. Putting it outside the card means the hand-lettered headline and the checklist keep the geometry they were designed with.Verified rather than assumed:
hyperframes check: the same 0 errors and the same warning counts asorigin/main, with one more contrast check passing (367/367 WCAG AA).hyperframes.heygen.com) and with a resolved symbol mark (Notion's cube overnotion.com). Both sit correctly.subject_domainscores 10.0 — every subject_domain slot carries gamma.com, a dimension that was permanently not applicable for this template before.No name slot, deliberately. There is no text slot here that can hold a bare brand name:
cardTop's marker line continues into the fixed words "OF ONE FILE", and a name set beside a wordmark reads twice — rendering it is how I found that, before committing. The mark plus the domain carry the attribution, which is what the other end-card templates do.Finishing this also surfaced two agent-side defects that only appear once a logo slot exists — a generated brand mark, and a domain recalled from training instead of taken from the submitted URL. Both are fixed in heygen-com/experiment-framework#48887.
The exchange composers scroll to follow the caret
promptis an editable slot in both exchange templates, but.comp-textis a fixed-width single line withwhite-space: preinside a composer that clips — and the packaged prompt already reaches the mic, so it has no headroom at all. A longer prompt slid under the mic and the send button and lost its tail, caret and all: the remix read as having typed into nowhere, cut mid-word. Both now scroll the typed text left to keep the caret visible, the way a real single-line input does, on the very per-character times the reveal already uses.Shrink-to-fit was the alternative and it is worse: with no headroom it shrinks the packaged prompt too, and it still cannot absorb a prompt twice as long without unreadable type. It was tried, measured, and dropped.
Verified rather than assumed:
claude-exchangeSSIM 1.000000 across 1284 frames;chatgpt-exchange0.999998 across 894 frames with 13 frames below 0.9999, against a 249-frame noise floor measured by rendering that template twice unchanged. Durations unchanged at 14.9s and 21.4s.That second line is a fix Magi's review turned up:
tl.to()returns the timeline, not the tween it created, so the rebuild was calling.kill()on the master timeline and stacking its keyframes on top of the ones it meant to replace — 49 before, 98 after, and the timeline off the ticker. Only an overflowing prompt reaches that path, i.e. exactly the remix the scroll exists for, which is why a packaged-defaults render never saw it. The steps now live in one nested child timeline that can be killed and replaced without reachingtl.Sequencing
These packages have to be re-promoted and the FE pin moved before any of this reaches users; the currently pinned commit (
3cacac5b) carries neither the descriptions norportrays(verified against the deployed zips). This PR is the source change only.🤖 Generated with Claude Code