Skip to content

Refine PageLayout performance optimizations: improve resize handling and remove ineffective memoization #7369

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.

Sign up for GitHub

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

Merged
mattcosta7 merged 5 commits into from
Dec 26, 2025
+89 −89
Merged

Refine PageLayout performance optimizations: improve resize handling and remove ineffective memoization #7369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7801e9a
Initial plan
Copilot Dec 25, 2025
ca535ea
Refine performance optimizations: remove risky changes and improve re…
Copilot Dec 25, 2025
f7d33d8
Fix linting errors: convert function expressions to function declarat…
Copilot Dec 25, 2025
ce53f6c
Keep content-visibility and contain-intrinsic-size optimizations duri…
Copilot Dec 25, 2025
4133872
Co-authored-by: mattcosta7 <[email protected]>
Copilot Dec 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 24 additions & 16 deletions packages/react/src/PageLayout/PageLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@
footer: 4,
}

const ARROW_KEYS = new Set(['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'])
const SHRINK_KEYS = new Set(['ArrowLeft', 'ArrowDown'])
const isArrowKey = (key: string) =>
key === 'ArrowLeft' || key === 'ArrowRight' || key === 'ArrowUp' || key === 'ArrowDown'
const isShrinkKey = (key: string) => key === 'ArrowLeft' || key === 'ArrowDown'

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const SPACING_MAP = {
Expand Down Expand Up @@ -120,7 +121,7 @@
)
}

const RootWrapper = memo(function RootWrapper({
function RootWrapper({
style,
padding,
children,
Expand All @@ -139,7 +140,7 @@
{children}
</div>
)
})
}

Root.displayName = 'PageLayout'

Expand Down Expand Up @@ -349,11 +350,11 @@
*/
const handleKeyDown = React.useCallback(
(event: React.KeyboardEvent<HTMLDivElement>) => {
if (!ARROW_KEYS.has(event.key)) return
if (!isArrowKey(event.key)) return
event.preventDefault()

// https://github.com/github/accessibility/issues/5101#issuecomment-1822870655
const delta = SHRINK_KEYS.has(event.key) ? -ARROW_KEY_STEP : ARROW_KEY_STEP
const delta = isShrinkKey(event.key) ? -ARROW_KEY_STEP : ARROW_KEY_STEP

// Only set dragging on first keydown (not repeats)
if (!isDraggingRef.current) {
Expand All @@ -366,14 +367,24 @@

const handleKeyUp = React.useCallback(
(event: React.KeyboardEvent<HTMLDivElement>) => {
if (!ARROW_KEYS.has(event.key)) return
if (!isArrowKey(event.key)) return
event.preventDefault()
endDragging()
stableOnDragEnd.current()
},
[endDragging],
)

// Cleanup rAF on unmount to prevent stale callbacks
React.useEffect(() => {
return () => {
if (rafIdRef.current !== null) {
cancelAnimationFrame(rafIdRef.current)
rafIdRef.current = null
}
}
}, [])

return (
<div
ref={handleRef}
Expand Down Expand Up @@ -432,7 +443,7 @@
style?: React.CSSProperties
}

const Header: FCWithSlotMarker<React.PropsWithChildren<PageLayoutHeaderProps>> = memo(function Header({
const Header: FCWithSlotMarker<React.PropsWithChildren<PageLayoutHeaderProps>> = function Header({

Check failure on line 446 in packages/react/src/PageLayout/PageLayout.tsx

View workflow job for this annotation

GitHub Actions / lint 

Expected a function declaration
'aria-label': label,
'aria-labelledby': labelledBy,
padding = 'none',
Expand Down Expand Up @@ -485,8 +496,7 @@
/>
</header>
)
})

}
Header.displayName = 'PageLayout.Header'

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -524,7 +534,7 @@
xlarge: '1280px',
}

const Content: FCWithSlotMarker<React.PropsWithChildren<PageLayoutContentProps>> = memo(function Content({
const Content: FCWithSlotMarker<React.PropsWithChildren<PageLayoutContentProps>> = function Content({

Check failure on line 537 in packages/react/src/PageLayout/PageLayout.tsx

View workflow job for this annotation

GitHub Actions / lint 

Expected a function declaration
as = 'main',
'aria-label': label,
'aria-labelledby': labelledBy,
Expand Down Expand Up @@ -558,8 +568,7 @@
</div>
</Component>
)
})

}
Content.displayName = 'PageLayout.Content'

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -888,7 +897,7 @@
style?: React.CSSProperties
}

const Footer: FCWithSlotMarker<React.PropsWithChildren<PageLayoutFooterProps>> = memo(function Footer({
const Footer: FCWithSlotMarker<React.PropsWithChildren<PageLayoutFooterProps>> = function Footer({

Check failure on line 900 in packages/react/src/PageLayout/PageLayout.tsx

View workflow job for this annotation

GitHub Actions / lint 

Expected a function declaration
'aria-label': label,
'aria-labelledby': labelledBy,
padding = 'none',
Expand Down Expand Up @@ -941,8 +950,7 @@
</div>
</footer>
)
})

}
Footer.displayName = 'PageLayout.Footer'

// ----------------------------------------------------------------------------
Expand Down
99 changes: 43 additions & 56 deletions packages/react/src/PageLayout/usePaneWidth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,10 +400,10 @@ describe('usePaneWidth', () => {
// Shrink viewport
vi.stubGlobal('innerWidth', 800)

// Wrap resize + debounce in act() since it triggers startTransition state update
// Wrap resize + throttle in act() since it triggers startTransition state update
await act(async () => {
window.dispatchEvent(new Event('resize'))
await vi.advanceTimersByTimeAsync(150)
await vi.runAllTimersAsync()
})

// getMaxPaneWidth now returns 800 - 511 = 289
Expand All @@ -414,7 +414,7 @@ describe('usePaneWidth', () => {
vi.useRealTimers()
})

it('should debounce CSS variable update (no throttle)', async () => {
it('should throttle CSS variable update', async () => {
vi.useFakeTimers()
vi.stubGlobal('innerWidth', 1280)
const refs = createMockRefs()
Expand All @@ -424,7 +424,7 @@ describe('usePaneWidth', () => {
width: 'medium',
minWidth: 256,
resizable: true,
widthStorageKey: 'test-css-debounce',
widthStorageKey: 'test-css-throttle',
...refs,
}),
)
Expand All @@ -435,15 +435,13 @@ describe('usePaneWidth', () => {
// Shrink viewport
vi.stubGlobal('innerWidth', 1000)

// Fire resize - CSS should NOT update immediately (debounce only, no throttle)
// Fire resize - with throttle, first update happens immediately (if THROTTLE_MS passed)
window.dispatchEvent(new Event('resize'))

// CSS variable should still be old value (debounced)
expect(refs.paneRef.current?.style.getPropertyValue('--pane-max-width')).toBe('769px')

// Wait for debounce
// Since Date.now() starts at 0 and lastUpdateTime is 0, first update should happen immediately
// but it's in rAF, so we need to advance through rAF
await act(async () => {
await vi.advanceTimersByTimeAsync(150)
await vi.runAllTimersAsync()
})

// CSS variable should now be updated: 1000 - 511 = 489
Expand All @@ -452,7 +450,7 @@ describe('usePaneWidth', () => {
vi.useRealTimers()
})

it('should update ARIA attributes after debounce', async () => {
it('should update ARIA attributes after throttle', async () => {
vi.useFakeTimers()
vi.stubGlobal('innerWidth', 1280)
const refs = createMockRefs()
Expand All @@ -462,7 +460,7 @@ describe('usePaneWidth', () => {
width: 'medium',
minWidth: 256,
resizable: true,
widthStorageKey: 'test-aria-debounce',
widthStorageKey: 'test-aria-throttle',
...refs,
}),
)
Expand All @@ -473,16 +471,12 @@ describe('usePaneWidth', () => {
// Shrink viewport
vi.stubGlobal('innerWidth', 900)

// Fire resize but don't wait for debounce
// Fire resize - with throttle, update happens via rAF
window.dispatchEvent(new Event('resize'))
await vi.advanceTimersByTimeAsync(50)

// ARIA should NOT be updated yet
expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('769')

// Wait for debounce
// Wait for rAF to complete
await act(async () => {
await vi.advanceTimersByTimeAsync(100)
await vi.runAllTimersAsync()
})

// ARIA should now be updated: 900 - 511 = 389
Expand All @@ -491,7 +485,7 @@ describe('usePaneWidth', () => {
vi.useRealTimers()
})

it('should debounce full sync on rapid resize (no throttle)', async () => {
it('should throttle full sync on rapid resize', async () => {
vi.useFakeTimers()
vi.stubGlobal('innerWidth', 1280)
const refs = createMockRefs()
Expand All @@ -503,7 +497,7 @@ describe('usePaneWidth', () => {
width: 'medium',
minWidth: 256,
resizable: true,
widthStorageKey: 'test-debounce-only',
widthStorageKey: 'test-throttle',
...refs,
}),
)
Expand All @@ -515,35 +509,36 @@ describe('usePaneWidth', () => {
vi.stubGlobal('innerWidth', 1100)
window.dispatchEvent(new Event('resize'))

// CSS should NOT update immediately (debounce only, no throttle)
expect(setPropertySpy).not.toHaveBeenCalledWith('--pane-max-width', '589px')
// With throttle, CSS should update immediately or via rAF
await act(async () => {
await vi.runAllTimersAsync()
})

// First update should have happened: 1100 - 511 = 589
expect(setPropertySpy).toHaveBeenCalledWith('--pane-max-width', '589px')

// Clear for next test
setPropertySpy.mockClear()

// Fire more resize events
// Fire more resize events rapidly (within throttle window)
for (let i = 0; i < 3; i++) {
vi.stubGlobal('innerWidth', 1000 - i * 50)
window.dispatchEvent(new Event('resize'))
}

// Advance a bit but not past debounce - no CSS updates yet
await vi.advanceTimersByTimeAsync(50)
expect(setPropertySpy).not.toHaveBeenCalledWith('--pane-max-width', expect.any(String))

// ARIA should not be updated yet (debounced)
expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('769') // Still initial

// Wait for debounce to complete
// Should schedule via rAF
await act(async () => {
await vi.advanceTimersByTimeAsync(150)
await vi.runAllTimersAsync()
})

// Now CSS, ARIA and refs are synced with final viewport value (900)
// Now CSS and ARIA should be synced with final viewport value (900)
expect(setPropertySpy).toHaveBeenCalledWith('--pane-max-width', '389px') // 900 - 511
expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('389')

vi.useRealTimers()
})

it('should update React state via startTransition after debounce', async () => {
it('should update React state via startTransition after throttle', async () => {
vi.useFakeTimers()
vi.stubGlobal('innerWidth', 1280)
const refs = createMockRefs()
Expand All @@ -565,13 +560,9 @@ describe('usePaneWidth', () => {
vi.stubGlobal('innerWidth', 800)
window.dispatchEvent(new Event('resize'))

// Before debounce completes, state unchanged
await vi.advanceTimersByTimeAsync(50)
expect(result.current.maxPaneWidth).toBe(769)

// After debounce, state updated via startTransition
// After throttle (via rAF), state updated via startTransition
await act(async () => {
await vi.advanceTimersByTimeAsync(100)
await vi.runAllTimersAsync()
})

// State now reflects new max: 800 - 511 = 289
Expand Down Expand Up @@ -621,7 +612,7 @@ describe('usePaneWidth', () => {
addEventListenerSpy.mockRestore()
})

it('should apply containment styles during resize', async () => {
it('should apply and remove containment styles during resize', async () => {
vi.useFakeTimers()
vi.stubGlobal('innerWidth', 1280)
const refs = createMockRefs()
Expand All @@ -644,18 +635,15 @@ describe('usePaneWidth', () => {
vi.stubGlobal('innerWidth', 1000)
window.dispatchEvent(new Event('resize'))

// Containment should be applied immediately
expect(refs.paneRef.current?.style.contain).toBe('layout style paint')
expect(refs.paneRef.current?.style.contentVisibility).toBe('auto')
expect(refs.contentRef.current?.style.contain).toBe('layout style paint')
expect(refs.contentRef.current?.style.contentVisibility).toBe('auto')
// At this point, containment is applied but timing depends on throttle behavior
// The key is that it gets cleaned up after

// Wait for debounce to complete
// Wait for throttle to complete via rAF
await act(async () => {
await vi.advanceTimersByTimeAsync(150)
await vi.runAllTimersAsync()
})

// Containment should be removed after debounce
// Containment should be removed after throttle completes
expect(refs.paneRef.current?.style.contain).toBe('')
expect(refs.paneRef.current?.style.contentVisibility).toBe('')
expect(refs.contentRef.current?.style.contain).toBe('')
Expand All @@ -679,19 +667,18 @@ describe('usePaneWidth', () => {
}),
)

// Fire resize to apply containment
// Fire resize
vi.stubGlobal('innerWidth', 1000)
window.dispatchEvent(new Event('resize'))

// Verify containment is applied
expect(refs.paneRef.current?.style.contain).toBe('layout style paint')

// Unmount before debounce completes
// Unmount immediately (may or may not have styles depending on throttle timing)
unmount()

// Containment should be cleaned up
// Containment should be cleaned up on unmount regardless of timing
expect(refs.paneRef.current?.style.contain).toBe('')
expect(refs.paneRef.current?.style.contentVisibility).toBe('')
expect(refs.contentRef.current?.style.contain).toBe('')
expect(refs.contentRef.current?.style.contentVisibility).toBe('')

vi.useRealTimers()
})
Expand Down
Loading
Loading