[No QA] perf: React Compiler Batch 3: setState-in-effect warnings#83382
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
This comment has been minimized.
This comment has been minimized.
|
npm has a |
Replace useEffect+setState with React's recommended "adjust state during rendering" pattern for resetting imageError when source changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern. Remove useCallback and React.memo since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Use controlled/uncontrolled pattern instead of syncing prop to state via useEffect. When value prop is provided, use it directly. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern. Remove useMemo since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Derive error visibility during render instead of clearing error state in a useEffect. The error is shown only when both checkboxes are not yet accepted, matching the original behavior without the effect. Co-authored-by: Cursor <cursoragent@cursor.com>
Compute freeTrialText inline during render instead of storing it in state and updating via useEffect. Remove useState since the value is purely derived from props and Onyx data. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for syncing task title and description from Onyx. Remove useMemo wrappers since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Move error message clearing from useEffect to render-time using a composite key of task fields. Narrow the remaining effect to only handle parentReportID changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Move the synchronous time update from useEffect to render-time using prev-timezone tracking. Keep only the interval in the effect. Remove useCallback and useMemo since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for syncing dateOption and startDate from Onyx card data. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern. Remove useCallback and useMemo since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for setting the skip-selection-change flag when formattedAmount changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for updating isRendered and shared value when isExpanded changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for filtering selected options when options or filter function change. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for detecting new card IDs. Remove useCallback since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Move synchronous dimension sync from useEffect to render-time for non-web platforms. Replace playback state useEffect+setState with adjust-state-during-render pattern for thumbnail toggling. Co-authored-by: Cursor <cursoragent@cursor.com>
Move the synchronous desktop shift reset from useLayoutEffect to render-time. Keep async measurement in useLayoutEffect for mobile. Remove useMemo since the file now compiles with React Compiler. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for initializing userSelectedCountry from countryDefaultValue. Co-authored-by: Cursor <cursoragent@cursor.com>
Move animation reset state updates from useEffect to render-time. Replace DOM measurement in effect with a callback ref that measures width when the element mounts. Keep only the delayed hide timer in the effect. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern. Dismiss the modal when currency changes to USD instead of on every render cycle. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for syncing selectedItems when initiallySelectedItems changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for updating currentLocale when translations finish loading. Co-authored-by: Cursor <cursoragent@cursor.com>
Move isFetchingData state updates from useEffect to render-time. Keep only the API call (openPolicyAccountingPage) in the effect. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace useEffect+setState with adjust-state-during-render pattern for resetting isLoadingMore when reportsLimit changes. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@chuckdries I don't think running through all the test steps here on all platforms is really necessary because this has already been QA'd by Applause. I was able to repro and fix the couple bugs they reported. |
…ge hydration The previous useInitialValue hook captured cardList on the first render, which could be empty if Onyx hadn't hydrated yet. The new useInitialOnyxValue hook subscribes to an Onyx key and captures its value once the status first transitions to 'loaded', giving a stable snapshot unaffected by hydration timing. Made-with: Cursor
Defer rendering SearchMultipleSelectionPicker until the search advanced filters form has loaded from Onyx, preventing initiallySelectedItems from being captured as empty during initial hydration. Made-with: Cursor
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c16a7eab12
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
chuckdries
left a comment
There was a problem hiding this comment.
Mostly a huge fan of this, just had a few questions that are probably me being unfamiliar with react compiler
…mModal Replace local ConfirmModal and useDismissModalForUSD with the global useConfirmModal system. Auto-dismiss on external currency change to USD is handled via useEffect. Remove all manual memoization since the component compiles with React Compiler. Delete now-unused useDismissModalForUSD hook and its tests. Made-with: Cursor
Remove the complex featureStates local state buffer and derive all values directly from the Onyx policy object. The highlight animation for newly enabled features is now computed purely during render by comparing current and previous pendingFields. This eliminates ~40 lines of diffing/buffering logic, removes useState, and reorganizes the component for readability. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.33-5 🚀
|
Explanation of Change
This PR resolves 46
react-hooks/set-state-in-effectReact Compiler ESLint warnings across ~30 files, reducing--max-warningsfrom 381 to 335.The primary refactoring pattern used is "adjust state during render" (React docs): instead of calling
setStateinside auseEffectto sync props/Onyx data to local state, we track previous dependency values inuseStateand conditionally update state during the render phase. This avoids cascading renders from synchronoussetStatein effects.Additional cleanup per file:
useMemo,useCallback, andReact.memofrom files that now compile cleanly with React Compiler (manual memoization is unnecessary and counterproductive when the compiler handles it)Fixed Issues
(partial) #68765
Tests
npm run lintpasses with--max-warnings=346(down from 383)npx eslint <file> --max-warnings=0Relevant BrowserStack Test Cases (PR-51):
Workspace features (
WorkspaceInitialPage,WorkspaceOverviewPage,withPolicyConnections):Accounting connections (
withPolicyConnections):Expensify Card (
ExpensifyCardContextProvider):Company cards (
AmexCustomFeed,TransactionStartDateStep):Virtual/lost card (
ReportCardLostConfirmMagicCodePage):Avatar (
Avatar.tsx):Image viewing (
Lightbox,ImageView,ThumbnailImage):Video (
VideoPlayerPreview):Drag and drop (
useDragAndDrop):Search filters (
SearchMultipleSelectionPicker):Subscription/wallet (
FreeTrial,WalletStatementPage,TermsStep):Tasks (
NewTaskDetailsPage,NewTaskPage):Settlement (
AnimatedSettlementButton):Locale/internationalization (
LocaleContextProvider):Offline tests
N/A — no behavioral changes, only refactoring how state is synchronized
QA Steps
N/A — [No QA] refactoring-only change with no behavioral differences
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
N/A — refactoring only, no UI changes
Android: mWeb Chrome
N/A — refactoring only, no UI changes
iOS: Native
N/A — refactoring only, no UI changes
iOS: mWeb Safari
N/A — refactoring only, no UI changes
MacOS: Chrome / Safari
N/A — refactoring only, no UI changes
Made with Cursor