fix(website): reduce CPU-heavy homepage motion - #880
Conversation
* gate SVG motion by visibility * polish animation segments * perf-tune to 60 FPS target
📝 WalkthroughWalkthroughThis PR overhauls the stage2 scroll-driven hero animation system through a shared ChangesStage2 Hero Animation System Overhaul
Sequence Diagram(s)sequenceDiagram
participant Viewport as IntersectionObserver
participant Switcher as bindMotionVisibility
participant Plugin as pauseDiagram/resumeDiagram
participant Timeline as timeline state
Viewport->>Switcher: entry.isIntersecting change
Switcher->>Switcher: toggle is-motion-active class
Viewport->>Plugin: out of view
Plugin->>Timeline: pause
Viewport->>Plugin: back in view
Plugin->>Timeline: resume
stateDiagram-v2
[*] --> Expanding: progress increases
Expanding --> Settling: aperture reaches target radius
Settling --> Settled: settled detection triggered
Settled --> Expanding: progress changes again
note right of Settling
syncApertureVisual applies
smooth easing via RAF
calculates settled state
end note
sequenceDiagram
participant Anime as anime timeline
participant FlightHelpers as translate helpers
participant Placement as ensureAutoPlacement
participant DOM as DOM layer element
Anime->>Anime: onUpdate fired each frame
Anime->>FlightHelpers: call setLayerFlightMotion(...)
Anime->>FlightHelpers: call setLayerFlightSize(...)
FlightHelpers->>DOM: apply translate/width/height
Placement->>FlightHelpers: setLayerFlightFrame(travelProgress)
Placement->>Placement: sync source magazine motion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/src/_includes/sections/hero.njk (1)
177-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize scrubber input as non-focusable while container is
aria-hidden.
aria-hidden="true"is set on.stage2-scrubber, but the nested range input starts focusable until JS sync runs. Set initialdisabledandtabindex="-1"in markup to keep DOM/accessibility state consistent from first paint.Suggested patch
- <input class="stage2-scrubber-input" type="range" min="0" max="1000" value="0" aria-label="Stage 2 sequence position" aria-valuemin="0" aria-valuemax="1000" aria-valuenow="0"> + <input class="stage2-scrubber-input" type="range" min="0" max="1000" value="0" aria-label="Stage 2 sequence position" aria-valuemin="0" aria-valuemax="1000" aria-valuenow="0" disabled tabindex="-1">🤖 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 `@website/src/_includes/sections/hero.njk` around lines 177 - 188, The range input element with class stage2-scrubber-input is currently focusable even though its parent container stage2-scrubber has aria-hidden="true", creating an accessibility inconsistency. Add the disabled attribute and tabindex="-1" to the stage2-scrubber-input element to ensure it remains non-focusable from initial page load, keeping the DOM and accessibility state consistent throughout the component lifecycle.
🧹 Nitpick comments (1)
website/src/assets/site.css (1)
1174-1184: ⚡ Quick winRoute the new stage2/debug colors through design tokens.
These changed blocks add raw hex/RGBA colors in theme-sensitive UI surfaces, which bypasses the dark-first token system and makes future palette changes harder. Prefer existing custom properties or define stage2-specific variables at the stage root.
As per coding guidelines, “Use CSS custom properties for theming in stylesheets” and “Use CSS custom properties matching the dark-first design system in src/assets/site.css.”
Also applies to: 6262-6312, 6540-6545, 6875-6880
🤖 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 `@website/src/assets/site.css` around lines 1174 - 1184, The body:not(.docs-body)::after pseudo-element rule contains a hard-coded background color of `#08090b` instead of using CSS custom properties from the design token system. Replace the background property value with an appropriate CSS custom property that aligns with the dark-first design system (such as an existing token for background colors or a new stage2-specific variable if one doesn't exist). Ensure any new custom properties are defined at the appropriate root scope and follow the established naming conventions for CSS tokens in the site.css file.Source: Coding guidelines
🤖 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.
Inline comments:
In `@website/src/assets/model-slicing.js`:
- Around line 620-625: In the heroSvgMotionResumeTimer callback function,
replace the stale latestScrollProgress variable with a fresh computation from
the current scroll bounds using the observer path. Instead of passing
latestScrollProgress directly to stageProgressFromScrollProgress, recompute the
scroll progress from the current scroll bounds before passing it to
stageProgressFromScrollProgress to ensure canRunHeroSvgMotion receives accurate
current scroll data.
In `@website/src/assets/plugin-architecture.js`:
- Around line 238-245: The `is-motion-active` class is added to the diagram
before animation playback begins, but the completion callback function passed to
timeline.call() does not remove this class after the observer is disconnected
and timeline is cleared. Add a statement to remove the `is-motion-active` class
from the diagram element within this callback, after the existing observer
cleanup code, so that motion-gated CSS styles do not remain active indefinitely
after the animation completes.
In `@website/src/assets/site.css`:
- Around line 5680-5686: Fix the stylelint errors in the opacity property
declarations in site.css. The multiline calc() function needs proper operator
placement formatting - ensure the multiplication and addition operators are
positioned according to stylelint rules for operator placement in calc()
expressions. Additionally, add empty lines before any non-custom property
declarations (like opacity) that appear after custom property declarations to
separate them properly. Apply these formatting fixes to the opacity property
with calc() at lines 5680-5686, and also to the similar opacity declarations at
lines 5817-5820 and 6539-6540.
In `@website/src/assets/title-reveal.js`:
- Line 130: The early return at the stage2 check in the title-reveal.js file
exits before canceling any pending title reveal timers, allowing previously
queued reveals to still execute during stage2. Move the timer cancellation logic
that clears any queued reveal timers before the early return statement when
is-stage2-active is detected, and also apply the same stage2 guard check before
any direct queueReveal calls to prevent them from being scheduled when stage2 is
already active.
In `@website/src/assets/topology-switcher.js`:
- Around line 116-143: The bindMotionVisibility function does not properly keep
the MOTION_ACTIVE_CLASS synchronized when the user changes their reduced motion
preference, and the early return prevents proper initialization. Refactor by
creating a separate sync() function that handles toggling MOTION_ACTIVE_CLASS
based on both the intersection observer visibility state and the
reduceMotion.matches state, then call this sync function from both the
IntersectionObserver callback entries.forEach and the reduceMotion
addEventListener change handler to ensure consistent synchronization with the
prefers-reduced-motion media query at all times.
---
Outside diff comments:
In `@website/src/_includes/sections/hero.njk`:
- Around line 177-188: The range input element with class stage2-scrubber-input
is currently focusable even though its parent container stage2-scrubber has
aria-hidden="true", creating an accessibility inconsistency. Add the disabled
attribute and tabindex="-1" to the stage2-scrubber-input element to ensure it
remains non-focusable from initial page load, keeping the DOM and accessibility
state consistent throughout the component lifecycle.
---
Nitpick comments:
In `@website/src/assets/site.css`:
- Around line 1174-1184: The body:not(.docs-body)::after pseudo-element rule
contains a hard-coded background color of `#08090b` instead of using CSS custom
properties from the design token system. Replace the background property value
with an appropriate CSS custom property that aligns with the dark-first design
system (such as an existing token for background colors or a new stage2-specific
variable if one doesn't exist). Ensure any new custom properties are defined at
the appropriate root scope and follow the established naming conventions for CSS
tokens in the site.css file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83fef3bd-452a-4aaa-a85c-04a2562e7e5d
📒 Files selected for processing (8)
website/src/_includes/components/document-head.njkwebsite/src/_includes/sections/hero.njkwebsite/src/assets/hero-viz.jswebsite/src/assets/model-slicing.jswebsite/src/assets/plugin-architecture.jswebsite/src/assets/site.csswebsite/src/assets/title-reveal.jswebsite/src/assets/topology-switcher.js
020545b to
7e95806
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/src/assets/model-slicing.js (1)
2833-2833: 💤 Low valueConsider using
progressFromScroll()for consistency with the SVG motion resume fix.This fallback still uses
latestScrollProgress, which can be stale when using the scroll observer. The primarycurrentStageProgresspath is usually populated, but for consistency with the fix at line 638, consider:- stageProgress = currentStageProgress || stageProgressFromScrollProgress(latestScrollProgress); + stageProgress = currentStageProgress || stageProgressFromScrollProgress(progressFromScroll());The risk is low since
currentStageProgressis typically populated before this runs, but fresh computation is more reliable.🤖 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 `@website/src/assets/model-slicing.js` at line 2833, The fallback logic in the line with the stageProgress assignment is using latestScrollProgress which can be stale when the scroll observer is active. For consistency with the SVG motion resume fix applied at line 638, replace the fallback from stageProgressFromScrollProgress(latestScrollProgress) with a call to progressFromScroll() instead. This ensures that fresh computation is used rather than potentially stale scroll progress values, maintaining consistency across the codebase even though currentStageProgress is typically populated before this line executes.
🤖 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 `@website/src/assets/model-slicing.js`:
- Line 2833: The fallback logic in the line with the stageProgress assignment is
using latestScrollProgress which can be stale when the scroll observer is
active. For consistency with the SVG motion resume fix applied at line 638,
replace the fallback from stageProgressFromScrollProgress(latestScrollProgress)
with a call to progressFromScroll() instead. This ensures that fresh computation
is used rather than potentially stale scroll progress values, maintaining
consistency across the codebase even though currentStageProgress is typically
populated before this line executes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77744a4d-13aa-4adb-88e9-f39b51e02d5e
📒 Files selected for processing (6)
website/src/_includes/sections/hero.njkwebsite/src/assets/model-slicing.jswebsite/src/assets/plugin-architecture.jswebsite/src/assets/site.csswebsite/src/assets/title-reveal.jswebsite/src/assets/topology-switcher.js
🚧 Files skipped from review as they are similar to previous changes (4)
- website/src/_includes/sections/hero.njk
- website/src/assets/topology-switcher.js
- website/src/assets/plugin-architecture.js
- website/src/assets/site.css
Summary
Summary by CodeRabbit
New Features
Improvements
Bug Fixes