fix storybook issues#90381
Conversation
|
@MelvinBot storybook preview background is white. Which PR caused this? It was working in #78416. 
|
|
#78416 caused this. That PR replaced - addons: ['@storybook/addon-essentials', '@storybook/addon-a11y', '@storybook/addon-webpack5-compiler-babel'],
+ addons: ['@storybook/addon-a11y', '@storybook/addon-webpack5-compiler-babel', '@storybook/addon-docs'],
Fix: Either add |
|
App/src/stories/Form.stories.tsx Lines 104 to 109 in 03d9e7c @MelvinBot this line causes App/src/stories/Form.stories.tsx Lines 156 to 160 in 03d9e7c @MelvinBot this line caused |
|
1. "Too many re-renders" (AddressSearch / lines 104-109) Caused by #49700 (merged 2024-09-27). That PR migrated Fix: Move the Onyx setter calls into a 2. "Couldn't find values for screen ScreenWrapper transition status" (StateSelector / lines 156-160) Caused by #40369 (merged 2024-04-26). That PR added Fix: Add a |
|
@MelvinBot suggest code changes for #90381 (comment) |
|
Here are the suggested code changes for both issues: 1. Fix "Too many re-renders" in Form.stories.tsxWrap the Onyx setter calls in - import React, {useState} from 'react';
+ import React, {useEffect, useState} from 'react'; function Template(props: FormProviderProps & FormProviderOnyxProps) {
- // Form consumes data from Onyx, so we initialize Onyx with the necessary data here
- setIsLoading(props.formID, !!props.formState?.isLoading);
- setDraftValues(props.formID, props.draftValues);
-
- if (props.formState?.error) {
- setErrors(props.formID, {error: props.formState.error as string});
- } else {
- clearErrors(props.formID);
- }
+ useEffect(() => {
+ setIsLoading(props.formID, !!props.formState?.isLoading);
+ setDraftValues(props.formID, props.draftValues);
+
+ if (props.formState?.error) {
+ setErrors(props.formID, {error: props.formState.error as string});
+ } else {
+ clearErrors(props.formID);
+ }
+ }, [props.formID, props.formState?.isLoading, props.formState?.error, props.draftValues]);Apply the same change to 2. Fix "ScreenWrapper transition status" error in StorybookAdd a + import ScreenWrapperStatusContext from '@components/ScreenWrapper/ScreenWrapperStatusContext'; >
- <Story />
+ <ScreenWrapperStatusContext.Provider value={{didScreenTransitionEnd: true, isSafeAreaTopPaddingApplied: false, isSafeAreaBottomPaddingApplied: false}}>
+ <Story />
+ </ScreenWrapperStatusContext.Provider>
</ComposeProviders>
|
@MelvinBot this happens even after removing this code. |
|
Agreed — the root cause is a cyclic Primary cause:
Additionally, To fix:
This should make these components compatible with React Compiler without needing an exclusion. |
|
@MelvinBot suggest code changes for #90381 (comment) |
|
Here are the concrete code changes for all three files: 1.
|
@MelvinBot I fixed this by removing App/src/hooks/useAutoFocusInput.ts Line 83 in 03d9e7c The race condition was repetitive setting true/false in But we should not remove this line. What is alternative solution? |
|
The root cause isn't line 83 itself — it's that the Fix: Hoist the mock object so const navigationMock = {
navigate: () => {},
getState: () => ({routes: []}),
addListener,
};
const useNavigation = isJestEnv
? realReactNavigation.useNavigation
: () => navigationMock;This keeps line 83 intact while breaking the re-render cycle. |
|
@MelvinBot your fix still not working |
|
My previous suggestion was incomplete — the stable navigation mock alone isn't enough. There are two problems in the Problem 1 (my previous suggestion): Problem 2 (the main issue): Fix: Change the import {useEffect} from 'react';
// Stable reference so useNavigation doesn't cause re-renders
const navigationMock = {
navigate: () => {},
getState: () => ({routes: []}),
addListener,
};
const useNavigation = isJestEnv
? realReactNavigation.useNavigation
: () => navigationMock;
// Run callback in useEffect (like real useFocusEffect), not synchronously during render
const useFocusEffect = isJestEnv
? realReactNavigation.useFocusEffect
: (callback: () => (() => void) | void) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => callback(), [callback]);
};This way the callback only runs once (since |
@MelvinBot investigate failing tests |
|
The The test mocks both modules separately. The Fix: Add useNavigation: () => ({
navigate: jest.fn(),
addListener: jest.fn(),
getState: jest.fn(() => undefined),
}),Also, regarding my earlier comment — the same principle applies to your global mock at |
2 participants
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari