Repository navigation
perf(renderer): pause long-lived status animations offscreen and honor reduced motion - #37162
Conversation
d8acb00 to
005530a
Compare
b0c628e to
19b51cf
Compare
005530a to
38d6180
Compare
38d6180 to
865e3fc
Compare
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description provides strong coverage of the changes, rationale, measurements, visual proof, and testing. However, it omits the required Risk section and does not provide an actual linked issue; re… | Add a Linked Issue section with the applicable issue reference. Add a Risk section with the risk level, affected users, possible failure modes, and rollback method. Complete any applicable checklist items and required template sections befo… |
| Docstring Coverage | |
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main renderer performance and reduced-motion changes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description provides strong coverage of the changes, rationale, measurements, visual proof, and testing. However, it omits the required Risk section and does not provide an actual linked issue; referencing stacked PR
#37153does not satisfy the template's issue requirement.Resolution
Add a Linked Issue section with the applicable issue reference. Add a Risk section with the risk level, affected users, possible failure modes, and rollback method. Complete any applicable checklist items and required template sections before merging.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 8 functions across 8 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
…r reduced motion The stale-review shimmer animates background-position on the main thread for as long as review data stays stale, once per sidebar card. It now steps 30 times per sweep and pauses when its card is out of view. Pending-check spinners also pause offscreen, and pending checks, the phone-driver dot and the notebook busy pulse stop under reduced motion.
865e3fc to
d8130bf
Compare
Stacked on #37153 (reuses its offscreen observer). Retarget to
mainafter that merges.ELI5
Some status animations (the shimmer on a sidebar card whose PR info is stale, the spinner on pending CI checks) can run for an hour or more. One of them redraws its text every single frame. Now they redraw less often, pause when scrolled out of view, and stop for people who asked their OS to reduce motion.
What Changed
steps(30)per 2.4 s sweep, via a newshimmer-steps-*utility (--shimmer-timing, defaultlinear, so other shimmers are unchanged). It also pauses while its card is out of view.[data-animation-offscreen]now pauses animations on the marked element and its descendants (animation-play-state: paused). The agent ring keeps its stricteranimation: nonefrom perf(renderer): stop agent spinners that are scrolled out of view #37153 so reveal re-anchors phase.observeOffscreenAnimationnow acceptsnullso it can be used directly as a React ref.CHECK_COLOR.pending(same amber) instead of repeating the raw class.Why: audit of long-lived animations
Any running CSS animation keeps the compositor ticking at the display refresh rate. If it animates a paint property, the main thread also repaints every frame. The common practice for always-on indicators is: pause when offscreen, pause when the document is hidden, use
steps()to cap frame work, and honor reduced motion. Audit of every infinite animation in the renderer (~450animate-spin, ~83animate-pulse, 12 custom keyframes):steps()"Hidden" is handled by Chromium throttling a hidden/occluded window (re-enabled in #9395). Everything else found is bounded (button and fetch loaders, skeletons, onboarding/tips).
Not changed here (follow-up): the client-hosted browser page position loop runs
requestAnimationFrameevery frame while any client-hosted page is attached. It tracks pane moves that fire no events, so replacing it needs its own change.Measurements
Minimal Electron page, 20 stale-style shimmer labels plus 20 rings, 60 Hz, 4 s trace, renderer threads (ms of CPU per second):
linear(before)steps(30)(after)Offscreen labels drop to zero animation work, the same as the rings in #37153 (200-workspace lineage: renderer+GPU −55%, main thread −81%). Paint cost scales with refresh rate, so on 120/144 Hz displays the linear shimmer costs 2–2.4× more, and
steps(30)stays at 12.5 updates/s.Verified in a dev build: the shimmer computes
steps(30)/2.4s; descendants of an offscreen-marked element computepaused; the agent ring under the attribute computesanimation-name: none.Visual Proof
Stale shimmer, before (linear) vs after (
steps(30)), same 2.4 s sweep:stale-shimmer-before-after.mp4
Reduced-motion and offscreen changes have no visible effect (static icon, or element not on screen).
Testing
pnpm testover sidebar, right-sidebar, terminal-pane, notebook and the observer tests: 589 files / 5399 tests passpnpm tc:web, oxlint/oxfmt,pnpm check:code-quality:changed origin/main(0 findings)IntersectionObserverare platform-independent. Web client shares the renderer. Mobile is native and unaffected.Agent skill upstream boundary
Checklist