Improve robustness of responsive tabs logic to avoid flaky sb tests#3017
Improve robustness of responsive tabs logic to avoid flaky sb tests#3017beaesguerra wants to merge 6 commits intomainfrom
Conversation
…ld be flaky for the ResponsiveNavigationTabs Toggle Icon sb interaction test. It was because we would check overflow when the tabs data changes with icons, rather than when the icons are rendered. To make it more robust, we check overflow once the tabs layout is rendered and when there are any mutations to it or its descendants
🦋 Changeset detectedLatest commit: 9fb2465 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 |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (ad886d8) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3017"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3017 |
|
Size Change: +73 B (+0.06%) Total Size: 122 kB 📦 View Changed
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-hljyvzdsmn.chromatic.com/ Chromatic results:
|
…s of responsive logic in ResponsiveTabs and ResponsiveNavigationTabs by using a MutationObserver to trigger when we should check for overflow to decide if a dropdown or horizontal tabs layout should be used
… rendering cycle when the container has padding/margin
…r resize observer when width changes
beaesguerra
force-pushed
the
reduce-flakiness-responsive-tabs
branch
from
418dfc5 to
9fb2465
Compare
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean, well-reasoned fix for the icon rendering race condition.
Extended reasoning...
Overview
The PR touches a single hook file () plus a changeset entry. The changes address a race condition where overflow was measured before icons had finished rendering, causing flaky Storybook interaction tests.
Changes
Three focused improvements:
- MutationObserver: A new wires up a on (the horizontal-tabs scrollable wrapper). This defers the overflow check until after DOM mutations settle (icons mounted, labels changed, tabs added/removed), which is exactly when the measurement is meaningful.
- containerWidthRef deduplication: The now skips if the container width hasn't changed, avoiding spurious re-checks on non-width resize events.
- tabsWidthRef gap fix: The stored threshold now accounts for any gap (padding/margin) between the container and the scrollable element, preventing the infinite layout-flip edge case described in the changeset.
Security Risks
None. This is entirely UI layout measurement logic with no auth, data handling, or external inputs involved.
Level of Scrutiny
Low. The change is contained to a single internal hook, the logic is well-commented and mechanically sound, no visual regressions (Chromatic: 0 changed snapshots), and the bundle delta is negligible (+73 B). The lifecycle is correctly managed — the observer is disconnected on cleanup and the effect re-runs appropriately when (which captures ) changes.
Other Factors
No bugs were flagged by automated review. The fix directly matches the described root cause (measuring before icons render). No outstanding reviewer comments.
| // Store the container width needed to show tabs without | ||
| // overflow. Accounts for any gap (padding/margin) between | ||
| // the container and the scrollable element. | ||
| tabsWidthRef.current = | ||
| scrollableWrapper.scrollWidth + | ||
| (container.clientWidth - scrollableWrapper.clientWidth); |
There was a problem hiding this comment.
This wasn't related to the test flakiness, but an improvement to avoid an edge case where the layout constantly changes between the dropdown and horizontal tabs layout when the root container has padding/margin. It now accounts for additional spacing between the container and scrollable element
| @@ -89,8 +92,12 @@ export function useResponsiveLayout( | |||
| scrollableWrapper.clientWidth; | |||
|
|
|||
| if (hasOverflow) { | |||
| // Store the width before switching | |||
| tabsWidthRef.current = scrollableWrapper.scrollWidth; | |||
| // Store the container width needed to show tabs without | |||
| // overflow. Accounts for any gap (padding/margin) between | |||
| // the container and the scrollable element. | |||
| tabsWidthRef.current = | |||
| scrollableWrapper.scrollWidth + | |||
| (container.clientWidth - scrollableWrapper.clientWidth); | |||
| setShowDropdown(true); | |||
| } | |||
| } | |||
| @@ -106,19 +113,13 @@ export function useResponsiveLayout( | |||
| }, [showDropdown, elementWithOverflowRef, containerRef]); | |||
|
|
|||
| React.useEffect(() => { | |||
| // This effect handles the case where the length of tabs or the tabs | |||
| // labels change. This determines whether to switch to the dropdown or | |||
| // tabs layout. | |||
| // When tabsSignature changes and dropdown is shown, reset to horizontal tabs | |||
| // layout so we can re-measure with the new content (label, icon, number of tabs, etc). | |||
| // The MutationObserver handles checking for overflow once the new content | |||
| // is rendered. | |||
| if (showDropdown) { | |||
| // When tabsSignature changes and dropdown is shown, reset to | |||
| // tabs layout so we can re-measure and see if we can switch | |||
| // back | |||
| tabsWidthRef.current = null; | |||
| setShowDropdown(false); | |||
| } else { | |||
| // When tabsSignature changes and tabs layout is shown, check | |||
| // for overflow | |||
| checkOverflow(); | |||
There was a problem hiding this comment.
Previously, when tabsSignature changed when the tabs layout is shown, we would check for overflow. Now, this check for overflow happens in the MutationObserver so that we check for overflow after the DOM has updated
| // Only check for overflow if the container width has changed | ||
| if (containerWidthRef.current !== container.clientWidth) { | ||
| containerWidthRef.current = container.clientWidth; | ||
| checkOverflow(); |
There was a problem hiding this comment.
Not directly related to the SB test flakiness, but we can reduce the number of times we check for overflow by only checking when there is a change in width in the resize observer.
Previously, checkOverflow would also be called whenever the layout changed since the height of the dropdown and horizontal tabs layout were different, so the resize observer would always trigger the checking logic.
| const mutationObserver = new MutationObserver(() => { | ||
| checkOverflow(); | ||
| }); | ||
|
|
||
| mutationObserver.observe(element, { | ||
| childList: true, | ||
| subtree: true, | ||
| characterData: true, | ||
| }); |
There was a problem hiding this comment.
This is where we set up the mutation observer so we check for overflow when a descendant has a change.
This is what should address the SB test flakiness since it will check for overflow after the icons are rendered
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
marcysutton
left a comment
marcysutton
left a comment
There was a problem hiding this comment.
This looks good to me! I do wonder whether there will be noticeable performance impacts of a MutationObserver in a useEffect. But if it fixes the test flakiness and doesn't visibly thrash the layout, seems like a decent trade-off!
@marcysutton The MutationObserver should only be triggered when the horizontal tabs layout changes descendant nodes (adds/removes tabs or adds/removes icons in tabs) or the tab labels change! One thing to note is that the observer is watching the horizontal tabs element, rather than the container element (which contains both horizontal tabs and dropdown tabs). Because of this, the MutationObserver is set up and taken down when the layout changes so that we don't have to watch for unrelated changes when it is using the dropdown layout. Realistically, most of the responsive tabs use cases are static, so the mutation observer is there to handle edge cases! |
3 participants
Summary:
Previously, the Storybook interactions tests run in CI would be flaky for the ResponsiveNavigationTabs Toggle Icon interaction test. The test would sometimes fail because the layout wouldn't properly switch to the dropdown layout after the icons were toggled on. This was because we would check overflow when the tabs data changes with icons, rather than when the icons are rendered. To make it more robust, we check overflow once the tabs layout is rendered and when there are any changes to it or its descendants (using a MutationObserver).
Other improvements include:
checkOverflowlogic only when the width changes in the ResizeObserver to prevent additional callsIssue: WB-2311
Test plan:
?path=/story/packages-tabs-responsivetabs--interactive?path=/story/packages-tabs-responsivenavigationtabs--interactive