Repository navigation
[No QA] Disallow manual memoization in compiled React code - #104522
Conversation
React Navigation invokes placeholder renderers directly. Mount the placeholder through a hook-free callback so its theme subscription does not change the scene hook order.
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
…memoization-104514 # Conflicts: # src/pages/ReportSubmitToContent.tsx # src/pages/workspace/rules/FlagForReviewRules/FlagForReviewRulePageBase.tsx
Track effect dependencies with the names key instead of a transient array. Derive requested names inside the effect so resolving a cold asset load does not restart it on every render.
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
…memoization-104514 # Conflicts: # src/components/LHNOptionsList/LHNOptionsList.tsx # src/components/LHNOptionsList/OptionRowLHN/OptionRowLHNData.tsx # src/components/MapView/MapView.tsx # src/components/MapView/MapViewImpl.web.tsx # src/components/Modal/ReanimatedModal/Backdrop/index.web.tsx # src/components/MoneyRequestReportView/MoneyRequestReportTransactionsNavigation.tsx # src/components/MoneyRequestReportView/useMoneyRequestReportSortedTransactions.ts # src/components/MultiGestureCanvas/useCanvasPinchGesture.ts # src/components/MultiGestureCanvas/useCanvasTapGestures.ts # src/components/PopoverProvider/index.tsx # src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx # src/components/Search/SearchAutocompleteList.tsx # src/components/Search/SearchList/BaseSearchList/index.native.tsx # src/components/Search/SearchList/BaseSearchList/index.tsx # src/components/Search/SearchRouter/SearchRouter.tsx # src/components/Search/hooks/useSearchSnapshot.ts # src/components/TagPicker/index.tsx # src/hooks/useOriginalReportID.ts # src/pages/EnablePayments/Pay/Onfido/OnfidoStep.tsx # src/pages/RoomDescriptionPage.tsx # src/pages/Search/SearchMoneyRequestReportPage.tsx # src/pages/inbox/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx # src/pages/iou/request/step/DynamicIOURequestStepTag.tsx # src/pages/iou/request/step/IOURequestEditReportCommon.tsx # src/pages/media/AttachmentModalScreen/AttachmentModalBaseContent/index.tsx # src/pages/media/AttachmentModalScreen/routes/report/ReportAddAttachmentModalContent/index.tsx # src/pages/workspace/WorkspaceMembersPage.tsx # src/pages/workspace/expensifyCard/WorkspaceExpensifyCardListPage.tsx # src/pages/workspace/invoices/WorkspaceInvoiceVBASection.tsx # src/pages/workspace/tags/WorkspaceTagsPage.tsx
React Navigation warns when useFocusEffect gets an unmemoized callback. Also let the lint rule allow useCallback for useFocusEffect imported from @react-navigation/core.
|
Regression tests completed with no issues found 🎉 |
roryabraham
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10e49e9c1d
ℹ️ 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".
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
regression testing successfully completed, there are conflicts
Expo's environment index signature leaves TZ typed as any. Declare its optional string type so timezone save/restore tests pass ESLint without casts or suppressions.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 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.5.24-1 🚀
|
|
No help site update needed. This PR is an internal refactor that removes manual memoization and adds a lint rule, so no feature, label, or workflow changes for users, and no article sentence becomes incorrect. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.5.24-8 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
React Compiler already owns memoization in compiled components and hooks. Manual wrappers are unnecessary and can prevent the compiler from improving memoization.
rulesdir/no-manual-memoization, enforced only when both Babel and OXC fully compile and memoize the file. Keep theuseCallbackexception for React Navigation'suseFocusEffect.React.memo,useMemo, anduseCallbackwrappers, including the eight files whose compilation failed solely because of existing manual memoization. Those eight now compile with both compilers. Files with unrelated compiler failures remain out of scope..claude/skills/app-coding-standards/rules/clean-react-0-compiler.md, and its references.AI Tests
--no-cache), changed-file lint, type checking, Babel/OXC compiler compliance, formatting, spelling, and the Storybook smoke test.SentryHtmlTreeAsStringTest.tsfail identically in the original checkout; this dependency-patch issue is unchanged and out of scope. Worktree-only Jest resolution overrides were used for shared dependencies; no repository Jest configuration was changed.Main merge revalidation (2026-10-21)
e8dad8b4bebin the existing worktree and resolved 30 conflicted files, preserving upstream behavior and this PR's memoization removal.bun ./scripts/lint/index.ts --no-cache, changed-file lint, type checking, Babel/OXC compiler compliance, repository-wide formatting, changed-file spelling, the Storybook smoke test, all 658 Bun tests, and 152 focused Jest tests across nine suites. All 30 conflicted files also pass focused lint; an AST comparison confirms all 61 upstream memoized callbacks retain their behavior.NoDirectPersonalDetailsListRuleTest.ts(Cannot assign to read only property '__private_3_steps'). The same failures reproduce in the existing baseline worktree atbc54f602df7; the test, rule, transformer, Jest configuration, and dependency lockfile are unchanged from currentmain.Main merge revalidation (2026-10-26)
f386f2399cfin the existing worktree and resolved eight conflicted files, retaining the latestmainbehavior without restoring redundant manual memoization.react-native-vision-camerais installed at 4.7.2 (requires 5.2.3), andreact-native-onyxis installed at 3.0.117 (requires 3.0.119). Changed-file spelling reports the existingrecognisestypo intests/unit/AccountingConnectionConfirmationTest.tsx:219, which is identical tomain.JavaScript action build verification (2026-10-26)
preserveSymlinksoption so shared worktree dependencies produce the same module paths as a clean CI install. Rebuilt 25 action bundles; their only changes are dependency paths from../../App/node_modules/tonode_modules/.awesome-phonenumber5.4.0 was installed instead of the locked 6.11.0. Rebuilt the two affected bundles with 6.11.0 and the repository's existing patch, without changing the shared dependency install..github/scripts/verifyActions.shwith a repeat rebuild producing no diff, focused lint, formatting, spelling, andnpm run typecheck -- tsconfig.node.json.Export dropdown review fix (2026-10-26)
defaultSelectedIndex, without changing the shared dropdown or restoring manual memoization.Latest main merge revalidation (2026-10-26)
a611f4ca31din the existing worktree, resolving five conflicts while retaining the latest new-report and workspace-task completion behavior without restoring manual memoization. The export-selection review fix and its tests are unchanged.Conflict resolution and ESLint repair (2026-10-26)
e731b9b7a9d, resolving the two search-owner/per-diem conflicts while preserving upstream cardholder resolution and per-diem error handling without restoring manual memoization. The export-selection fix is unchanged.process.env.TZasanyin the date/transaction tests. Added the optional string declaration insrc/types/env.d.ts; both affected test files now pass cache-free lint without casts or suppressions.Fixed Issues
$ #104514
Tests
Relying primarily on Applause regression tests.
Offline tests
Same as tests.
QA Steps
None - relying on regression tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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