Refactor: split contexts to state and actions for SearchContext#83206
Conversation
sharabai
force-pushed
the
refactor/split-contexts-to-state-and-actions-for-SearchContext
branch
3 times, most recently
from
c717d0c to
ac75e7e
Compare
|
@codex review |
![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: c6caf65fa1
ℹ️ 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".
| const searchActionsContextValue: SearchContextActionsValue = { | ||
| removeTransaction, | ||
| setCurrentSearchHashAndKey, | ||
| setCurrentSearchQueryJSON, | ||
| setSelectedTransactions, |
There was a problem hiding this comment.
searchActionsContextValue is rebuilt on every SearchContextProvider render, and setCurrentSearchHashAndKey is also recreated each render, so the actions context value changes by reference even when nothing meaningful changed. Because context propagation is reference-based, this forces all useSearchActionsContext consumers to re-render on any parent/provider re-render, and in Search/index.tsx it also churns the useFocusEffect callback dependencies and repeatedly re-runs the focus effect while focused. This undermines the state/actions split and creates avoidable render/effect work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
React compiler should handle those cases
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
sharabai
force-pushed
the
refactor/split-contexts-to-state-and-actions-for-SearchContext
branch
2 times, most recently
from
a780071 to
3c1fb69
Compare
staszekscp
left a comment
staszekscp
left a comment
There was a problem hiding this comment.
Looking good, just two small comments!
| useLayoutEffect(() => { | ||
| searchContextDataRef.current = searchContextData; | ||
| }, [searchContextData]); |
There was a problem hiding this comment.
Why do you use useLayoutEffect instead of useEffect?
There was a problem hiding this comment.
If useEffect is used, there's a chance that an unnecessary re-render would happen.
searchContextDataRef is used in clearSelectedTransactions in a way of early return if there's no data to clear in order to prevent an unnecessary setState call, which would trigger re-render.
src/components/Search/index.tsx in turn uses clearSelectedTransations in a useFocusEffect. src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx uses it in useFocusEffect as well.
clearSelectedTransactions is used in their useEffect and since those are nested deeper in the tree, their useEffect would fire first with a stale value if we used useEffect
|
|
||
| const setSelectedTransactions: SearchContextProps['setSelectedTransactions'] = useCallback((selectedTransactions, data = []) => { | ||
| const setSelectedTransactions: SearchContextActionsValue['setSelectedTransactions'] = (selectedTransactions, data = []) => { |
There was a problem hiding this comment.
If you've decided to switch from arrow functions to function declarations I think all the methods should be aligned to use it - now we get a mix. Personally I would stick here to arrow functions, since it aligns better to the rest of the code (we use arrow functions within components) and the difference is strictly stylistic
There was a problem hiding this comment.
Alright, I'll change those back to arrow functions to be consistent. I was wrongly trying to follow STYLES.md when it explained preferred styles for functions in library modules, but we're in Components.
sharabai
force-pushed
the
refactor/split-contexts-to-state-and-actions-for-SearchContext
branch
from
3c1fb69 to
4562868
Compare
staszekscp
left a comment
staszekscp
left a comment
There was a problem hiding this comment.
One last comment and we're good to go!
sharabai
force-pushed
the
refactor/split-contexts-to-state-and-actions-for-SearchContext
branch
2 times, most recently
from
a7dbe04 to
0388d92
Compare
melvin-bot
Bot
requested review from
joekaufmanexpensify and
mkhutornyi
and removed request for
a team
|
@mkhutornyi Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
sharabai
changed the title
sharabai
changed the title
sharabai
force-pushed
the
refactor/split-contexts-to-state-and-actions-for-SearchContext
branch
from
0388d92 to
cb4f518
Compare
|
Please fix conflict |
sharabai
force-pushed
the
refactor/split-contexts-to-state-and-actions-for-SearchContext
branch
from
cb4f518 to
8d3ae7c
Compare
|
@mkhutornyi Conflicts are resolved! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4f5183cb
ℹ️ 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".
| const searchActionsContextValue: SearchActionsContextValue = { | ||
| removeTransaction, | ||
| setCurrentSearchHashAndKey, | ||
| setCurrentSearchQueryJSON, | ||
| setSelectedTransactions, |
There was a problem hiding this comment.
The new actions context value is rebuilt from non-memoized inline handlers on every render, so clearSelectedTransactions, setCurrentSearchHashAndKey, etc. now get a new identity each time the provider renders. Several consumers use these handlers in effect dependencies (for example useFocusEffect/useEffect paths in src/components/Search/index.tsx), which causes repeated teardown/re-run cycles and can repeatedly execute selection-reset side effects while focused. This also defeats the intended rerender reduction from splitting state and actions contexts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
False alarm. Auto memoized by React Compiler
| [route.name, context, reportID, allTransactions, allReportActions, ancestors, isDelegateAccessRestricted, showDelegateNoAccessModal], | ||
| [ | ||
| route.name, | ||
| selectedTransactionIDs, | ||
| selectedTransactions, |
There was a problem hiding this comment.
NAB (as existing code): This component is react compilable so we can remove useCallback
| [context, allPolicies, allReports, route.name, selectedTransactionsForReject, isDelegateAccessRestricted, currentUserAccountID, showDelegateNoAccessModal, betas], | ||
| [ | ||
| currentSearchHash, | ||
| clearSelectedTransactions, | ||
| allPolicies, |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-25.at.3.51.51.pm.mov |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
🚧 @mountiny 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/mountiny in version: 9.3.26-0 🚀
|
|
Hi @sharabai Could you please share steps for QA team? |
|
@izarutskaya I think generic regression testing around the Search will cover this |
mountiny
changed the title
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.26-8 🚀
|
sharabai
changed the title
6 participants
Explanation of Change
Migration of the Search Context to follow state/actions split pattern:
Fixed Issues
$ #80271
PROPOSAL:
Tests
The following flow should test that new state and actions contexts do their job correctly
Offline tests
QA Steps
Same as tests
Offline tests
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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-native-83206.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS-web-83206.mp4