-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: bound invalid render durations #2671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1481,10 +1481,10 @@ export const gsapRules: LintRule<LintContext>[] = [ | |
| message: | ||
| "GSAP tween uses `repeat: -1` (infinite). Infinite repeats break the deterministic " + | ||
| "capture engine which seeks to exact frame times. Use a finite repeat count calculated " + | ||
| "from the composition duration: `repeat: Math.floor(duration / cycleDuration) - 1`.", | ||
| "from the composition duration: `repeat: Math.max(0, Math.floor(duration / cycleDuration) - 1)`.", | ||
| fixHint: | ||
| "Replace `repeat: -1` with a finite count, e.g. `repeat: Math.floor(totalDuration / singleCycleDuration) - 1`. " + | ||
| "Use Math.floor (not Math.ceil) to ensure the animation fits within the total duration.", | ||
| "Replace `repeat: -1` with a finite count, e.g. `repeat: Math.max(0, Math.floor(totalDuration / singleCycleDuration) - 1)`. " + | ||
| "Use Math.floor (not Math.ceil) so the animation fits, and clamp at zero so a short composition cannot evaluate to -1.", | ||
| snippet: truncateSnippet(snippet), | ||
| }); | ||
| } | ||
|
|
@@ -1510,14 +1510,41 @@ export const gsapRules: LintRule<LintContext>[] = [ | |
| "For example, Math.ceil(10.5 / 2) - 1 = 5 repeats → 6 cycles × 2s = 12s, exceeding 10.5s.", | ||
| fixHint: | ||
| "Use `Math.floor` instead of `Math.ceil` to ensure the animation fits within the duration: " + | ||
| "`repeat: Math.floor(totalDuration / cycleDuration) - 1`. " + | ||
| "`repeat: Math.max(0, Math.floor(totalDuration / cycleDuration) - 1)`. " + | ||
| "Math.floor(10.5 / 2) - 1 = 4 repeats → 5 cycles × 2s = 10s ✓", | ||
| snippet: truncateSnippet(snippet), | ||
| }); | ||
| } | ||
| return findings; | ||
| }, | ||
|
|
||
| // gsap_repeat_floor_unclamped | ||
| ({ scripts }) => { | ||
| const findings: HyperframeLintFinding[] = []; | ||
| // A direct floor-minus-one expression becomes GSAP's infinite -1 sentinel when | ||
| // the visible duration is shorter than one full cycle. Math.max-wrapped forms | ||
| // intentionally do not match because `repeat:` is followed by Math.max, not Math.floor. | ||
| const pattern = /repeat\s*:\s*Math\.floor\s*\([^)]+\)\s*-\s*1/g; | ||
| for (const { snippet } of scanScriptsForRegexMatches(scripts, pattern, { | ||
| stripComments: false, | ||
| contextBefore: 40, | ||
| contextAfter: 40, | ||
| })) { | ||
| findings.push({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Question — warning severity for a pattern that can reach the deterministic-capture-breaking |
||
| code: "gsap_repeat_floor_unclamped", | ||
| severity: "warning", | ||
| message: | ||
| "GSAP repeat calculation can evaluate to -1 when the composition is shorter than one cycle, " + | ||
| "which GSAP interprets as an infinite repeat.", | ||
| fixHint: | ||
| "Clamp the finite repeat count at zero: " + | ||
| "`repeat: Math.max(0, Math.floor(totalDuration / cycleDuration) - 1)`.", | ||
| snippet: truncateSnippet(snippet), | ||
| }); | ||
| } | ||
| return findings; | ||
| }, | ||
|
|
||
| // scene_layer_missing_visibility_kill | ||
| ({ scripts, tags }) => { | ||
| const findings: HyperframeLintFinding[] = []; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,17 +67,15 @@ export interface ValidateNoGpuEncodeInput { | |
| */ | ||
| export const SYSTEM_FONT_USED = "SYSTEM_FONT_USED"; | ||
|
|
||
| /** | ||
| * Typed code for {@link validateDistributedDuration}. A duration this large | ||
| * almost always means an unbounded runtime timeline escaped into plan(), | ||
| * e.g. GSAP `repeat: -1` reporting its internal sentinel duration. Letting | ||
| * that reach chunk planning creates billions of frames and turns an authoring | ||
| * error into worker churn. | ||
| */ | ||
| /** Typed code for invalid duration metadata resolved by the shared browser probe. */ | ||
| export const DISTRIBUTED_DURATION_OUT_OF_RANGE = "DISTRIBUTED_DURATION_OUT_OF_RANGE"; | ||
| /** Generic alias; the legacy value remains stable for workflow retry policies. */ | ||
| export const RENDER_DURATION_OUT_OF_RANGE = DISTRIBUTED_DURATION_OUT_OF_RANGE; | ||
|
|
||
| /** Distributed renders are operationally bounded to one day of output. */ | ||
| /** All render paths are operationally bounded to one day of output. */ | ||
| export const MAX_DISTRIBUTED_DURATION_SECONDS = 24 * 60 * 60; | ||
| /** Generic alias retained alongside the distributed public API. */ | ||
| export const MAX_RENDER_DURATION_SECONDS = MAX_DISTRIBUTED_DURATION_SECONDS; | ||
|
|
||
| /** | ||
| * Reject any config that would let GPU encode or hardware-GL slip into a | ||
|
|
@@ -143,13 +141,13 @@ export function validateNoSystemFonts(compiledHtml: string): void { | |
| } | ||
| } | ||
|
|
||
| export function validateDistributedDuration(input: { | ||
| export function validateRenderDuration(input: { | ||
| duration: number; | ||
| totalFrames: number; | ||
| fps: number; | ||
| }): void { | ||
| const { duration, totalFrames, fps } = input; | ||
| const maxFrames = Math.ceil(MAX_DISTRIBUTED_DURATION_SECONDS * fps); | ||
| const maxFrames = Math.ceil(MAX_RENDER_DURATION_SECONDS * fps); | ||
| if ( | ||
| Number.isFinite(duration) && | ||
| duration > 0 && | ||
|
|
@@ -163,12 +161,21 @@ export function validateDistributedDuration(input: { | |
| } | ||
|
|
||
| throw new PlanValidationError( | ||
| DISTRIBUTED_DURATION_OUT_OF_RANGE, | ||
| `[planValidation] Distributed render duration is out of range: ` + | ||
| RENDER_DURATION_OUT_OF_RANGE, | ||
| `[planValidation] Render duration is out of range: ` + | ||
| `duration=${String(duration)}s totalFrames=${String(totalFrames)} fps=${String(fps)} ` + | ||
| `(maxDuration=${String(MAX_DISTRIBUTED_DURATION_SECONDS)}s, maxFrames=${String(maxFrames)}). ` + | ||
| `(maxDuration=${String(MAX_RENDER_DURATION_SECONDS)}s, maxFrames=${String(maxFrames)}). ` + | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocker — this message change breaks a pre-existing integration test. The generalization from |
||
| `This usually means an unbounded timeline escaped into render planning, such as ` + | ||
| `GSAP repeat:-1 / yoyo loops without an explicit finite root duration. Add a finite ` + | ||
| `data-duration or replace infinite repeats with a finite repeat count before rendering.`, | ||
| ); | ||
| } | ||
|
|
||
| /** Backward-compatible distributed entry point for existing adopters. */ | ||
| export function validateDistributedDuration(input: { | ||
| duration: number; | ||
| totalFrames: number; | ||
| fps: number; | ||
| }): void { | ||
| validateRenderDuration(input); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Nit — regex only catches inline
repeat: Math.floor(...) - 1, not the variable-assigned form.\brepeat\s*:\s*Math\.floorrequiresMath.floorto sit directly afterrepeat:. So the equivalent pattern via a local variable —const count = Math.floor(duration / cycle) - 1; gsap.timeline({ repeat: count });— is invisible to the rule. Not blocking (the direct-write form is by far the most common), but if HF authors reach for that idiom the warning never fires. Worth either (a) a follow-up rule that tracks theMath.floor(...) - 1binding and its use inrepeat:, or (b) a rule-doc note saying the check is inline-form-only so authors don't assume the linter covers all paths. — Rames D Jusso