[WEB-4845] Eliminate forced reflows in Header and Expander components #996
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR introduces performance optimizations for Header and Expander components by implementing two new custom hooks ( Changes
Sequence Diagram(s)sequenceDiagram
actor User as User (scrolling)
participant Header as Header Component
participant IO as IntersectionObserver
participant RAF as requestAnimationFrame
participant DOM as Scrollpoint Elements
participant State as Active Class State
User->>DOM: Scroll near scrollpoints
DOM->>IO: Elements enter/exit viewport
IO->>IO: Calc closest to header (64px margin)
IO->>RAF: Schedule state update
RAF->>State: Update activeClassName
State->>Header: Re-render with new class
Header->>User: Visual feedback (styled scrollpoint)
sequenceDiagram
actor Component as Expander Component
participant RO as ResizeObserver
participant RAF as requestAnimationFrame
participant State as contentHeight State
participant Render as Component Render
Component->>RO: Observe inner element
Component->>RO: Mount complete
Note over RO: Content resizes
RO->>RAF: Schedule height read
RAF->>State: Update contentHeight (rounded)
State->>Render: Trigger re-render
Render->>Component: Recompute memoized height & showControls
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. Comment |
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
2 times, most recently
from
7607085 to
9afd9d0
Compare
kennethkalmer
changed the title
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
from
57304a4 to
65e4310
Compare
kennethkalmer
changed the title
kennethkalmer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@fix-ably-ui.md`:
- Line 11: The review points out a stale filename reference to Header; update
all references from src/core/Header.js to src/core/Header.tsx (including
imports, documentation, comments, and any error/stack trace annotations) so they
match the actual Header component; search for occurrences of "Header.js" and
replace them with "Header.tsx" and verify the Header component export name
(Header) is unchanged so imports like import Header from 'src/core/Header'
resolve to the .tsx file.
In `@src/core/hooks/use-themed-scrollpoints.test.ts`:
- Around line 10-43: The test suite currently overwrites
global.IntersectionObserver and global.requestAnimationFrame in beforeEach
(assigning via the mocked factory that captures observerCallback and sets
mockObserve/mockUnobserve/mockDisconnect) but never restores the originals,
causing cross-file test pollution; update the file to save the originals (e.g.,
const originalIntersectionObserver = global.IntersectionObserver and const
originalRequestAnimationFrame = global.requestAnimationFrame) before mocking in
beforeEach and restore them in afterEach (reassign global.IntersectionObserver =
originalIntersectionObserver and global.requestAnimationFrame =
originalRequestAnimationFrame), keeping the existing mock variables
(observerCallback, mockObserve, mockUnobserve, mockDisconnect) intact.
In `@src/core/hooks/use-themed-scrollpoints.ts`:
- Around line 18-21: In the use-themed-scrollpoints hook, handle the case where
scrollpoints becomes empty by clearing any active class/state: inside the
useEffect that currently returns when scrollpoints.length === 0, instead of
returning immediately, remove the active class from the previously active
element (via the same ref or query used elsewhere), reset the active index/state
(e.g., activeIndex or activeRef) to null/undefined, and clear any stored
references/timers used by functions like setActiveClass or onScroll; update the
cleanup so the hook leaves no stale theming when scrollpoints goes from
non-empty to empty.
🧹 Nitpick comments (4)
src/core/Expander.tsx (1)
69-80: Addfocus-baseclass for keyboard accessibility.As per coding guidelines, interactive elements should include the
focus-baseclass for consistent focus states.♻️ Proposed fix
<button data-testid="expander-controls" className={cn( heightThreshold === 0 && !expanded ? "" : "mt-4", - "cursor-pointer font-bold text-gui-blue-default-light hover:text-gui-blue-hover-light", + "cursor-pointer font-bold text-gui-blue-default-light hover:text-gui-blue-hover-light focus-base transition-colors", controlsClassName, )} >vite.config.mts (1)
20-23: Consider adding tests foruseContentHeighthook.The
use-themed-scrollpoints.test.tsis included, but there's no corresponding test file for the newuseContentHeighthook. Given that this hook is critical for the Expander performance fix, test coverage would help ensure reliability.Would you like me to help generate unit tests for the
useContentHeighthook, or open an issue to track this?src/core/hooks/use-themed-scrollpoints.ts (1)
1-4: Reuse the sharedHEADER_HEIGHTconstant.Keeping a local
64risks drift if the header height changes; importing the shared constant keeps the hook aligned withHeader.Proposed refactor
-import { useState, useEffect, useRef } from "react"; -import { ThemedScrollpoint } from "../Header/types"; - -const HEADER_HEIGHT = 64; +import { useState, useEffect, useRef } from "react"; +import { ThemedScrollpoint } from "../Header/types"; +import { HEADER_HEIGHT } from "../utils/heights";src/core/Header.tsx (1)
250-259: Cancel the throttled scroll handler on cleanup.The
es-toolkit/compatthrottle function attaches acancel()method to prevent trailing calls from firing after the listener is removed. Without this, a scheduled trailing invocation could fire after unmount and trigger state updates.Proposed refactor
return () => { window.removeEventListener("scroll", throttledHandleScroll); + throttledHandleScroll.cancel(); };
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
2 times, most recently
from
c231305 to
026fa7c
Compare
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
from
4394cbf to
fe01643
Compare
aralovelace
left a comment
aralovelace
left a comment
There was a problem hiding this comment.
@kennethkalmer this is really great Ken! I only notice that the border under Meganav when it scrollback to the top its not disappearing anymore?
aralovelace
left a comment
aralovelace
left a comment
There was a problem hiding this comment.
Also same for light theme
aralovelace
left a comment
aralovelace
left a comment
There was a problem hiding this comment.
I also notice in the website app its not coming back to top nav version as well
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
2 times, most recently
from
f5b53f1 to
5fce2b6
Compare
|
@aralovelace thank you for the great reviews and testing, I really really appreciate it. I think I've fixed it now, in Safari on the Voltaire review I can see the navigation behave correctly now. Mind giving it another spin? |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/hooks/use-content-height.ts`:
- Around line 26-34: The requestAnimationFrame callback inside the
ResizeObserver can run after unmount and call setContentHeight on an unmounted
component; fix by capturing the RAF id returned from requestAnimationFrame (and
the observer) in scope when scheduling, canceling that RAF in the hook cleanup
(via cancelAnimationFrame) and disconnecting observerRef
(observerRef.current.disconnect()), and optionally guard the callback with a
mounted flag before calling setContentHeight; update the code around
observerRef, ResizeObserver, requestAnimationFrame, and setContentHeight to
ensure cancellation/guarding in cleanup.
🧹 Nitpick comments (7)
src/core/hooks/use-content-height.ts (1)
42-42:refas dependency won't trigger re-runs whenref.currentchanges.The
RefObjectitself is stable across renders, so changes toref.currentwon't cause the effect to re-run. This is fine if the ref is attached to a stable element, but if the element changes (e.g., conditional rendering), the observer won't update. Consider documenting this limitation or using a callback ref pattern if dynamic element changes are expected.src/core/Expander.tsx (1)
69-80: Addfocus-baseandtransition-colorsclasses to the interactive button.Per coding guidelines, interactive elements should include the
focus-baseclass for consistent focus styling andtransition-colorsfor hover state transitions.♻️ Proposed fix
<button data-testid="expander-controls" className={cn( heightThreshold === 0 && !expanded ? "" : "mt-4", - "cursor-pointer font-bold text-gui-blue-default-light hover:text-gui-blue-hover-light", + "cursor-pointer font-bold text-gui-blue-default-light hover:text-gui-blue-hover-light focus-base transition-colors", controlsClassName, )} >src/core/Header/types.ts (1)
1-4: Consider adding JSDoc documentation for the public type.Since
ThemedScrollpointis part of the public API (re-exported fromHeader.tsx), adding JSDoc would improve developer experience and align with coding guidelines.📝 Suggested documentation
+/** + * Represents a themed scrollpoint that triggers header appearance changes + * when the element with the specified id scrolls into view. + */ export type ThemedScrollpoint = { + /** The DOM element id to observe */ id: string; + /** CSS classes to apply to the header when this scrollpoint is active */ className: string; };src/core/hooks/use-themed-scrollpoints.test.ts (1)
508-536: Minor: rAF spy created after mock is already in place.The
requestAnimationFrameis already mocked inbeforeEach, so spying on it at line 520 after the hook is rendered works but could be clearer if the spy was set up before triggering the callback.💡 Alternative approach
it("uses requestAnimationFrame for state updates", () => { const elem = document.createElement("div"); elem.id = "zone1"; document.body.appendChild(elem); const scrollpoints = [ { id: "zone1", className: "theme-light" }, { id: "zone2", className: "theme-dark" }, ]; + const rafSpy = vi.spyOn(global, "requestAnimationFrame"); + renderHook(() => useThemedScrollpoints(scrollpoints)); - const rafSpy = vi.spyOn(global, "requestAnimationFrame"); - // Simulate intersection observerCallback( [ { target: elem, isIntersecting: true, } as unknown as IntersectionObserverEntry, ], {} as IntersectionObserver, ); expect(rafSpy).toHaveBeenCalled(); rafSpy.mockRestore(); });src/core/hooks/use-themed-scrollpoints.ts (2)
4-4: Consider importingHEADER_HEIGHTfrom the shared constants.
HEADER_HEIGHTis already defined insrc/core/utils/heights.ts(as shown in the relevant code snippets). Duplicating the constant creates a maintenance risk if the value changes.♻️ Proposed fix
import { useState, useEffect, useRef } from "react"; import { ThemedScrollpoint } from "../Header/types"; +import { HEADER_HEIGHT } from "../utils/heights"; -const HEADER_HEIGHT = 64;
58-60: Minor: Defensive fallback forboundingClientRect.The fallback to
getBoundingClientRect()is defensive coding. Per the IntersectionObserver spec,boundingClientRectis always populated on entries. This fallback adds a synchronous layout read that the PR aims to eliminate, though it should rarely (if ever) be triggered.Consider removing the fallback or adding a comment explaining when it might be needed:
- const rect = - entry.boundingClientRect || entry.target.getBoundingClientRect(); + // boundingClientRect is always available per IntersectionObserver spec + const rect = entry.boundingClientRect;src/core/Header.tsx (1)
233-259:previousVisibilityresets on each effect run due tonoticeBannerVisibledependency.The
previousVisibilityvariable is a closure that gets re-initialized tonoticeBannerVisiblewhenever the effect re-runs. SincenoticeBannerVisibleis in the dependency array (line 259), changing visibility causes the effect to re-run, potentially skipping the deduplication check on the first scroll after the change.Consider using a ref to persist across re-runs:
♻️ Proposed fix using ref
+ const previousVisibilityRef = useRef(noticeBannerVisible); + useEffect(() => { if (!isNoticeBannerEnabled) { return; } const noticeElement = document.querySelector('[data-id="ui-notice"]'); if (!noticeElement) { console.warn("Header: Notice element not found"); return; } - let previousVisibility = noticeBannerVisible; - const handleScroll = () => { const scrollY = window.scrollY; const isNoticeHidden = noticeElement.classList.contains( "ui-announcement-hidden", ); const shouldBeVisible = scrollY <= COLLAPSE_TRIGGER_DISTANCE && !isNoticeHidden; - if (shouldBeVisible !== previousVisibility) { - previousVisibility = shouldBeVisible; + if (shouldBeVisible !== previousVisibilityRef.current) { + previousVisibilityRef.current = shouldBeVisible; setNoticeBannerVisible(shouldBeVisible); } }; // ...rest unchanged - }, [isNoticeBannerEnabled, noticeBannerVisible]); + }, [isNoticeBannerEnabled]);
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
2 times, most recently
from
02eb191 to
0daff73
Compare
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
from
d8ec1f1 to
6b2fa71
Compare
|
@aralovelace I see there are still some issues with the themed scroll points, let me resolve them properly first |
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
from
6b2fa71 to
9095ee3
Compare
Replace expensive getBoundingClientRect() calls with IntersectionObserver API
to eliminate forced reflows during scroll handling.
- `src/core/hooks/use-themed-scrollpoints.ts` - Custom hook using IntersectionObserver for scrollpoint detection
- `src/core/Header/types.ts` - Extracted ThemedScrollpoint type to avoid circular dependencies
- `src/core/hooks/use-themed-scrollpoints.test.ts` - Comprehensive unit tests for the new hook
- `src/core/Header.tsx`:
- Replace scrollpointClasses state with useThemedScrollpoints hook
- Optimize notice banner visibility logic with cached DOM reference
- Add passive flag to scroll listener for iOS performance
- Remove getBoundingClientRect calls from scroll handler
- `vite.config.mts` - Add new hook test to unit test suite
- `package.json` / `pnpm-lock.yaml` - Add @testing-library/react dev dependency
- **Before**: 71-193ms of forced reflows per scroll event
- **After**: <2ms overhead (98% improvement)
- **Eliminated**: All getBoundingClientRect calls during scroll
- **Added**: Passive scroll listener for iOS optimization
1. **IntersectionObserver API** - Async intersection detection eliminates forced reflows
2. **Cached DOM references** - Query notice element once on mount, not every scroll
3. **Passive scroll listener** - `{ passive: true }` flag for iOS performance
4. **Change detection** - Only update state when values actually change
5. **requestAnimationFrame batching** - Batch IntersectionObserver callbacks
- 14 new unit tests for useThemedScrollpoints hook (all passing)
- All existing tests pass
- Backward compatible - no API changes to HeaderProps or ThemedScrollpoint type
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Document the analysis and planning for Header and Expander performance optimizations that eliminate forced reflows causing iOS device overheating. Files: - docs/plans/header-performance-optimization.md - docs/plans/expander-performance-optimization.md These docs capture the original requirements, problem analysis, and proposed solutions for future reference. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Replace synchronous clientHeight reads with ResizeObserver API to eliminate forced reflows during expand/collapse and resize events. Changes: - Created useContentHeight hook using ResizeObserver - Replaced manual clientHeight reads in Expander component - Removed throttled resize listener (ResizeObserver handles this) - Added useMemo for showControls and height calculations Performance Impact: - Before: 67-92ms of forced reflows - After: <5ms overhead (94% improvement) Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Documents Observer API patterns (IntersectionObserver, ResizeObserver), testing strategies, and common pitfalls from Header/Expander performance work. Includes custom hook guidelines, Storybook performance testing workflow, and checklist for async hook development. Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
kennethkalmer
force-pushed
the
WEB-4845-performance-fixes
branch
from
9095ee3 to
06163b6
Compare
|
@aralovelace I think I nailed it down, tested with Storybook in Chrome, Safari & Firefox and the themed scrollpoints behaved much better. |
Jira Ticket Link / Motivation
WEB-4845 - Performance optimization to eliminate iOS device overheating
Our website (ably.com) is experiencing severe CPU issues on iOS devices causing overheating complaints. Chrome DevTools Performance profiling identified two critical bottlenecks:
Both components were using synchronous layout queries (
getBoundingClientRect(),clientHeight) that force the browser to recalculate layout on every interaction.Summary of Changes
This PR implements performance optimizations for both components using modern browser APIs, plus comprehensive documentation for future performance work.
Header Optimization (useThemedScrollpoints)
New Files:
src/core/hooks/use-themed-scrollpoints.ts- Custom hook using IntersectionObserversrc/core/hooks/use-themed-scrollpoints.test.ts- Comprehensive unit tests (15 tests)src/core/Header/types.ts- Extracted ThemedScrollpoint typeModified Files:
src/core/Header.tsx- Replaced getBoundingClientRect() with useThemedScrollpoints hookvite.config.mts- Added new hook test to suitepackage.json/pnpm-lock.yaml- Added @testing-library/react dev dependencyPerformance Impact:
Bug Fixes:
Expander Optimization (useContentHeight)
New Files:
src/core/hooks/use-content-height.ts- Custom hook using ResizeObserverModified Files:
src/core/Expander.tsx- Replaced clientHeight with useContentHeight hookPerformance Impact:
Pages Affected:
/chat- Was 67ms reflows (52% of total) → Now <5ms/pubsub- Was 92ms reflows (96% of total) → Now <5msDocumentation
Planning Docs:
docs/plans/header-performance-optimization.md- Header optimization requirements and analysisdocs/plans/expander-performance-optimization.md- Expander optimization requirements and analysisAgent Development Guide:
AGENTS.md- Added comprehensive performance optimization section (337 lines):These docs capture patterns, pitfalls, and best practices discovered during this work to help future agents and developers avoid common issues.
How to Manually Test
1. Performance Testing (Chrome DevTools)
Header Test:
getBoundingClientRectcalls in scroll handlerExpander Test:
clientHeightreads2. Functional Testing
Themed Scrollpoints:
Expander:
3. Run Tests
pnpm testAll 206 tests should pass.
Key Optimizations
Testing
Merge/Deploy Checklist
docs/plans/Notes
17.13.0-dev.0daff736Summary by CodeRabbit
Refactor
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.