Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 30, 2025

Dialog component now reads data-dialog-scroll-optimized attribute directly from body element instead of calling useFeatureFlag('primer_react_css_has_selector_perf'). This decouples Dialog from the feature flag system and ensures consistent optimization behavior across multiple React roots.

Changes

Dialog.tsx

  • Removed useFeatureFlag import and hook call
  • Check document.body.hasAttribute('data-dialog-scroll-optimized') inside useEffect at mount time
  • Changed dependency array from [usePerfOptimization] to []

FeatureFlags.tsx

  • Changed useEffect to useIsomorphicLayoutEffect to ensure body attribute is set before child effects run
  • Fixes timing issue where Dialog would mount before FeatureFlags set the attribute

Rationale

  • Cross-root consistency: Multiple React roots with different flag values now behave consistently once any provider sets the body attribute
  • Alignment with CSS: JavaScript logic now matches CSS selector body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll)
  • Simpler coupling: Dialog observes global DOM state rather than depending on feature flag context

Changelog

Changed

  • Dialog component now checks body attribute instead of calling useFeatureFlag hook
  • FeatureFlags provider uses useIsomorphicLayoutEffect for body attribute management

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Internal refactor with no API changes or user-facing behavior changes.

Testing & Reviewing

All existing Dialog tests pass, including feature flag behavior tests. The attribute check happens at mount time, so Dialog's optimization state is determined when it renders.

Merge checklist

Original prompt

Summary

Refactor the Dialog component to check for the existence of data-dialog-scroll-optimized attribute on the body element instead of calling useFeatureFlag('primer_react_css_has_selector_perf') directly.

Reasoning

  1. Solves mixed flag values across React roots: If multiple React roots exist with different feature flag values, checking the body attribute ensures consistent behavior. Once any FeatureFlags provider sets data-dialog-scroll-optimized on body, all dialogs will use the optimized path regardless of their own provider's flag value.

  2. Decouples Dialog from FeatureFlags: Dialog doesn't need to import or call useFeatureFlag. It observes the DOM state that the provider has established.

  3. Matches the CSS logic: The CSS already uses the attribute to short-circuit:

    body:not([data-dialog-scroll-optimized]):has(.Dialog.DisableScroll)

    Having JS also check the attribute keeps the logic aligned.

  4. Performance: document.body.hasAttribute('data-dialog-scroll-optimized') is an O(1) attribute lookup - not prohibitively expensive. This is the same type of operation browsers do constantly during CSS matching.

Changes Required

In packages/react/src/Dialog/Dialog.tsx:

  1. Remove the useFeatureFlag import and call
  2. Check document.body.hasAttribute('data-dialog-scroll-optimized') inside the useEffect instead
  3. Change the dependency array to [] since we read from DOM at mount time
React.useEffect(() => {
  const scrollbarWidth = window.innerWidth - document.body.clientWidth
  const dialog = dialogRef.current
  const usePerfOptimization = document.body.hasAttribute('data-dialog-scroll-optimized')

  dialog?.classList.add(classes.DisableScroll)
  document.body.style.setProperty('--prc-dialog-scrollgutter', `${scrollbarWidth}px`)

  if (usePerfOptimization) {
    document.body.setAttribute('data-dialog-scroll-disabled', '')
  }

  return () => {
    dialog?.classList.remove(classes.DisableScroll)

    const remainingDialogs = document.querySelectorAll(`.${classes.DisableScroll}`)

    if (remainingDialogs.length === 0) {
      document.body.style.removeProperty('--prc-dialog-scrollgutter')
      if (usePerfOptimization) {
        document.body.removeAttribute('data-dialog-scroll-disabled')
      }
    }
  }
}, [])

In packages/react/src/FeatureFlags/FeatureFlags.tsx:

No changes needed - the provider should still set data-dialog-scroll-optimized on body when the flag is enabled.

In tests:

Update Dialog.test.tsx to:

  1. Remove dependency on useFeatureFlag mocking if present
  2. Ensure tests set up the body attribute directly when testing the optimized path
  3. Verify Dialog correctly reads the attribute state at mount time

Notes

  • The FeatureFlags provider remains responsible for setting data-dialog-scroll-optimized on body
  • Dialog becomes a consumer of that DOM state rather than the feature flag directly
  • This is a refactor with no user-facing behavior changes when flags are consistent across roots

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@changeset-bot
Copy link

changeset-bot bot commented Dec 30, 2025

⚠️ No Changeset found

Latest commit: b223180

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Dec 30, 2025
@github-actions
Copy link
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 integration-tests: skipped manually label to skip these checks.

Copilot AI changed the title [WIP] Refactor Dialog component to check body attribute for scroll optimization Refactor Dialog to check DOM attribute instead of useFeatureFlag hook Dec 30, 2025
Copilot AI requested a review from mattcosta7 December 30, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants