Removes a resize subscription and expensive :has selectors from pagelayout
#7302
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 8c5edd0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
integration-tests: recommended
Contributor
|
👋 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 |
mattcosta7
commented
Dec 11, 2025
Comment on lines
-840
to
-888
| handleRef={handleRef} | ||
| onDrag={(delta, isKeyboard = false) => { | ||
| const deltaWithDirection = isKeyboard ? delta : position === 'end' ? -delta : delta | ||
| const maxWidth = maxPaneWidthRef.current | ||
|
|
||
| if (isKeyboard) { | ||
| // Clamp keyboard delta to stay within bounds | ||
| const newWidth = Math.max(minPaneWidth, Math.min(maxWidth, currentWidthRef.current! + deltaWithDirection)) | ||
| if (newWidth !== currentWidthRef.current) { | ||
| currentWidthRef.current = newWidth | ||
| paneRef.current?.style.setProperty('--pane-width', `${newWidth}px`) | ||
| updateAriaValues(handleRef.current, {current: newWidth}) | ||
| } | ||
| } else { | ||
| // Apply delta directly via CSS variable for immediate visual feedback | ||
| if (paneRef.current) { | ||
| const newWidth = currentWidthRef.current! + deltaWithDirection | ||
| const clampedWidth = Math.max(minPaneWidth, Math.min(maxWidth, newWidth)) | ||
|
|
||
| // Only update if the clamped width actually changed | ||
| // This prevents drift when dragging against min/max constraints | ||
| if (clampedWidth !== currentWidthRef.current) { | ||
| paneRef.current.style.setProperty('--pane-width', `${clampedWidth}px`) | ||
| currentWidthRef.current = clampedWidth | ||
| updateAriaValues(handleRef.current, {current: clampedWidth}) | ||
| } | ||
| } | ||
| } | ||
| }} | ||
| // Save final width to localStorage (skip React state update to avoid reconciliation) | ||
| onDragEnd={() => { | ||
| // For mouse drag: The CSS variable is already set and currentWidthRef is in sync. | ||
| // We intentionally skip setPaneWidth() to avoid triggering expensive React | ||
| // reconciliation with large DOM trees. The ref is the source of truth for | ||
| // subsequent drag operations. | ||
| setWidthInLocalStorage(currentWidthRef.current!) | ||
| }} | ||
| position={positionProp} | ||
| // Reset pane width on double click | ||
| onDoubleClick={() => { | ||
| const defaultWidth = getDefaultPaneWidth(width) | ||
| // Update CSS variable and ref directly - skip React state to avoid reconciliation | ||
| if (paneRef.current) { | ||
| paneRef.current.style.setProperty('--pane-width', `${defaultWidth}px`) | ||
| currentWidthRef.current = defaultWidth | ||
| updateAriaValues(handleRef.current, {current: defaultWidth}) | ||
| } | ||
| setWidthInLocalStorage(defaultWidth) | ||
| }} |
Contributor
Author
There was a problem hiding this comment.
Moved into the drag handle
mattcosta7
commented
Dec 11, 2025
| } | ||
| } | ||
| }) | ||
| viewportWidthObserver.observe(document.documentElement) |
Contributor
Author
There was a problem hiding this comment.
this was causing observation on any change inside the body, not just page resizing
mattcosta7
commented
Dec 11, 2025
| * ARIA values are set in JSX for SSR accessibility, | ||
| * then updated via DOM manipulation during drag for performance | ||
| */ | ||
| const DragHandle: React.FC<DragHandleProps> = ({ |
Contributor
Author
There was a problem hiding this comment.
pulls this out to a a child, and avoid passing through props to the divider
mattcosta7
commented
Dec 11, 2025
| } | ||
| // If pane is resizable, the divider should be draggable | ||
| draggable={resizable} | ||
| handleRef={handleRef} |
Contributor
Author
There was a problem hiding this comment.
moves the draghandle to a child - so we can keep VerticalDividerslimmer
mattcosta7
commented
Dec 11, 2025
| // ResizeObserver on document.documentElement fires on any content change (typing, etc), | ||
| // causing INP regressions. Window resize only fires on viewport changes. | ||
| // eslint-disable-next-line github/prefer-observers | ||
| window.addEventListener('resize', throttledUpdateMax) |
Contributor
Author
There was a problem hiding this comment.
replaces the resize observer with a more granular raf
github-actions
bot
requested a deployment
to
storybook-preview-7302
mattcosta7
force-pushed
the
fix-resize-obsever-subscripotion
branch
from
c7abafa to
70f8c48
Compare
mattcosta7
changed the base branch from
main
to
revert-7305-revert-7275-revert-7274-revert-7251-mc/copilot/sub-pr-7248
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/8678 |
mattcosta7
changed the title
:has selectors from pagelayout
mattcosta7
merged commit 8b08d12
into
revert-7305-revert-7275-revert-7274-revert-7251-mc/copilot/sub-pr-7248
46 checks passed
Merged
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses a ResizeObserver performance regression in
@primer/react'sPageLayoutcomponent and introduces several optimizations and improvements. The main changes focus on improving drag performance by replacing expensive CSS selectors with a more efficient JavaScript-driven approach, updating the drag handle logic, and enhancing performance testing stories.Performance and architectural optimizations:
:has()CSS selectors with a JavaScript-manageddata-draggingattribute on.PageLayoutContent, improving drag performance by avoiding costly style recalculations. [1] [2] [3]ResizeObserversubscription logic for viewport width tracking, simplifying code and relying on more direct CSS variable reads for pane sizing.Accessibility and usability improvements:
aria-valuemin,aria-valuemax,aria-valuenow,aria-valuetext) to the drag handle for improved accessibility, updating them efficiently during drag operations. [1] [2] [3]ARROW_KEY_STEP), making adjustments easier and more consistent. [1] [2]Performance testing enhancements:
SearchInput(withAutocomplete), making the test scenarios more representative of real-world usage. [1] [2] [3] [4] [5] [6]Bug fixes:
ResizeObserverby simplifying how pane sizing and drag state are managed.Context and maintainability:
contentRefin thePageLayoutContextto allow coordinated updates and more maintainable code when managing drag state. [1] [2] [3] [4]These changes collectively result in smoother drag interactions, better accessibility, and more maintainable code in the
PageLayoutcomponent.Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist