perf(core): remove react-transition-group, use CSS data-state machines#3369
Open
orrgottlieb wants to merge 3 commits into
Open
perf(core): remove react-transition-group, use CSS data-state machines#3369orrgottlieb wants to merge 3 commits into
orrgottlieb wants to merge 3 commits into
Conversation
Replace react-transition-group <CSSTransition> usage with explicit data-state machines (entering/entered/exiting/exited) driven by useState + setTimeout, with timing constants sourced from SCSS. Affected components: Counter, Modal, Toast, MultiStepIndicator/StepIndicator. Reduces runtime dependency on react-transition-group and trims the rendered tree (no Transition wrapper). Snapshot for Toast is regenerated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Code Review by Qodo
1. Toast root missing data-vibe
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+212
to
+224
| <Text | ||
| ref={nodeRef} | ||
| id={id} | ||
| data-testid={dataTestId || getTestId(ComponentDefaultTestId.TOAST, id)} | ||
| type="text2" | ||
| element="div" | ||
| color="fixedLight" | ||
| className={cx(classNames, { | ||
| [styles.enterActive]: transitionStage === "entering", | ||
| [styles.exitActive]: transitionStage === "exiting" | ||
| })} | ||
| role="alert" | ||
| aria-live="polite" |
Contributor
There was a problem hiding this comment.
1. Toast root missing data-vibe 📘 Rule violation ⚙ Maintainability
The Toast component’s root rendered element (Text/div) does not include the required data-vibe attribute. This breaks standardized component identification used by design-system tooling.
Agent Prompt
## Issue description
`Toast` renders a root `div` (via `Text element="div"`) without the required `data-vibe` attribute.
## Issue Context
Compliance requires every component to expose a standardized `data-vibe` attribute on its root DOM element for consistent identification and tooling.
## Fix Focus Areas
- packages/core/src/components/Toast/Toast.tsx[211-225]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+140
to
+168
| const [transitionStage, setTransitionStage] = useState<"entering" | "entered" | "exiting" | "exited">( | ||
| show ? "entering" : "exited" | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (show) { | ||
| if (transitionStage === "exited") { | ||
| setTransitionStage("entering"); | ||
| return; | ||
| } | ||
| if (transitionStage === "entering") { | ||
| const timer = setTimeout(() => setTransitionStage("entered"), MODAL_ENTER_MS); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| } else { | ||
| if (transitionStage === "entered" || transitionStage === "entering") { | ||
| setTransitionStage("exiting"); | ||
| return; | ||
| } | ||
| if (transitionStage === "exiting") { | ||
| const timer = setTimeout(() => setTransitionStage("exited"), MODAL_EXIT_MS); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| } | ||
| }, [show, transitionStage]); | ||
|
|
||
| if (!show && transitionStage === "exited") { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
2. Modal reopen stuck 🐞 Bug ≡ Correctness
In Modal and Toast, if show/open flips back to true while transitionStage is exiting, the exit timer cleanup can clear the only scheduled transition to exited and there is no logic to move from exiting back to entering, leaving the component stuck mounted with exit/hidden styles despite being open. Additionally, when opening from exited, each component can render for one frame without the required enter “FROM” styles/classes, causing a visible flash/jump before the enter animation applies.
Agent Prompt
## Issue description
Modal and Toast have a transition state machine edge case and a first-open paint glitch.
1) If `show`/`open` becomes `true` while `transitionStage === "exiting"`, the exit timer cleanup may cancel the only scheduled transition to `exited`, and there is no explicit branch to move from `exiting` back to `entering`, leaving the component stuck mounted in exit/hidden styles while it is supposed to be open.
2) When toggling from closed to open (`exited` → open), each component can render one frame in `transitionStage === "exited"` without the required enter “FROM” classes/styles (Modal: missing `.containerEnter`; Toast: missing `.enterActive`/FROM state), causing a visible flash/jump before the enter animation starts.
## Issue Context
- The current effect logic handles opening only from `exited`/`entering` and closing from `entered`/`entering`/`exiting`, but does not handle `open/show === true` while already `exiting`.
- Cleanup functions can clear timers when props change, which can cancel an in-flight exit transition and leave the state machine without a deterministic way to complete or reverse.
- Modal SCSS relies on `.containerEnter` as the FROM state to prevent a mount flash, so it needs to be present before the first paint on open.
- Toast styling uses keyframes (`slideIn`/`slideOut`) and a base `.toast` transform that is already at the final position, so applying the enter class only after a paint can create a noticeable jump; this is especially relevant in controlled usage where Toast remains mounted and is toggled via `open`.
## Fix Focus Areas
- packages/core/src/components/Modal/Modal/Modal.tsx[137-174]
- packages/core/src/components/Toast/Toast.tsx[178-223]
- packages/core/src/components/Toast/Toast.module.scss[3-130]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
📦 Bundle Size Analysis Changed Components
Unchanged Components
📊 Summary:
|
Update Modal focus lock test to use fake timers so the new CSS-driven exit transition completes before assertions. Import act from @testing-library/react since React 16 doesn't export it directly. Restore Toast snapshot file that was inadvertently removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit 5dca595 |
Counter now animates upward when the value increases (new digit slides in from the bottom, old slides out to the top) and downward when it decreases (new from the top, old to the bottom). Uses @Keyframes so the entrance animation runs reliably without the two-frame trick that CSSTransition provided. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit 5cdb200 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
User description
Summary
react-transition-group <CSSTransition>with explicitdata-statestate machines (entering/entered/exiting/exited) in Counter, Modal, Toast, and MultiStepIndicator/StepIndicator.useState+setTimeout, with timing constants sourced from the corresponding SCSS animation durations.Why
react-transition-groupis a relatively heavy runtime dependency for what these components actually need (a small finite state machine that toggles a CSS class). Inlining the state transitions:<Transition>wrapper render cost,Notes
COUNTER_SWAP_TRANSITION_MS = 150MODAL_ENTER_MS = 250/MODAL_EXIT_MS = 150TOAST_TRANSITION_MS = 400Test plan
react-transition-groupimports inpackages/core/src/components🤖 Generated with Claude Code
PR Type
Enhancement
Description
Remove
react-transition-groupdependency, replace with CSS state machinesuseState+setTimeoutAdd direction-aware Counter animations
@keyframesfor reliable entrance animationsSimplify rendered tree by eliminating
<Transition>wrapper componentsUpdate Modal focus lock test to use fake timers for exit transition completion
Diagram Walkthrough
File Walkthrough
Counter.module.scss
Add directional keyframe animations for counter swappackages/core/src/components/Counter/Counter.module.scss
@keyframesanimationscounterEnterFromBottom,counterEnterFromTop,counterExitToTop,counterExitToBottomfadeEnterUp,fadeEnterDown,fadeExitUp,fadeExitDownanimationproperty withforwardsfill-mode instead oftransitionCounter.tsx
Replace react-transition-group with CSS state machinepackages/core/src/components/Counter/Counter.tsx
react-transition-groupimports (CSSTransition,SwitchTransition)COUNTER_SWAP_TRANSITION_MS = 150constant matching SCSS durationdisplayedText,swapStage, anddirectionstate+wrapper with simpleelementuseEffecthooks to manage exiting → entering statetransitions
Modal.tsx
Replace react-transition-group with CSS state machinepackages/core/src/components/Modal/Modal/Modal.tsx
react-transition-groupimport (CSSTransition)useEffectto importsMODAL_ENTER_MS = 250andMODAL_EXIT_MS = 150constantstransitionStagestate(entering/entered/exiting/exited)
wrapper with conditional rendering based on transition stageuseEffecthook managing enter/exit state transitionsnullwhen not showing and fully exitedStepIndicator.tsx
Replace react-transition-group with CSS state machinepackages/core/src/components/MultiStepIndicator/components/StepIndicator/StepIndicator.tsx
react-transition-groupimports (CSSTransition,SwitchTransition)STEP_INDICATOR_SWAP_TRANSITION_MS = 150constantdisplayedStatus,swapStagestate+wrapper with simpleelementuseEffecthooks managing exiting → entering statetransitions
displayedStatustoStepCircleDisplayinstead of currentstatusToast.tsx
Replace react-transition-group with CSS state machinepackages/core/src/components/Toast/Toast.tsx
react-transition-groupimport (CSSTransition)useStateto React importsTOAST_TRANSITION_MS = 400constant matching original timeouttransitionStagestate(entering/entered/exiting/exited)
wrapper with directelementuseEffecthook managing enter/exit state transitionsnullwhen not open and fully exitedModal.test.tsx
Update modal test for CSS state machine transitionspackages/core/src/components/Modal/Modal/tests/Modal.test.tsx
react-transition-groupmock that was bypassing transitionsactfrom@testing-library/reactinstead of relying on Reactexport
vi.useFakeTimers()to focus lock release test to allow exittransition to complete
act()callvi.useRealTimers()cleanup after testToast.snapshot.test.tsx
Remove transition mock and update snapshotspackages/core/src/components/Toast/tests/Toast.snapshot.test.tsx
react-transition-groupmock that was bypassing transitionsviimport since mocking is no longer neededhideIconcase to includeopenprop forconsistency
wrapper