Revert "Improve PageLayout pane drag performance with pointer capture and GPU-accelerated transforms" #7274
Conversation
… and GPU…" This reverts commit 12e12f5.
|
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
There was a problem hiding this comment.
Pull request overview
This PR reverts performance optimizations for PageLayout pane dragging that were introduced in #7251. The revert is being done for release debugging purposes and may need to be included in the next release. The changes restore the previous implementation which uses React state management and standard event handling instead of the optimized approach with pointer capture, direct DOM manipulation, and GPU-accelerated transforms.
Key Changes
- Reverted from pointer events with capture to mouse events for drag handling
- Restored React state-based width management instead of direct DOM manipulation via
style.setProperty() - Removed performance optimization CSS rules (containment, will-change, transforms)
- Deleted performance testing stories and related Axe test exclusions
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/PageLayout/PageLayout.tsx | Reverted drag implementation from optimized pointer capture to state-based mouse events; restored localStorage integration via state; removed viewport width tracking with ResizeObserver |
| packages/react/src/PageLayout/snapshots/PageLayout.test.tsx.snap | Updated snapshots to include --pane-width in inline styles (now set via React state) |
| packages/react/src/PageLayout/PageLayout.module.css | Removed CSS performance optimizations (containment, will-change, transforms) and restored simpler drag styling with body-level cursor/selection rules |
| packages/react/src/PageLayout/PageLayout.performance.stories.tsx | Deleted performance testing stories file (492 lines) |
| e2e/components/Axe.test.ts | Removed skip entries for deleted performance stories |
| .changeset/olive-heads-enter.md | Deleted changeset as changes are being reverted |
| const body = document.body as HTMLElement | undefined | ||
| body?.removeAttribute('data-page-layout-dragging') | ||
| } | ||
| }, [isDragging, isKeyboardDrag, currentWidth, minWidth, maxWidth]) |
There was a problem hiding this comment.
This effect has a problematic dependency array. Including currentWidth, minWidth, and maxWidth will cause the effect to re-run and recreate all event listeners whenever any of these values change. This is inefficient and can cause issues:
- During drag operations, width values update frequently, causing constant listener churn
- New listeners are created with each state change, but the old listeners in currently executing handlers still reference stale values
- The effect cleanup runs on every state change, potentially removing listeners mid-drag
Consider using refs for these values (e.g., currentWidthRef.current) instead of including them in the dependency array, or restructure the logic to avoid this frequent re-registration.
| // TODO: Support touch events | ||
| if (isDragging || isKeyboardDrag) { | ||
| window.addEventListener('mousemove', handleDrag) | ||
| window.addEventListener('keydown', handleKeyDrag) |
There was a problem hiding this comment.
The keydown event listener is attached globally to window, which means it will fire for ALL keyboard events anywhere in the document, not just when the drag handle is focused. This will cause the pane to resize whenever arrow keys are pressed anywhere on the page during a keyboard drag session.
This is different from the onKeyDown handler on the drag handle element (line 287) which only fires when the handle itself receives the key event. The global listener should either:
- Check if the drag handle has focus before processing the event
- Only handle continuation of a drag already initiated via the handle's onKeyDown
- Be removed in favor of handling all keyboard input through the element's onKeyDown handler
| onDragStart?.() | ||
| } | ||
| }} | ||
| onKeyDown={(event: React.KeyboardEvent) => { | ||
| if ( | ||
| event.key === 'ArrowLeft' || | ||
| event.key === 'ArrowRight' || | ||
| event.key === 'ArrowUp' || | ||
| event.key === 'ArrowDown' | ||
| ) { | ||
| setIsKeyboardDrag(true) | ||
| onDragStart?.() |
There was a problem hiding this comment.
The onDragStart callback is being called but it's not provided by the parent component. Looking at the usage in the Pane component (line 719), no onDragStart prop is passed to VerticalDivider, so onDragStart?.() will never execute. This makes the code confusing and suggests incomplete implementation.
If onDragStart is intended for future use, consider removing it from this revert. If it's meant to be used internally, it should be implemented in the parent component.
| React.useEffect(() => { | ||
| if (paneRef.current !== null) { | ||
| const paneStyles = getComputedStyle(paneRef.current as Element) | ||
| const maxPaneWidthDiffPixels = paneStyles.getPropertyValue('--pane-max-width-diff') | ||
| const minWidthPixels = paneStyles.getPropertyValue('--pane-min-width') | ||
| const paneWidth = paneRef.current.getBoundingClientRect().width | ||
| const maxPaneWidthDiff = Number(maxPaneWidthDiffPixels.split('px')[0]) | ||
| const minPaneWidth = Number(minWidthPixels.split('px')[0]) | ||
| const viewportWidth = window.innerWidth | ||
| const maxPaneWidth = viewportWidth > maxPaneWidthDiff ? viewportWidth - maxPaneWidthDiff : viewportWidth | ||
| setMinWidth(minPaneWidth) | ||
| setMaxWidth(maxPaneWidth) | ||
| setCurrentWidth(paneWidth || 0) | ||
| } | ||
| }, [paneRef, isKeyboardDrag, isDragging]) |
There was a problem hiding this comment.
This effect reads from paneRef.current to calculate width constraints, but paneRef is a ref object which doesn't trigger re-renders when its .current property changes. The effect depends on isDragging and isKeyboardDrag to re-run, which means the width constraints are only recalculated when drag state changes.
This could lead to stale values if:
- The pane is resized through other means (window resize, CSS changes)
- The component mounts before
paneRef.currentis assigned
Consider adding a ResizeObserver or window.resize listener to keep these values in sync, or move this logic into the drag handlers where it's actually needed.
| const maxPaneWidthDiff = Number(maxPaneWidthDiffPixels.split('px')[0]) | ||
| const minPaneWidth = Number(minWidthPixels.split('px')[0]) |
There was a problem hiding this comment.
The string parsing using .split('px')[0] is fragile and will fail if the CSS variable value doesn't include 'px' (e.g., if it's a number without units, or uses different units like 'rem' or 'em'). This will result in parsing the entire string and getting NaN.
Consider using parseInt() or parseFloat() which automatically stops at the first non-numeric character, or add validation to handle cases where the value might not be in the expected format.
| const maxPaneWidthDiff = Number(maxPaneWidthDiffPixels.split('px')[0]) | |
| const minPaneWidth = Number(minWidthPixels.split('px')[0]) | |
| const maxPaneWidthDiff = parseFloat(maxPaneWidthDiffPixels) | |
| const minPaneWidth = parseFloat(minWidthPixels) |
| if (isPaneWidth(width)) { | ||
| return defaultPaneWidth[width] | ||
| } else if (isCustomWidthOptions(width)) { | ||
| return Number(width.default.split('px')[0]) |
There was a problem hiding this comment.
The same fragile string parsing pattern .split('px')[0] is used here. If width.default doesn't contain 'px' or is in a different format, this will fail silently and return NaN, which will then be passed to Number() resulting in NaN.
Consider using parseInt(width.default, 10) which is more robust and handles various string formats.
| return Number(width.default.split('px')[0]) | |
| return parseInt(width.default, 10) |
| }, | ||
| [paneRef], | ||
| ) | ||
| setCurrentWidth(currentWidth + delta) |
There was a problem hiding this comment.
The keyboard drag handler modifies currentWidth directly via setCurrentWidth(currentWidth + delta), but this creates a stale closure issue. The effect depends on currentWidth in its dependency array, so when currentWidth changes, the entire effect re-runs, creating new event listeners with the updated value. However, during a single keydown event, the state update from setCurrentWidth won't be reflected until the next render, which could lead to incorrect width calculations during rapid key presses.
Consider using a functional state update: setCurrentWidth(prev => prev + delta) to ensure you're always working with the most recent value.
| setCurrentWidth(currentWidth + delta) | |
| setCurrentWidth(prev => prev + delta) |
llastflowers
added
the
integration-tests: skipped manually
Reverts #7251 for release debugging; may need to be included in next release.